-
Notifications
You must be signed in to change notification settings - Fork 299
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
Time - Haben #38
base: master
Are you sure you want to change the base?
Time - Haben #38
Conversation
Design Scaffolding for the Hotel project
HotelSection 1: Major Learning Goals
Section 2: Code Review and Testing Requirements
Section 3: Feature Requirements
Overall Feedback
Additional FeedbackGreat work overall! You've built your first project with minimal starting code. This represents an incredible milestone in your journey, and you should be proud of yourself! I am particularly impressed by the way you implemented tricky logic using a solid object oriented design. You have written a number of tests that are clear are clear and thorough. I've left a few inline comments suggesting ways you might refactor your code and places where you should test edge cases. Please follow up if you have any questions. Keep up the hard work! Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
available_rooms = [] | ||
# iterate through the rooms to find available room | ||
@rooms.each do |room| | ||
reservations = list_reservations_for_room(room, 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.
Interesting design choice to use list_reservations_for_room
here. This works! However, you don't necessarily need to know all the reservations for that room, just the ones that overlap. Consider if there's a way you could refactor this to make it more efficient.
@end_date = end_date | ||
end | ||
# check if date selected overlaps with the date_range | ||
def overlap?(new_start_date, new_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.
Nice use of a boolean expression to return true
or false
. You don't even need a conditional :)
|
||
############################################# | ||
|
||
describe "overlap?" do |
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 fleshing out these tests!
end | ||
end | ||
describe "list_reservations_for_room" do | ||
it "returns a list of reservations that fall with in the given date range for a specific room." do |
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 should also have an edge case test for the situation where there are no reservations for a given room.
expect(reservation).must_be_kind_of Hotel::Reservation | ||
end | ||
|
||
it "return false if there are no availalbe rooms" do |
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.
Minor note, perhaps rephrase as it "raises an exception if there are no available rooms" do
end | ||
|
||
# assert | ||
expect{(@hotel_controller.reserve_room(start_date, end_date))}.must_raise ArgumentError |
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.
Consider using a custom exception.
|
||
end | ||
|
||
describe "show_reservations_for_date" do |
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.
Consider the edge case where there are no reservations for a particular date.
|
||
describe "wave 2" do | ||
describe "find_available_rooms" do | ||
it "takes two dates and returns a list" do |
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 need a few more test cases here. For instance, Make sure that it finds the correct available rooms. What if there are no available rooms?
Assignment Submission: Hotel
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection
HotelController
. Also trying to decide whetherBlock
class to inherit fromHotelController
class.HotelController
class.overlap
test, it will return false if the new date range is completely after the date range. This test falls into the expected usage for the DateRange class.