-
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
Hayden (Edges) - Hotel #42
base: master
Are you sure you want to change the base?
Conversation
…ervations, and implement RoomBooker#reservation_cost plus helper method RoomBooker#find_reservation
…vailable_rooms method
…reserve_available_room, and refactor RoomBooker#list_available_rooms and RoomBooker#list_reservations, move RoomBooker#reservation_cost to Reservation class
…remove obsolete methods, refactor RoomBooker#list_available_rooms and RoomBooker#new_reservation, implement all RoomBlock related methods, RoomBooker#set_room_rate, and RoomBooker#room_unavailable? error handling
HotelWhat We're Looking For
Hayden! Wonderful job on this. I also want to highlight that your tests, by the way they're structured, named, and written, are thorough and so clear, while at the same time being very very good/fun when you needed test data/names. I felt like it was really easy to get to know your code through its tests. Well done! I'm adding a few comments directly into your code. The themes of these comments are mostly just suggestions for minor refactors. Otherwise overall, your code is clean and hit the requirements in a very elegant way. Nice work! |
@@ -0,0 +1,33 @@ | |||
module DateLogic |
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 is one of the most appropriate ways to use a module and I'm very very very happy with this.
return true | ||
else | ||
return false | ||
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.
Would it be as readable/work as well if we one-lined this to:
return reservation.check_in...reservation.check_out).cover?(date)
module BookingLogic | ||
class Room | ||
end | ||
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.
Don't forget to remove unused files before you submit your project/as you refactor them out!
if DateLogic.date_range_include?(reservation, date) | ||
list_of_reservations << reservation | ||
end | ||
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 might be a good opportunity to use select
maybe?
|
||
unavailable_rooms.each do |unavailable_room| | ||
available_rooms.delete_if { |room| room.id == unavailable_room.id } | ||
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 block of code is wonderful ✨ I love it!
I kept looking at this block of code and thinking of a way to improve it. I thought of a couple ways that could potentially be less readable, like subtracting the two arrays (which I'm unsure works with structs), or using a destructive reject!
or select!
. Just a thought!
room_unavailable?(room, check_in, check_out) | ||
new_reservation = BookingLogic::Reservation.new(room, check_in, check_out) | ||
reservations << new_reservation | ||
return new_reservation |
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 just want to call out that we never formally teach this pattern, but this is a great and common pattern: when a method makes something new, it returns that new thing in the end. Nice!
return new_reservation | ||
end | ||
|
||
def set_room_rate(room_id, custom_rate) |
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 think this method is unused? I think you probably wrote this and then realized you should move it to the RoomBlock class, which you did... but you left this code here! You can probably get rid of this if that's the case
…oom to more loosely couple the classes
Hotel
Congratulations! You're submitting your assignment!
Comprehension Questions