Conversation
HotelWhat We're Looking For
|
| require_relative 'reservation' | ||
|
|
||
| class Hotel | ||
| NUM_ROOMS = 20 |
There was a problem hiding this comment.
Should this class be in a module?
| # Find room that's available. | ||
| available_rooms = find_available_rooms(start_date, end_date) | ||
| raise RuntimeError.new("there are no rooms available") if available_rooms.empty? | ||
| room_number = available_rooms.to_a.sample |
There was a problem hiding this comment.
Good call raising an error here - that seems like an appropriate way to indicate that the method can't continue.
Instead of raising a RuntimeError, best practice is to define your own appropriately named exception type that inherits from StandardError and raise that. The reason is that RuntimeError is very broad, and doesn't give much information about what went wrong. Many things could produce a RuntimeError, but a NoRoomsAvailableError tells the caller exactly what went wrong.
| @reservations.each do |reservation| | ||
| if reservation.start_date < end_date && reservation.end_date > start_date | ||
| available_rooms.delete(reservation.room_number) | ||
| end |
There was a problem hiding this comment.
Here, Hotel reaches into the Reservation and looks at its data directly. This is both an example of tight coupling, and violation of the law of demeter (the two tend to go hand in hand).
A cleaner approach might be to define a method Reservation#overlap?(start_date, end_date) that contains the logic on line 36, and call that here:
@reservations.each do |reservation|
if reservation.overlap?(start_date, end_date)
available_rooms.delete(reservation.room_number)
end
endI know we hadn't read POODR ch 4 yet when you wrote this code, but this is what Metz is talking about when she says you should "ask for what instead of telling how".
| let(:booked_hotel) do | ||
| booked_hotel = Hotel.new | ||
| (NUM_BOOKED_ROOMS).times do | ||
| booked_hotel.make_reservation(@seventh, @eleventh) |
There was a problem hiding this comment.
I really like this organization - having all of an empty, a mostly-booked and a fully-booked hotel available for your tests up front makes the process of testing much simpler.
| def calculate_total_cost() | ||
| total_nights = @end_date - @start_date #probably convert to integer after test | ||
| total_nights = total_nights.to_i / (24 * 60 * 60) | ||
| ROOM_PRICE * total_nights |
There was a problem hiding this comment.
I like the decision to make this a method rather than storing the cost as an instance variable. That way if the number of nights or the cost per night were to change (say in a future version of the program), the total cost will automatically be correct.
|
|
||
| it "make reservation for fully booked hotel" do | ||
| expect{fully_booked_hotel.make_reservation(@seventh, @eleventh)}.must_raise RuntimeError | ||
| end |
There was a problem hiding this comment.
There are a number of questions around when specifically a room is available that these tests don't cover. In particular, if a hotel is fully booked for one range of dates and you get another request...
No room is available if the dates:
- Same dates
- Overlaps in the front
- Overlaps in the back
- Completely contained
- Completely containing
The room is available if the dates:
- Completely before
- Completely after
- Ends on the checkin date
- Starts on the checkout date
Hotel
Congratulations! You're submitting your assignment!
Comprehension Questions