-
Notifications
You must be signed in to change notification settings - Fork 40
Space - Diana #32
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 - Diana #32
Conversation
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 wrote such thorough unit tests! The number of edge cases you wrote out is incredible. That practice will be invaluable as you go into your internship and beyond :) I do see some room for improvement around ensuring that your tests all run when submitting the project. Running "rake" on the command line was not working for me. (See inline comments for more context on why.) It especially concerns me that the tests weren't working because it causes me to wonder if you were running the tests frequently while you were writing code. I'm hopeful you were running your tests and this was somehow working for you even though it wasn't for me. I also see room for improvement in terms of data structure choice, specifically for @rooms within HotelManager. See my inline comments for details. Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
| @@ -0,0 +1,183 @@ | |||
| require 'test_helper.rb' | |||
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 line was causing the tests to be unable to run. I had to change it to look like the other tests.
| require 'test_helper.rb' | |
| require_relative 'test_helper' |
| def rooms | ||
| @rooms = [] | ||
| 20.times do |i| | ||
| @rooms << {(("room#{i+1}").to_sym) => []} |
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 see that @rooms is setup as an array of hashes. For future reference, this would have been much easier to work with simply as a hash, where the key to each item in the has is the room number and the value is the array of reservations.
| # add reservation to master room list (@rooms) | ||
| reserved_room = Hotel::DateRange.new(start_date, end_date) | ||
| reserved = Hotel::Reservation.new(reserved_room.start_date, reserved_room.end_date) | ||
| @rooms[room_index][chosen_room] << reserved |
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.
If @rooms were updated to match what I've noted in the comment above, there wouldn't be a need to index into the data structure twice like it's done here.
Assignment Submission: Hotel
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection