-
Notifications
You must be signed in to change notification settings - Fork 40
Space - Katie #22
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?
Space - Katie #22
Conversation
… return a list of actual reservations
…ange instance variable. Also made reservations_for_room an instance variable
…oom method if a room is not available
… not an instance of Hotel::Room, and removed the unused and commented out date range method which now lives in the intiliaze method
…ions specifically
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 that you did a great job breaking down how each class in your design could track and update data! There aren't too many dependencies between classes, and nothing is too tightly coupled! I also think you were very methodical in your tests! I do see some room for improvement around reducing down complicated operations. This is admittedly tricky to do on a deadline, but whenever possible, avoiding using loops where they aren't necessary and avoiding having two variables with mutually dependent data will make your code faster, and make it harder to make mistakes! Your code produces the following warning: Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
| @@ -0,0 +1,5 @@ | |||
| { | |||
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.
Looks like you forgot to .gitignore the coverage folder!
| @available_rooms.delete(room) | ||
| @reserved_rooms.push(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.
This is pretty good, but did you consider using the rooms as keys and a bool as a value in a hash? That would reduce your data overhead a bit, and means that you don't run the risk of forgetting to update one of the two arrays! :)
| self.nights.times do |day| | ||
| range.nights.times do |day_in_range| | ||
| return true if (range.start_date + day_in_range) == (@start_date + day) | ||
| end | ||
| end | ||
| return false |
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.
There is a simpler version of this logic that doesn't require looping! Draw a timeline, and put a date range on it, then draw out all possible relationships the ranges can have with it. I often take the approach of making a table or drawing a sketch when I want to simplify boolean logic.
| end | ||
| end | ||
|
|
||
| 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.
Love how thorough this is!
| require_relative 'room.rb' | ||
|
|
||
| module Hotel | ||
| class HotelController |
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 class is REALLY well scoped! It's letting the classes that its composed of do the heavy lifting!
Assignment Submission: Hotel
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection