-
Notifications
You must be signed in to change notification settings - Fork 49
Branches - Kelsey #24
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
…f available rooms by date
…scount to a Reservation
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! Your code looks great! Overall, it's well-written code; it's clean, logical, readable, and with great code style. Overall, I think you made great decisions; your details in optional arguments and class and method design, thorough test cases (and a lot of great edge cases), and helper methods that broke up functionality are all great habits that I hope you keep! Overall, one big comment for improvement is about test organization: If the "Act" that you're testing is a method that belongs in one class, then that test belongs to that class's tests, even if it's conceptually about a different class. A huge thing you did was test making reservations in Blocks/about Blocks in the block tests, even though it's a ReservationManager method. Just a note for next time! Also, I have a few comments for small suggestions of improvement attached to your code. This all being said, you did a great job on this project, and I have a lot of confidence in your fundamentals! Keep it up! |
| module Hotel | ||
| class ReservationManager | ||
|
|
||
| attr_accessor :rooms, :reservations, :blocks |
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'd probably use attr_reader here! (Your tests still pass if it's attr_reader)
lib/reservation_manager.rb
Outdated
| @calendar = calendar | ||
| end | ||
|
|
||
| def request_reservation(start_date, end_date, type: :room, block_id: nil, amount: 1) |
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 method is great!
lib/reservation_manager.rb
Outdated
| return block | ||
| end | ||
|
|
||
| def create_reservation(start_date, end_date, room, discount: discount) |
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 like these helper methods!!
| available_rooms_by_id = available_rooms_by_id - [reservation.room.id] | ||
| end | ||
| end | ||
| if available_rooms_by_id[0] == 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.
Minor suggestion: consider refactoring this so it's more clearly checking that the array is empty, like .empty? or .length == 0
| end | ||
| end | ||
| if available_rooms_by_id[0] == nil | ||
| return [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.
Minor suggestion: Instead of returning [nil], maybe an empty array would feel more appropriate
| if room.id == id | ||
| rooms_by_id << room | ||
| end | ||
| 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.
Consider using .select instead here! That method should give you back an array of everything that matches your criteria
test/block_test.rb
Outdated
| end | ||
|
|
||
| it "adds up to 5 rooms to a Block" do | ||
| new_block = @hotel.request_reservation(Date.parse('2019-10-31'), Date.parse('2019-11-04'), type: :block, amount: 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.
Because the "Act" step is a method that belongs to a ReservationManager (aka the method request_reservation), this test should be moved to the reservation_manager_test
test/block_test.rb
Outdated
| expect{@hotel.request_reservation(Date.parse('2019-10-31'), Date.parse('2019-11-04'), type: :block, amount: 5)}.must_raise ArgumentError | ||
| end | ||
|
|
||
| it "raises an exception if adding a Block to a Block" 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!
test/reservation_manager_test.rb
Outdated
|
|
||
| # adds conflicting reservations to test data | ||
| 14.times do |i| | ||
| @test_reservations << Hotel::Reservation.new(Date.parse('2019-10-31'), Date.parse('2019-11-04'), @test_rooms[i + 6]) |
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 setup! This is a very subtle thing, but... you add test reservations to your test reservation manager by shoveling in Reservations into @test_reservations. Instead of relying on shoveling into @test_reservations, in this case, it may be good in the setup to literally use @hotel.request_reservation so you rely on the reservation manager's interface to add reservations, without relying on knowing that it's an array (that can be modified by reference)
| end | ||
|
|
||
| def get_date_range(start_date, end_date) | ||
| if !valid_start_date?(start_date) |
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.
Clever, but was this a requirement for our project? :)
Also, consider this: What happens to your tests in a year?
Otherwise, I really like how this method/class is structured: it separately has methods to determine if one property is valid. Keep this pattern up!
Hotel
Congratulations! You're submitting your assignment!
Comprehension Questions