-
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
Michelle - Edges - Hotel #31
base: master
Are you sure you want to change the base?
Conversation
…o handle reservations
…so added a method to find reservations that match a certain date
…pt hash-like input + began similar input-related work on Reservation#initialize
…m and #create_reservation
…rved. Also added error handling for invalid end date
…ember now :( prob Reservation or BookingSystem
…lving dates plus commented questions/todos
…alization to kwargs
HotelWhat We're Looking For
|
def list_res_for_date(check_date) | ||
matching_res = @reservations.select { |reservation| reservation.has_date?(check_date) } | ||
|
||
return matching_res.empty? ? nil : matching_res |
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 enumerable here.
For the return value, I think that an empty array fits better than nil
here. An empty array is typically used to indicate "I found no matches" when you're returning a collection (it's a collection with no members). nil
is used when you're supposed to return one instance.
def list_avail_rooms_for_range(check_in:, check_out:) | ||
date_range = construct_cal_checker(check_in: check_in, check_out: check_out) | ||
|
||
booked_rooms = @reservations.select { |reservation| reservation.overlap?(date_range) }.map { |reservation| reservation.room_num } |
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 break this complex string of enumerables out across multiple lines.
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 has a couple of excellent examples of where your design allowed you to have loose coupling between classes. For example, when you loop through the list of reservations or blocks, instead of BookingSystem
knowing about the details of how a Reservation
stores dates, it calls a method and lets Reservation
take care of the details.
|
||
if !held_rooms.empty? | ||
held_rooms = held_rooms[0] # list within a list | ||
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 doesn't quite do what you want. If you have two overlapping blocks, then you'll end up with an array like this: [[1, 2, 3], [7, 8, 9]]
, but your code will only grab the first sub-array.
The array method .flatten
does what you're looking for.
def create_reservation(check_in:, check_out:) | ||
id = generate_res_id() #<-- create new reservation id | ||
avail_rooms = list_avail_rooms_for_range(check_in: check_in, check_out: check_out) #<-- grab first available 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.
I like that you've broken out finding the list of available rooms as a separate helper method. That makes reading this method much easier.
class Calendar | ||
attr_reader :check_in, :check_out | ||
|
||
def initialize(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.
I like that you've made this its own class, but I'm not sure I agree with the name Calendar
. Calendar implies that it's tracking a list of events, when this just tracks a start and end date. Something like DateRange
or CalendarBooking
might be a better name.
def overlap?(other_dates) # param: Calendar obj | ||
if !(other_dates.check_in <= @check_out-1) || !(other_dates.check_out-1 >= @check_in) | ||
return false | ||
else |
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 subtracting 1 here and using <=
, you can use <
.
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 the expression on line 31 will always produce true or false, you don't need an if statement. You could write: return !(other_dates.check_in <= @check_out-1) || !(other_dates.check_out-1 >= @check_in)
|
||
unless @check_in < @check_out | ||
raise StandardError, "Invalid date range: end date must occur after start date." | ||
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.
Instead of raising a StandardError
, best practice is to define your own appropriately named exception type that inherits from StandardError
and raise that. The reason is that StandardError
is very broad, and if the code that calls this method has to rescue StandardError
, that will catch things like ArgumentError
s and NameError
s that indicate true bugs.
it "returns nil if no rooms are available" do | ||
20.times do |i| | ||
res = Hotel::Reservation.new(id:1, room_num: i+1, check_in: "1999-1-1", check_out: "1999-12-31") | ||
booking_system.reservations << res |
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, I like this edge case.
describe "#list_res_for_date" do | ||
# see Calendar#has_date? for additional tests | ||
let(:res_1) {Hotel::Reservation.new( | ||
id: "1", |
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.
What about reservations that came through a block?
describe "#overlap?" do | ||
it "returns false for no overlap: other dates begin and end before reserved dates" do | ||
other_dates = Hotel::Calendar.new(check_in: "1986-07-01", check_out: "1986-07-15") | ||
|
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 work implementing these test cases. The list may have come from us originally, but you are the one who did the work of turning them into real tests, and that diligence is appreciated.
…tion so it can interact with this method
Hotel
Congratulations! You're submitting your assignment!
Comprehension Questions