Booking confirmed#84
Conversation
📝 WalkthroughWalkthroughDeletes the Changes
Sequence Diagram(s)(Skipped) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/main/java/frontend/controller/BookingController.java`:
- Around line 76-78: The call dialogPane.lookup(".content.label") can return
null and cause a NullPointerException when setStyle() is invoked; update the
code around DialogPane dialogPane = alert.getDialogPane() to capture the lookup
result into a variable (e.g., Node contentLabel =
dialogPane.lookup(".content.label")), check contentLabel for null before calling
setStyle(), and only call setStyle() when contentLabel is non-null (optionally
log or ignore when null) to avoid the NPE.
🧹 Nitpick comments (3)
src/main/java/frontend/controller/BookingController.java (3)
43-48: Consider usingisBlank()instead ofisEmpty()for text validation.
isEmpty()returnsfalsefor whitespace-only strings like" ", which would pass validation but likely isn't intended. UsingisBlank()(Java 11+) orgetText().trim().isEmpty()would catch this edge case.Suggested improvement
- if (firstNameField.getText().isEmpty() || lastNameField.getText().isEmpty() || + if (firstNameField.getText().isBlank() || lastNameField.getText().isBlank() || date == null || startTime == null) {
86-86: Inconsistent spinner default value.The spinner is initialized with a default value of
2ininitialize()(line 31), but after a successful booking, it's reset to1. Consider using the same default value for consistency.Suggested fix
- guestsSpinner.getValueFactory().setValue(1); + guestsSpinner.getValueFactory().setValue(2);
90-93: Consider user-friendly error messages.Displaying raw exception messages (
e.getMessage()) may expose internal details or show technical jargon to end users. For production, consider mapping exceptions to user-friendly messages while logging the full details for debugging.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/backend/entities/Customer.javasrc/main/java/frontend/controller/BookingController.java
💤 Files with no reviewable changes (1)
- src/main/java/backend/entities/Customer.java
🔇 Additional comments (1)
src/main/java/frontend/controller/BookingController.java (1)
29-35: LGTM!The condensed spinner initialization is cleaner, and the time range setup (12:00-20:00) is appropriate for restaurant booking hours.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/main/java/frontend/controller/BookingController.java`:
- Line 31: The guests spinner default is inconsistent: it’s initialized with
SpinnerValueFactory.IntegerSpinnerValueFactory(1, 8, 2) (default 2) but later
reset to 1 after booking; update one side so both use the same default (pick
either default 1 or 2) — locate the initialization of guestsSpinner (where
SpinnerValueFactory.IntegerSpinnerValueFactory is used) and the post-booking
reset (the code that sets the spinner value after booking, e.g.,
guestsSpinner.getValueFactory().setValue(...) or a new ValueFactory) and make
them match (set the reset to the same default as the initial factory or change
the initial factory default to the reset value).
- Around line 52-74: BookingController uses restaurant from
RestaurantHandler.getCurrentRestaurant() without checking for null before
calling bookingService.bookTable(...) and restaurant.getName(), which can cause
NPEs; add a null check for restaurant at the start of the booking flow (before
calling bookTable and before referencing restaurant.getName()), and if null, set
statusLabel to an error message (red) and show an Alert with a clear message to
select a restaurant, then return early so bookTable and subsequent uses of
restaurant are never called.
🧹 Nitpick comments (1)
src/main/java/frontend/controller/BookingController.java (1)
93-96: Consider sanitizing exception messages for user display.Displaying
e.getMessage()directly could expose internal error details. If theBookingServicethrows exceptions with technical messages, consider providing a user-friendly fallback message.Optional: User-friendly error handling
} catch (Exception e) { - statusLabel.setText(e.getMessage()); + String userMessage = e.getMessage() != null ? e.getMessage() : "An error occurred. Please try again."; + statusLabel.setText(userMessage); statusLabel.setStyle("-fx-text-fill: red;"); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/frontend/controller/BookingController.java
🔇 Additional comments (3)
src/main/java/frontend/controller/BookingController.java (3)
43-48: LGTM on validation logic.The required field validation covers the essential booking information. Phone and email appear to be intentionally optional, which is acceptable for the booking flow.
85-91: LGTM on form cleanup.The form reset logic correctly clears all fields after the confirmation alert is shown, ensuring the alert displays the booking details before clearing.
77-81: Good null-check fix.The null check addresses the previous NPE concern. The CSS selector
.content.labelcorrectly targets a DialogPane content label element that has both thecontentandlabelstyle classes. This is the proper selector syntax for an element with multiple classes.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| new SpinnerValueFactory.IntegerSpinnerValueFactory(1, 8, 2) | ||
| ); | ||
|
|
||
| guestsSpinner.setValueFactory(new SpinnerValueFactory.IntegerSpinnerValueFactory(1, 8, 2)); |
There was a problem hiding this comment.
Inconsistent spinner default values.
The spinner is initialized with a default of 2 guests (line 31), but after booking it resets to 1 (line 89). Consider using a consistent default value.
Proposed fix to use consistent default value
- guestsSpinner.getValueFactory().setValue(1);
+ guestsSpinner.getValueFactory().setValue(2);Also applies to: 89-89
🤖 Prompt for AI Agents
In `@src/main/java/frontend/controller/BookingController.java` at line 31, The
guests spinner default is inconsistent: it’s initialized with
SpinnerValueFactory.IntegerSpinnerValueFactory(1, 8, 2) (default 2) but later
reset to 1 after booking; update one side so both use the same default (pick
either default 1 or 2) — locate the initialization of guestsSpinner (where
SpinnerValueFactory.IntegerSpinnerValueFactory is used) and the post-booking
reset (the code that sets the spinner value after booking, e.g.,
guestsSpinner.getValueFactory().setValue(...) or a new ValueFactory) and make
them match (set the reset to the same default as the initial factory or change
the initial factory default to the reset value).
| Booking booking = bookingService.bookTable( | ||
| restaurant, | ||
| firstNameField.getText(), | ||
| lastNameField.getText(), | ||
| emailField.getText(), | ||
| phoneField.getText(), | ||
| guestsSpinner.getValue(), | ||
| startTime, | ||
| endTime, | ||
| date | ||
| restaurant, | ||
| firstNameField.getText(), | ||
| lastNameField.getText(), | ||
| emailField.getText(), | ||
| phoneField.getText(), | ||
| guestsSpinner.getValue(), | ||
| startTime, | ||
| endTime, | ||
| date | ||
| ); | ||
|
|
||
| statusLabel.setText( | ||
| "Booking confirmed! ID: " + booking.getId() | ||
| statusLabel.setText("Booking confirmed! ID: " + booking.getId()); | ||
| statusLabel.setStyle("-fx-text-fill: green;"); | ||
|
|
||
| Alert alert = new Alert(Alert.AlertType.INFORMATION); | ||
| alert.setTitle("Booking confirmed"); | ||
| alert.setHeaderText(null); | ||
| alert.setContentText( | ||
| "Booking confirmed for " + firstNameField.getText() + " " + lastNameField.getText() + "\n" + | ||
| "Restaurant: " + restaurant.getName() + "\n" + | ||
| "Date & Time: " + date + " at " + startTime + "\n" + | ||
| "Guests: " + guestsSpinner.getValue() |
There was a problem hiding this comment.
Potential NullPointerException if restaurant is null.
restaurant is initialized from RestaurantHandler.getCurrentRestaurant() at line 16 and used without a null check. If no restaurant is selected, restaurant.getName() at line 72 (and the booking call at line 52) will throw an NPE.
Proposed fix to add null check in validation
- if (firstNameField.getText().isEmpty() || lastNameField.getText().isEmpty() ||
- date == null || startTime == null) {
+ if (restaurant == null || firstNameField.getText().isEmpty() || lastNameField.getText().isEmpty() ||
+ date == null || startTime == null) {
statusLabel.setText("Please fill in all required fields");
statusLabel.setStyle("-fx-text-fill: red;");
return;
}🤖 Prompt for AI Agents
In `@src/main/java/frontend/controller/BookingController.java` around lines 52 -
74, BookingController uses restaurant from
RestaurantHandler.getCurrentRestaurant() without checking for null before
calling bookingService.bookTable(...) and restaurant.getName(), which can cause
NPEs; add a null check for restaurant at the start of the booking flow (before
calling bookTable and before referencing restaurant.getName()), and if null, set
statusLabel to an error message (red) and show an Alert with a clear message to
select a restaurant, then return early so bookTable and subsequent uses of
restaurant are never called.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.