-
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 - Sarah Read-Brown - Hotel #47
base: master
Are you sure you want to change the base?
Conversation
…d to be called later
…or edge case anything that isn't a date object
…test specs so they all pass again
…ng to reserve more then 5 rooms
HotelWhat We're Looking For
Good job overall. There are a couple places where your design could be improved, particularly around the interaction between |
|
||
def valid_date? | ||
valid_input? | ||
if @check_in > @check_out |
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.
Nitpick: I like that these validations are called from your constructor, but I think the code would be a little more readable if they were invoked directly, rather than via make_nights_arr
.
def update_room_state(rooms_to_book,num_rooms,state) | ||
num_rooms.times do |i| | ||
rooms_to_book[i].booked = state | ||
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.
You could use an each
loop here with the same effect.
lib/reservations.rb
Outdated
range = DateRange.new(check_in,check_out).nights_arr | ||
range.each do |date| | ||
@all_reservations.each do |booking| | ||
if booking.date_range.nights_arr.include?(date) |
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.
Instead of looping through every date in the range, it might be a little cleaner to write a method DateRange#overlap?(other)
. Then this loop could be simplified to:
booked_rooms = []
range = DateRange.new(check_in,check_out)
@all_reservations.each do |booking|
if booking.date_range.overlap?(range)
# add rooms to booked_rooms
end
end
return booked_rooms
It's doing the same work, but I would argue this version is more readable, and does a better job of separating responsibilities. It also makes testing easier!
@block_info = block_info | ||
end | ||
|
||
def rooms_available_block |
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.
Because this method is already inside the Block
class, you shouldn't need to add _block
to the name.
@rooms.each do |room| | ||
if room.booked == false || room.booked == nil | ||
rooms_available << room | ||
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.
This method for keeping track of which rooms are reserved is an interesting idea, but I believe you will run into trouble if you have multiple blocks for different dates containing the same rooms. Because the Room
instances are shared between blocks, marking a room reserved for one block will make it reserved for all blocks.
end | ||
|
||
def reserve_from_block(block, num_rooms) | ||
rooms_to_book = block.rooms_available_block |
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 that you call a method on block
here rather than manipulating its state directly. Good example of loose coupling.
arr_booked.length.must_equal arr_booked_unique.length | ||
end | ||
it "returns an empty array if nothing is booked" do | ||
@hotel.check_reservations(@check_in + 1,@check_out + + 1).must_be_empty |
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 for this method.
@hotel.make_reservation(@check_in + 2, @check_out + 2,20) | ||
end | ||
describe "make a block " do | ||
it "will create a block, " do |
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 a nested describe
to organize things.
it "returns an empty array if everything is booked" do | ||
@hotel.make_reservation(@check_in,@check_out,18) | ||
@hotel.check_availability(@check_in,@check_out).must_be_empty | ||
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 other cases I'd be interested in exploring around when a room is considered available. My full list includes:
- 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
…ed some of the code in check_reservations class
Hotel
Congratulations! You're submitting your assignment!
Comprehension Questions