-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sockets-Chantal #29
base: master
Are you sure you want to change the base?
Sockets-Chantal #29
Conversation
HotelWhat We're Looking For
|
require_relative 'reservation' | ||
|
||
class Hotel | ||
NUM_ROOMS = 20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
end
I 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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