-
Notifications
You must be signed in to change notification settings - Fork 49
Leaves_Ga-Young #33
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
base: master
Are you sure you want to change the base?
Leaves_Ga-Young #33
Conversation
HotelWhat We're Looking ForTest Inspection
Code Review
Overall 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 that you tested all sides of the booking problem! I do see some room for improvement around class delegation. The discussion of POODR chapter 4 should help here. |
lib/reserv_system.rb
Outdated
| @reservations << new_res | ||
| not_reserved_rooms[0].reservation << new_res | ||
| else | ||
| raise ArgumentError.new("Cannot make a reservation for that date range. No rooms available.") |
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 don't think this is a problem with the arguments. This is just an error, or perhaps you should define a custom error.
lib/reserv_system.rb
Outdated
| def make_reservation(start_time, end_time) | ||
| not_reserved_rooms = not_reserved_on_date_range(start_time, end_time) | ||
|
|
||
| if not_reserved_rooms.class == Array |
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 weird way to check this. We generally don't check the class of something in ruby. I'd rather see you always return an array, and check like, the length of the array.
lib/reserv_system.rb
Outdated
| else | ||
| curr_room.reservation.each do |each_res| | ||
| if overlap?(start_time, end_time, | ||
| each_res.start_time, each_res.end_time) == true |
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.
have this bit of logic live in the reservation rather than the system. That way you don't have to reach inside the reservation to get the info.
test/reserv_system_test.rb
Outdated
| end | ||
| end | ||
|
|
||
| describe "determining date 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.
Nice!
|
|
||
| 1. A lot of my tests seem redundant. While I want to make sure every | ||
| possible scenario is covered and that my code is doing what I want it | ||
| to, I may be able to cut some 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.
Your tests are very thorough, and I appreciate it!
Hotel
Congratulations! You're submitting your assignment!
Comprehension Questions