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.
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.
I would probably break this complex string of enumerables out across multiple lines.
There was a problem hiding this comment.
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.
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.
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.
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.
Instead of subtracting 1 here and using <=, you can use <.
There was a problem hiding this comment.
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.
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 ArgumentErrors and NameErrors 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 |
| 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.
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.
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