Pipes - Iuliia - Hotel #21
Conversation
…ded new parameter - room
…uring reservation
… passed all of trem
…s max; added tests and passed them
…fuctored some tests to pass last changes
…changed some tests to pass them
…ctored all the tests and passed them all
HotelWhat We're Looking For
Good job overall. Your high-level design is deliberate and well-thought-out, though there are still a couple places where the |
|
|
||
| def dates_within_range | ||
| dates = (@check_in .. @check_out - 1).map { |date| date } | ||
| return dates |
There was a problem hiding this comment.
Here you're mapping the list of dates to itself. You could instead say:
return (@check_in..@check_out-1).to_a| def list_of_available_rooms(date_range) | ||
| available_rooms = {} | ||
| @rooms.each do |room, price| | ||
| booked_dates = room_unavailable(room) #array of dates |
There was a problem hiding this comment.
This certainly solves the problem, but the logic is pretty complex, especially when you factor in the call to room_unavailable. Your strategy seems to be:
available_rooms starts empty
for each room
unavailable_dates = []
for each reservation or block
if room matches
add dates to list of unavailable_dates
if unavailable_dates does not include desired dates
add room to available_rooms
The general strategy is to start with an empty list and add rooms to it one by one, and the outer loop is through the set of rooms.
An alternative approach that I think produces cleaner code is to start with a list of all the rooms, and have the outer loop go through reservations and blocks. When you find one that matches the desired date, you remove the room(s) from the list. In pseudocode:
available_rooms = list of all rooms
for each reservation or block rb
if rb.dates overlaps the desired dates
remove rb.rooms from available_rooms
Note that we're able to remove the inner loop entirely.
|
|
||
| def list_of_reservations(date) | ||
| list = @all_reservations.map { |reservation| reservation if reservation.date_range.dates_within_range.include?(date) } | ||
|
|
There was a problem hiding this comment.
The select enumerable might be a better choice than map here.
|
|
||
| if @check_in.class != Date || @check_out.class != Date || @check_in > @check_out || @check_in < Date.today | ||
| raise InvalidDate.new("Invalid date or date range") | ||
| end |
There was a problem hiding this comment.
Good use of a custom exception here
lib/hotel.rb
Outdated
| date_range = block.date_range | ||
| cost = @rooms[room] * (1 - @block_discount / 100.0) | ||
| new_reservation_from_block = Reservation.new(date_range, room, cost) | ||
| block.rooms.delete(room) |
There was a problem hiding this comment.
This is an example of tight coupling between Hotel and Block. You manipulate the block's date_range and list of rooms from the Hotel method. A cleaner approach might be to define a method Block#reserve_room that creates the reservation and removes the room from its internal list. Then if the implementation of Block ever changes, this code will not have to.
| date_range.dates_within_range.each do |date| | ||
| if !booked_dates.include?(date) | ||
| count += 1 | ||
| end |
There was a problem hiding this comment.
Instead of having this loop here, you might write a method DateRange#overlap?(other_date_range). This would both be easier to read and easier to test!
| it "Raise an error if check_in or check_out values are not types of Date class" do | ||
| proc { BookingSystem::DateRange.new("2017-9-15", "2017-9-17") }.must_raise BookingSystem::InvalidDate | ||
| end | ||
| it "Raise an error if given check_out date is before check_in date" do |
| reservation = @two_room_hotel.make_reservation(@date_range, 1) | ||
| second_reservation = @two_room_hotel.make_reservation(@date_range, 2) | ||
| proc { @two_room_hotel.list_of_available_rooms(@date_range) }.must_raise BookingSystem::NoRoomAvailableError | ||
| end |
There was a problem hiding this comment.
Good testing for the failure case here. Other cases I'd be interested in:
- What if there are no reservations? (it should return a list of all the rooms)
- Do reservations that don't overlap the date range affect the list? (they shouldn't)
- Check the boundaries of what counts as an overlap. What about a reservation that ends on the checkin date, or starts on the checkout date?
- What if your date range spans several small reservations? (all of the rooms from those reservations should be omitted from the list)
| end | ||
| if available_rooms.length == 0 | ||
| raise NoRoomAvailableError.new("No room is available on given dates") | ||
| end |
There was a problem hiding this comment.
I'm not sure I agree that an exception is justified here if there are no available rooms. In that case there's a way to tell the caller what happened using the existing channels: return an empty array.
Once the caller has that information, they can raise an error or not as they see fit.
| it "Raise an error if asked to reserve a room that is not available" do | ||
| reservation = @two_room_hotel.make_reservation(@date_range, 1) | ||
| proc { @two_room_hotel.make_reservation(@date_range, 1) }.must_raise BookingSystem::NoRoomAvailableError | ||
| end |
There was a problem hiding this comment.
This is a good start, but I'd like to see more coverage around what date ranges will conflict with each other. Cases I'd be interested in:
- Not available:
- Same dates
- Overlaps in the front
- Overlaps in the back
- Completely contained
- Completely containing
- Available:
- Completely before
- Completely after
- Ends on the checkin date
- Starts on the checkout date
2 participants
Hotel
Congratulations! You're submitting your assignment!
Comprehension Questions