Conversation
…spec_helper. Created initialize method for Room class
…ethods to be created.
…ly be used to determine which room a guest will be assigned to when a resewrvation is made
…ossible reservation date combinations
…l as accompamying specs. Created block class and tests
… Hotel with tests
HotelWhat We're Looking For
|
| def see_reservations_by_date(date) | ||
| this_dates_reservations = [] | ||
| @all_reservations.each do |reservation| | ||
| this_dates_reservations << reservation if reservation.start_date == date |
There was a problem hiding this comment.
The .select enumerable might be helpful to clean up this code.
| def see_available_rooms_by_date(possible_start_date, possible_end_date) | ||
| these_dates_available_rooms = [] | ||
| @all_rooms.each do |room| | ||
| these_dates_available_rooms << room if room.date_available?(possible_start_date, possible_end_date) |
There was a problem hiding this comment.
On line 34, I like that you call room.date_available? instead of having a bunch of date math here in Hotel. This is what Metz is talking about when she says you should "ask for what instead of telling how".
| def room_available?(new_start_date, new_end_date) | ||
| @all_rooms.each do |room| | ||
| return room if room.date_available?(new_start_date, new_end_date) | ||
| end |
There was a problem hiding this comment.
In Ruby, ending a method name with a question mark implies that it returns either true or false, but this method may return an instance of Room. A better name might be find_available_room.
| def hold_block(start_date, end_date, room_collection) | ||
| room_collection.each do |room| | ||
| raise ArgumentError, "Room #{room.number} is already in a block" if room.block_id != nil | ||
| end |
There was a problem hiding this comment.
I don't know that I agree with the idea of storing the block_id as an instance variable on the Room. That means that each room can only ever be included in one block. However, blocks don't last for ever - they're for a set amount of time, and on other dates the room should be free to be included in other blocks.
| if desired_block == nil | ||
| raise ArgumentError, "The block entered does not exist or it does not have any rooms available for reseervation" | ||
| elsif desired_block.room_still_available? | ||
| reservation = HotelSystem::Reservation.new(room: room, start_date: start_date, end_date: end_date, guest: guest) |
There was a problem hiding this comment.
This will not raise an exception if the block is full. To do so, you would need code like this:
if desired_block == nil || desired_block.room_still_available?
raise ArgumentError #...
else
# ...However, I would prefer you split the two cases and provide a little more detailed information in your error message.
|
|
||
| def calculate_cost | ||
| return ("%.2f" % ((@end_date - @start_date) * @price)).to_f | ||
| end |
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.
| if possible_start_date == reservation.start_date | ||
| return false | ||
| elsif reservation.start_date < possible_start_date && possible_start_date < reservation.end_date | ||
| return false |
There was a problem hiding this comment.
You can simplify all of this complex conditional logic into one line! See the instructor implementation.
Each of these would make an excellent test case though.
| hotel.reserve_room( | ||
| room, | ||
| Date.new(2019, 3, 10), | ||
| Date.new(2019, 3, 13), |
There was a problem hiding this comment.
You do a lot of repeated setup work throughout these tests. Would it be possible to consolidate this into a shared before block or something similar?
| describe "room_available? method" do | ||
| before do | ||
| @hotel = HotelSystem::Hotel.new | ||
| @hotel.all_rooms.slice!(2..19) |
There was a problem hiding this comment.
This is a clever way to make this test work. Another option would be to pass in the number of rooms to the Hotel constructor, possibly with a default value of 20.
| it "returns false if the possible start date matches the start date of the reservation" do | ||
| expect(@room.date_available?(Date.new(2019, 3, 11), Date.new(2019, 3, 12))).must_equal false | ||
| expect(@room.date_available?(Date.new(2019, 3, 11), Date.new(2019, 3, 14))).must_equal false | ||
| expect(@room.date_available?(Date.new(2019, 3, 11), Date.new(2019, 3, 15))).must_equal false |
There was a problem hiding this comment.
Good work nailing all of these test cases.
Hotel
Congratulations! You're submitting your assignment!
Comprehension Questions