-
Notifications
You must be signed in to change notification settings - Fork 45
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
Pipes - Kate Evans-Spitzer - Hotel #48
base: master
Are you sure you want to change the base?
Conversation
…ncludes_all_dates? which was renamed to Block.includes_dates?
HotelWhat We're Looking For
Great work overall! Your design is well-thought-out and deliberate, your specific method code is well-organized and easy to read, and your test coverage is solid. Keep up the hard work. |
lib/hotel.rb
Outdated
|
||
unless num_rooms.class == Integer | ||
raise(ArgumentError, "Number of rooms parameter must be Integer: #{num_rooms}") | ||
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.
I usually recommend against this sort of argument checking. If the number of rooms isn't an integer, then an error will happen as soon as you get to the times
call on the next line. Moreover, this prevents someone from implementing some other class that has a times
method and using it here. This is called "duck typing", and it's one of the big ideas that make dynamic languages like Ruby nice to use.
|
||
@reservations.each do |reservation| | ||
rooms.delete(reservation.room) if (rooms.include? reservation.room) && reservation.includes_dates?(checkin, checkout) | ||
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.
I really like this method - it is well-implemented and very easy to read.
Minor note: you don't need the (rooms.include? reservation.room)
. Attempting to delete an element from a list in which it doesn't appear will have no effect (not an error).
rooms = @rooms.dup | ||
@blocks.each do |block| | ||
if block.includes_dates?(start_date, end_date) | ||
rooms.delete_if { |room| block.rooms.include? room } |
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 use of an instance method (Block#includes_dates?
) to keep things loosely coupled.
it 'allows one range to end the day the next starts' do | ||
overlap = DateRange.overlap?('2017-09-06', '2017-09-10', '2017-09-10', '2017-09-20') | ||
overlap.must_equal false | ||
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.
I like these test cases! There are many similar cases it might be interesting to explore here around whether or not a room is available. My entire list includes:
- Does Overlap:
- Same dates
- Overlaps in the front
- Overlaps in the back
- Completely contained
- Completely containing
- Does Not Overlap:
- Completely before
- Completely after
- Ends on the checkin date
- Starts on the checkout date
|
||
describe 'includes_dates?' do | ||
it 'returns true if block includes provided dates' do | ||
includes_dates = @block.includes_dates?('2017-08-03', '2017-08-07') |
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.
Since you test all this functionality in the context of DateRange
, you can get away with just a cursory bit of coverage here. This is important for two reasons:
- If the functionality changes enough that the tests need to change, you currently need to change them in two places
- You don't want to burn yourself out writing a bajillion redundant tests.
proc { | ||
@hotel.find_available_rooms('2017-08-08', '2017-08-08') | ||
}.must_raise DatesError | ||
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.
Good coverage here.
Hotel
Congratulations! You're submitting your assignment!
Comprehension Questions