-
Notifications
You must be signed in to change notification settings - Fork 40
Space - Jessica Stone #26
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?
Conversation
…ms for reserve_block
…g reservation available:false
HotelSection 1: Major Learning Goals
Section 2: Code Review and Testing Requirements
Section 3: Feature Requirements
Overall Feedback
Additional FeedbackJessica! Great work on this project! Hotel is a huge project, and you did an incredible job of going from essentially nothing to a very fully implemented set work of work. I can really clearly see your very clean and direct code style that really prioritizes readability. I thought everything flowed and made sense very clearly; I think that your tests are so incredibly well-written! I think that you are thoughtful about the logic you write, and you clearly refactor; your use of helper methods, enumerable methods, post-fix conditionals, optional keywords, all balanced with pretty direct logic reads so well. I have a few comments with ideas for better syntax (namely the Overall, you did a great job with this project. Well done! Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
| return @rooms | ||
| end | ||
|
|
||
| def reserve_room(date_range, room = nil) |
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 case of the optional parameter! Makes total sense
| reservation = Reservation.new(date_range, @total_reservations, picked_room.number, picked_room.cost) | ||
|
|
||
| find_room(reservation.room_number).reservations << 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.
A very small design thing to consider in the future:
Since your HotelManager class manages number of total reservations, it makes sense that @total_reservations += 1 logic lives here. But since Room manages reservations, too, it might be interesting to see the above lines take more of an approach to:
- Find the right room in
HotelManager, THEN - Tell the right room to create and add a reservation.
I would personally achieve this by doing the find_room method first, and then creating an instance method in Room to create the Reservation object instance and shovel it into its reservations array. This is a thought for a future refactoring!
| let(:rooms) { | ||
| room1 = Hotel::Room.new(1, 200) | ||
| room2 = Hotel::Room.new(2, 200) | ||
| room3 = Hotel::Room.new(3, 200) | ||
| rooms = [room1, room2, room3] | ||
| } |
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 let syntax is super super super weird. It wants to assign the local variable to the last line evaluated in the let block.
This is all to say, in this block that defines a variable named rooms (determined by the line let(:rooms)), you technically don't need the rooms = part on line 16. This will help get rid of a warning you have when you run rake that says .../hotel/test/hotel_manager_test.rb:16: warning: assigned but unused variable - rooms
When you run rake, if you see a bunch of these warnings about assigned but unused variables, and it's about a variable in a let block, try deleting the var_name = part on the last line. Ruby is trying to say that that's redundant with the let block syntax.
| (3..20).each do |i| | ||
| @hotel_manager.reserve_room( @date_range, i) | ||
| 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.
Nice! Perfect approach to this test!
Assignment Submission: Hotel
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection