-
Notifications
You must be signed in to change notification settings - Fork 40
Space - Stephanie #31
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
…g with check availablity.
…oom. Changed date_range to is_available_range
…g, create rooms updated and working.
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 setup readable and logical relationships between classes! I do see some room for improvement around variable naming, instance variables, and readers/writers. Please see the inline comments for more details. Please reach out to me if any of my comments don't give enough detail or if you could use more explanation! Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
| attr_writer | ||
| attr_accessor |
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.
Just so you know, these lines aren't needed when you don't have any attributes you want to list for them.
| attr_writer | ||
| attr_accessor | ||
|
|
||
| def initialize(num: 20) |
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 great way to set this up! A great example of where input params to a constructor can be totally different than the instance variable(s)!
| end | ||
| end | ||
|
|
||
| def self.create_rooms(num: 20) |
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.
Cool that you made this its own method!
|
|
||
| def res_by_room(rm_num, start_date, end_date) | ||
| res_array = [] | ||
| @found_room = @all_rooms.find {|room| room.rm_num} |
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 could/should be a local variable rather than an instance variable since it is only used within this one method.
That would be changed by just removing the @
| @found_room = @all_rooms.find {|room| room.rm_num} | |
| found_room = @all_rooms.find {|room| room.rm_num} |
|
|
||
| module Hotel | ||
| class Manager | ||
| attr_reader :num, :all_rooms, :rm_reservations, :recloc |
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 should only define readers for instance variable of the Hotel class. The only of these 4 that is an instance variable of Hotel is all_rooms.
| attr_reader :num, :all_rooms, :rm_reservations, :recloc | |
| attr_reader :all_rooms |
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 imagine you might understand that and just forgot to update this line.
| rm_num: nil, | ||
| cost_per_day: 200, | ||
| total_cost: nil, | ||
| recloc: 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.
I'm having trouble understanding what this variable represents. It's best to name variables in such a way that their purpose is clear to anyone who might read the code.
|
|
||
| module Hotel | ||
| class Reservation | ||
| attr_reader :start_date, :end_date, :rm_num, :cost_per_day, :total_cost, :recloc |
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.
These all correspond to instance variables. Great!
| def find_total_cost(rec_loc) | ||
| res = "" | ||
| @all_rooms.each do |room| | ||
| if room.rm_reservations.find{|res| res.recloc[:recloc] == rec_loc}.class == Hotel::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.
This is just slightly too indented.
I use this handy VSCode plugin called "indent-rainbow" to make that really easy to notice.
Assignment Submission: Hotel
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection