Added docStrings and removed redundant todo comments#9
Conversation
Co-authored-by: Andreas Kaiberger <andreas.kaiberger@gmail.com> Co-authored-by: Martin Karlsson <Martin.Karlsson@iths.se> Co-authored-by: Rickard Ankar <rickard.ankar@iths.se>
📝 WalkthroughWalkthroughThis PR adds booking-management APIs to the repository, enriches CLI Menu validations and JavaDoc, removes Hotel room lifecycle stubs, adjusts Hibernate logging in App, and expands seed data in import.sql. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Client (Menu)
participant Service as BookingService
participant Repo as BookingRepository
participant DB as Database
User->>Service: request createBooking(emails, start, end, guests)
Service->>Repo: validateValues(...) / getEmptyRooms(start,end,guests)
Repo->>DB: query available rooms
DB-->>Repo: room list
Repo-->>Service: chosenRoom or none
alt room found
Service->>Repo: create(guestEmails,start,end,guests,totalPrice)
Repo->>DB: insert Booking, link Guests, assign Room
DB-->>Repo: persist confirmation
Repo-->>Service: true
Service-->>User: success message / booking id
else no room
Service-->>User: no availability message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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)
🔇 Additional comments (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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/example/BookingRepository.java (1)
140-163: Race condition: use persisted entity's ID instead ofmax(b.id)query.After
em.persist(booking), the entity's ID is assigned (if usingGenerationType.IDENTITYor after flush). Queryingmax(b.id)is unreliable under concurrent inserts—another transaction could insert between your persist and the max query. Usebooking.getId()directly after persist (with flush if needed).🔧 Suggested fix
em.persist(booking); + em.flush(); // Ensure ID is assigned - Query query = em.createQuery("select max(b.id) from Booking b"); - Long bookingId = (Long) query.getSingleResult(); for (String email : emailList) em.createNativeQuery("insert into guestBooking (guest_id, booking_id) values (?, ?)") .setParameter(1, guestRepository.get(email).getId()) - .setParameter(2, bookingId) + .setParameter(2, booking.getId()) .executeUpdate();src/main/resources/import.sql (1)
5-19: Add composite unique constraint onroomNumberandhotel_idto Room entity.The
REPLACE INTO Roomstatement relies on a unique constraint to detect and replace duplicate entries. Currently, the primary key is the auto-incrementidfield, soREPLACE INTOgenerates new IDs on each import rather than replacing existing rooms. TheroomNumberfield is marked with@NaturalIdbut lacks the composite unique constraint on(roomNumber, hotel_id)needed for REPLACE INTO to work correctly. The TODO comment on line 14 of Room.java ("make unique for each hotel") confirms this is a known issue. Without this constraint, repeated imports will create duplicate rooms.
🤖 Fix all issues with AI agents
In `@src/main/java/org/example/BookingRepository.java`:
- Around line 86-90: The JavaDoc for the method in BookingRepository that
converts Guest objects to GuestInfo incorrectly states "Returns a List of
BookingInfo"; update the `@return` line to "Returns a List of GuestInfo." Locate
the method that transforms Guest -> GuestInfo (referencing GuestInfo and the
converter method in BookingRepository) and change the JavaDoc return description
to match the actual return type List<GuestInfo>.
In `@src/main/java/org/example/Menu.java`:
- Around line 288-293: The JavaDoc for the Menu method that accepts parameters
firstName and lastName is incorrect: it claims a null check but the
implementation uses isEmpty() to check for empty strings. Update the method's
JavaDoc to state that it validates that firstName and lastName are non-empty
(not empty strings) and adjust the `@return` description to say it returns false
if either parameter is empty (""), otherwise true; alternatively, if you prefer
null-safety, modify the implementation to check for null or empty
(Objects.isNull or == null plus isEmpty()) and document that accordingly—refer
to the method in Menu that takes firstName and lastName.
In `@src/main/resources/import.sql`:
- Around line 1-2: The seed uses REPLACE INTO for Hotel (name) which won't
deduplicate because Hotel.name has no UNIQUE/PK and Room.roomNumber's `@NaturalId`
doesn't create a DB constraint; change the seeding to be idempotent by either
supplying explicit IDs in the INSERT/REPLACE statements for Hotel and Room, or
add proper UNIQUE constraints on Hotel.name and Room.roomNumber and switch to
INSERT ... ON DUPLICATE KEY UPDATE; update the SQL seed to use one of those
approaches and ensure symbols mentioned (Hotel, name, Room, roomNumber,
`@NaturalId`) are updated consistently.
🧹 Nitpick comments (5)
src/main/java/org/example/BookingService.java (1)
43-49: Minor typo in JavaDoc.The documentation accurately describes the method's purpose. However, there's a small grammatical error: "Methods takes" should be "Method takes".
📝 Suggested fix
/** - * Methods takes params and checks them for when input is invalid for booking logic + * Method takes params and checks them for when input is invalid for booking logic * `@param` numberOfGuests Number of guests in the booking * `@param` startDate Starting date for the booking * `@param` endDate Ending date for the booking * `@return` Returns true when none of the params are breaking booking logic, otherwise returns false. */src/main/java/org/example/Menu.java (2)
73-76: Minor typo in JavaDoc."and appropriate message" should be "an appropriate message".
📝 Suggested fix
/** * Displays all the bookings in the system. - * If no bookings exist, displays and appropriate message. + * If no bookings exist, displays an appropriate message. */
97-101: Minor typo in JavaDoc."guests email" should be "guest's email" (possessive).
📝 Suggested fix
/** * Displays all bookings associated with a specific guest. - * Prompts for the guests email address and validates that the guest exists + * Prompts for the guest's email address and validates that the guest exists * before displaying their bookings. */src/main/java/org/example/BookingRepository.java (2)
69-73: JavaDoc references "room" but should reference "booking".The description says "bookingId for the room you want" which is confusing. It should clarify this fetches guests associated with a booking.
📝 Suggested fix
/** - * Method to fetch guest info based on bookingId. - * `@param` bookingId bookingId for the room you want. - * `@return` Returns a List of Guest Objects for the chosen booking + * Method to fetch guest info based on bookingId. + * `@param` bookingId The ID of the booking to fetch guests for. + * `@return` Returns a List of Guest Objects for the specified booking. */
170-182:remove()always returns true even if no booking was deleted.The method returns
trueregardless of whether any rows were actually deleted. Consider returning whether the deletion affected any rows, or throwing an exception if the booking doesn't exist.📝 Suggested improvement
public boolean remove(String bookingId){ return emf.callInTransaction(em -> { em.createNativeQuery("delete gb.* from guestBooking gb where booking_id = ?") .setParameter(1, bookingId) .executeUpdate(); - em.createNativeQuery("delete b.* from Booking b where id = ?") + int deleted = em.createNativeQuery("delete b.* from Booking b where id = ?") .setParameter(1, bookingId) .executeUpdate(); - return true; + return deleted > 0; }); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/main/java/org/example/App.javasrc/main/java/org/example/Booking.javasrc/main/java/org/example/BookingRepository.javasrc/main/java/org/example/BookingService.javasrc/main/java/org/example/Hotel.javasrc/main/java/org/example/Menu.javasrc/main/resources/import.sql
💤 Files with no reviewable changes (2)
- src/main/java/org/example/Booking.java
- src/main/java/org/example/Hotel.java
🔇 Additional comments (7)
src/main/java/org/example/App.java (1)
7-21: LGTM! JavaDoc and SQL logging configuration.The class-level documentation and disabling of Hibernate SQL logging are appropriate for a cleaner runtime experience. Minor typo: "to Hotel CLI" could be "the Hotel CLI".
src/main/java/org/example/Menu.java (4)
11-19: LGTM! Class-level documentation.Clear and comprehensive documentation describing the Menu's capabilities.
35-38: LGTM! Menu loop documentation and implementation.The start method documentation accurately describes the behavior, and the switch-based menu implementation is clean.
Also applies to: 39-70
163-170: LGTM! Comprehensive booking creation flow.The createBooking method has good documentation and appropriate validation. The flow properly handles date parsing errors, room availability checks, and guest creation.
Also applies to: 171-242
245-249: LGTM! Booking removal and validation helpers.The removeBooking method and isInvalidBookingId helper are well-documented and properly validate user input before attempting database operations.
Also applies to: 250-263, 265-270, 271-286
src/main/java/org/example/BookingRepository.java (2)
20-26: LGTM! Room availability query.The getEmptyRooms method is well-documented, and the JPQL query correctly checks for rooms with sufficient capacity that don't overlap with existing bookings.
Also applies to: 27-47
49-52: LGTM! Query methods and documentation.The getBookings, getBookingInfoByGuest, and getMaxGuests methods are well-implemented with appropriate documentation.
Also applies to: 53-67, 101-106, 107-129, 184-187, 188-198
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| replace into Hotel (name) | ||
| values ('Hotel California'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full import.sql file
cat -n src/main/resources/import.sqlRepository: ithsjava25/project-jpa-grupp-6-g
Length of output: 667
🏁 Script executed:
# Search for Hotel and Room entity definitions
find . -type f -name "*.java" | head -20 | xargs grep -l "class Hotel"Repository: ithsjava25/project-jpa-grupp-6-g
Length of output: 114
🏁 Script executed:
# Look for schema or DDL files
find . -type f \( -name "schema.sql" -o -name "*schema*" -o -name "*.ddl" \) | head -10Repository: ithsjava25/project-jpa-grupp-6-g
Length of output: 58
🏁 Script executed:
# Check for JPA/Hibernate annotations on entities
find . -type f -name "Hotel.java" -o -name "Room.java" | xargs cat -n 2>/dev/nullRepository: ithsjava25/project-jpa-grupp-6-g
Length of output: 2995
Fix REPLACE INTO statements to enable idempotent seeding.
REPLACE INTO only replaces existing rows if they match a PRIMARY KEY or UNIQUE constraint. The Hotel table has no unique constraint on name, and the Room table's @NaturalId annotation on roomNumber does not create a database constraint. Without explicit id values or proper unique constraints, these statements will always insert new rows—running the script multiple times creates duplicate data. Use explicit IDs for both statements, or convert to INSERT ... ON DUPLICATE KEY UPDATE with appropriate unique constraints.
🤖 Prompt for AI Agents
In `@src/main/resources/import.sql` around lines 1 - 2, The seed uses REPLACE INTO
for Hotel (name) which won't deduplicate because Hotel.name has no UNIQUE/PK and
Room.roomNumber's `@NaturalId` doesn't create a DB constraint; change the seeding
to be idempotent by either supplying explicit IDs in the INSERT/REPLACE
statements for Hotel and Room, or add proper UNIQUE constraints on Hotel.name
and Room.roomNumber and switch to INSERT ... ON DUPLICATE KEY UPDATE; update the
SQL seed to use one of those approaches and ensure symbols mentioned (Hotel,
name, Room, roomNumber, `@NaturalId`) are updated consistently.
Co-authored-by: Elias Lennheimer Lennheimer.elias@gmail.com Co-authored-by: Martin Karlsson Martin.Karlsson@iths.se Co-authored-by: Rickard Ankar rickard.ankar@iths.se
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/example/BookingRepository.java (2)
140-163: PotentialNoSuchElementExceptionand unsafe ID retrieval pattern.Two concerns:
Line 142:
getFirst()throwsNoSuchElementExceptionif no rooms are available. The caller validates availability, but a race condition could cause this to fail.Lines 152-153: Using
max(b.id)to get the newly created booking's ID is unsafe in concurrent environments. Afterem.persist(booking), the entity should have its ID assigned (if using@GeneratedValue), so usebooking.getId()instead.🔧 Suggested fix
public boolean create(List<String> emailList, LocalDate startDate, LocalDate endDate, int numberOfGuests, BigDecimal totalPrice){ return emf.callInTransaction(em -> { - Room room = getEmptyRooms(startDate, endDate, numberOfGuests).getFirst(); + List<Room> emptyRooms = getEmptyRooms(startDate, endDate, numberOfGuests); + if (emptyRooms.isEmpty()) { + return false; + } + Room room = emptyRooms.getFirst(); Booking booking = new Booking(); booking.setStartDate(startDate); booking.setEndDate(endDate); booking.setTotalPrice(totalPrice); booking.setBookedRoom(room); em.persist(booking); + em.flush(); // Ensure ID is generated - Query query = em.createQuery("select max(b.id) from Booking b"); - Long bookingId = (Long) query.getSingleResult(); + Long bookingId = booking.getId(); for (String email : emailList)
165-182: Return value doesn't reflect actual deletion success.The method always returns
trueregardless of whether any rows were actually deleted. Consider checkingexecuteUpdate()return values to determine if the booking existed.Additionally, the
delete gb.* fromsyntax is MySQL-specific and may not work with other databases.💡 Suggested improvement
public boolean remove(String bookingId){ return emf.callInTransaction(em -> { - em.createNativeQuery("delete gb.* from guestBooking gb where booking_id = ?") + em.createNativeQuery("delete from guestBooking where booking_id = ?") .setParameter(1, bookingId) .executeUpdate(); - em.createNativeQuery("delete b.* from Booking b where id = ?") - .setParameter(1, bookingId) - .executeUpdate(); - return true; + int deleted = em.createNativeQuery("delete from Booking where id = ?") + .setParameter(1, bookingId) + .executeUpdate(); + return deleted > 0; }); }
🧹 Nitpick comments (1)
src/main/java/org/example/BookingRepository.java (1)
102-129: Consider defensive null handling for guest lookup.The method assumes the guest exists when calling
guestRepository.get(email).getId()on line 108. While the current caller (Menu.printBookingsByGuest) validates guest existence first, this method could throw an NPE if called from elsewhere without that validation.💡 Optional defensive approach
public List<BookingInfo> getBookingInfoByGuest(String email){ - long guestId = guestRepository.get(email).getId(); + Guest guest = guestRepository.get(email); + if (guest == null) { + return List.of(); + } + long guestId = guest.getId(); return emf.callInTransaction(em -> {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/example/BookingRepository.javasrc/main/java/org/example/Menu.java
🔇 Additional comments (14)
src/main/java/org/example/BookingRepository.java (5)
20-47: LGTM!The Javadoc accurately describes the method's purpose, parameters, and return type. The JPQL query correctly finds rooms with sufficient capacity that have no overlapping bookings.
49-67: LGTM!The Javadoc accurately describes the method. The implementation correctly fetches all bookings and maps them to
BookingInforecords.
69-84: LGTM!The Javadoc correctly describes this private helper method. The native query properly joins guests with bookings.
86-99: LGTM!The Javadoc return type has been corrected to accurately reflect that this method returns
List<GuestInfo>records.
184-198: LGTM!The Javadoc is accurate, and the implementation correctly handles the case where no rooms exist by returning 0.
src/main/java/org/example/Menu.java (9)
11-20: LGTM!The class-level Javadoc clearly documents the purpose and capabilities of the Menu class.
35-70: LGTM!The Javadoc accurately describes the main menu loop functionality.
73-95: LGTM!The Javadoc correctly documents the method behavior, including the empty bookings case.
97-134: LGTM!The Javadoc accurately describes the validation steps and flow for displaying bookings by guest.
136-161: LGTM!The Javadoc correctly documents the booking ID validation and guest display flow.
163-242: LGTM!The Javadoc comprehensively documents the interactive booking creation process. The implementation properly validates room availability before and after price confirmation.
245-263: LGTM!The Javadoc accurately describes the booking cancellation flow, including validation and relationship cleanup.
265-286: LGTM!The Javadoc correctly describes the validation logic. The method name
isInvalidBookingIdwith inverted return semantics is clearly documented.
288-300: LGTM!The Javadoc has been corrected to accurately describe that this method validates non-empty strings, not null checks.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Co-authored-by: Elias Lennheimer Lennheimer.elias@gmail.com Co-authored-by: Martin Karlsson Martin.Karlsson@iths.se Co-authored-by: Rickard Ankar rickard.ankar@iths.se
2 participants
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores
Removals
✏️ Tip: You can customize this high-level summary in your review settings.