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.
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.
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.
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_roomsIt'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.
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.
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.
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 |
| @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.
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.
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