-
Notifications
You must be signed in to change notification settings - Fork 48
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
Sockets - Erica #25
base: master
Are you sure you want to change the base?
Sockets - Erica #25
Conversation
…code to pass test
… wrote code to pass
…e, wrote code to pass, updated Reservation constructor
… by creating make_reservation and list_reservations_by_date methods
…and updated constructor and make_reservation to use new method
…pendencies between room and hotel manager
…ocks, DRY room class
… methods in HotelManager class
HotelWhat We're Looking For
|
def create_hash_of_rooms_with_status(rooms) | ||
hash = Hash[rooms.collect { |room| [room, "AVAILABLE"] }] | ||
return hash | ||
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 is a clever way to think about this problem.
lib/hotel_manager.rb
Outdated
found_reservations = [] | ||
@reservations.each do |reservation| | ||
if date >= reservation.start_date && date < reservation.end_date | ||
found_reservations.push(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.
The .select
enumerable might be helpful to clean up this code.
available_rooms = @rooms | ||
reservations.each do |reservation| | ||
if reservation.start_date >= start_date && reservation.end_date <= end_date | ||
available_rooms.delete(reservation.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.
Here the Hotel
goes in and looks at the details of the Reservation
class (its start date and end date), which is not great design. Hotel
needs to know a lot about Reservation
for that to work, and the two classes are more tightly coupled than they need to be. An alternative approach would be to write a method Reservation#overlaps?(start_date, end_date)
, and call that here instead:
reservations.each do |reservation|
if reservation.overlaps?(start_date, end_date)
available_rooms.delete(reservation.room)
end
end
I know we hadn't read POODR ch 4 yet when you wrote this code, but this is what Metz is talking about when she says you should "ask for what instead of telling how".
def calculate_cost | ||
total_cost = ((@end_date - @start_date) - 1) * @room.cost_per_night | ||
return total_cost | ||
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.
I like that you use a method for this rather than storing it in an instance variable. That way if the number of nights or cost per night changes, the cost will always be correct.
it "doesn't include reservations where the end date is the date" do | ||
reservations = @hotel_manager.list_reservations_by_date("2019-3-20") | ||
|
||
expect(reservations.length).must_equal 0 |
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 say this counts as an edge case - you're testing for the "edge" between two different behaviors this method can exhibit.
Hotel
Congratulations! You're submitting your assignment!
Comprehension Questions