-
Notifications
You must be signed in to change notification settings - Fork 46
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
Lindsay - Edges - hotel #44
base: master
Are you sure you want to change the base?
Conversation
… all reservations
…hat the dates are the correct class
…he reservation class to the reservation spec class
HotelWhat We're Looking For
Good job overall! Your design for how to keep track of reservations is a little odd but it gets the job done, and experimenting with design is one of the goals for this project. It is important to me that you understand why I find it odd, and that you're comfortable dividing up responsibilities between classes - as is, everything landed in Outside of design, I like what I see. Your method code is solid, your test coverage is pretty close to where we want it to be, and it's clear to me that the learning goals for this assignment were met. Be sure to review my inline comments, and keep up the hard work! |
|
||
@reservation_dates = reservation_dates | ||
@room_id = room_id | ||
@total_cost = reservation_cost(reservation_dates) |
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.
Rather than calculating the total cost up front and storing it in an instance variable, I would leave this as a behavior, a method is called whenever someone wants that information. That way if the number of nights or the cost changes, the total cost will automatically be correct - it will "just work".
def initialize | ||
@reservations = [] | ||
@room_bookings = {1 => [], 2 => [], 3 => [], 4 => [], 5 => [], 6 => [], 7 => [], 8 => [], 9 => [], 10 => [], 11 => [], 12 => [], 13 => [], 14 => [], 15 => [], 16 => [], 17 => [], 18 => [], 19 => [], 20 => []} | ||
@room_blocks = [] |
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 would probably build the room booking list in a loop rather than hardcoding it.
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 want to call out the way you keep track of which rooms are reserved on which dates. This strategy definitely works! However, it ends up duplicating information between this file and Reservation
. If you needed to, for example, change the dates on a reservation, or delete a reservation, you would also have to remember to do work on the @room_bookings
list here. This is a dependency, albeit a subtle one, and it will make your code more difficult to change.
Another option is to have the Reservation
s be the single source of truth regarding which rooms are reserved when. This file would have to loop through the reservations to find ones that overlap with the requested dates.
lib/reservation_hub.rb
Outdated
def add_reservation(start_date, end_date) | ||
|
||
@start_date = start_date | ||
@end_date = end_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.
You've done a great job of using vertical whitespace (empty lines) to break up the code in this file, which makes it very easy to read.
lib/reservation_hub.rb
Outdated
@total_rooms = total_rooms | ||
validate_dates | ||
|
||
reservation_dates = create_date_array(start_date, end_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.
There are a couple of behaviors here that have to do with working with dates (validate_dates
, create_date_array
), which make me think that a separate class to encapsulate this behavior might be appropriate. You could even include the logic to check for an overlap.
@reservations.each do | ||
|
||
if @reservations[index].reservation_dates.include?(date) | ||
reservations_by_date << @reservations[index] |
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 keeping a manual index here, you should use a block parameter:
@reservations.each do |reservation|
if reservation.reservation_dates.include?(date)
reservations_by_date << reservation
end
end
Or better yet, notice that we're using the select
pattern:
def find_reservations(date)
return @reservations.select do |reservation|
reservation.reservation_dates.include?(date)
end
end
|
||
def initialize(reservation_dates, room_id) | ||
|
||
@reservation_dates = reservation_dates |
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.
The name reservation_dates
is a little redundant, since we're already in the Reservation
class. Something like dates
or date_range
might be a better name.
it "raises an error if the hotel is fully booked" do | ||
|
||
start_date = Date.new(2018,01,05) | ||
end_date = Date.new(2018,01,06) |
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.
Nice, good work catching this edge case!
it "won't duplicate room ids if two reservations share the same dates" do | ||
|
||
reservation_dates = [1,2,3] | ||
room_id = @reservation_hub.assign_room(reservation_dates) |
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 test cases around whether two ranges of dates overlap that you're missing here. See the slack message posted in #general.
Hotel
Congratulations! You're submitting your assignment!
Comprehension Questions