-
Notifications
You must be signed in to change notification settings - Fork 49
Leaves - Samantha #41
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
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! I am particularly impressed by the way that you how you broke down complex functionality into methods in your HotelOrganizer class. One place you could grow is writing more comprehensive tests that consider both positive and negative edge and nominal cases. You did write a test that caught a bug in finding the available rooms for a given date range. Take a look back at this and ask a question if you are still having trouble seeing where this bug came from. |
test/hotel_organizer_test.rb
Outdated
| before do | ||
| @hotel_organizer = Hotel::HotelOrganizer.new | ||
|
|
||
| @reservation1 = @hotel_organizer.make_reservation(18, Date.new(2018,01,01), Date.new(2018,01,06), 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.
Consider storing your dates as variable so it is clear how the dates relate to each other and what you are testing.
| @reservation1 = @hotel_organizer.make_reservation(18, Date.new(2018,01,01), Date.new(2018,01,06), nil) | |
| start_date1 = Date.new(2018,01,01) | |
| end_date1 = start_date1 + 5 | |
| start_date2 = start_date1 + 2 | |
| end_date2 = start_date2 + 9 | |
| ... |
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.
Added refactors.txt file and roadmap to future changes.
lib/hotel_organizer.rb
Outdated
|
|
||
| while start_date != end_date | ||
| if reservation.date_range.include?(start_date) | ||
| result = 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.
You should return false as soon as you hit false, because you don't want to find an overlap and then reset to true, but you should not return true until you've looped through all the dates to make sure you haven't missed any overlaps.
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.
Fixed the loop and made new tests that are passing.
test/hotel_organizer_test.rb
Outdated
| list_rooms = @hotel_organizer.list_available_rooms(Date.new(2018,01,01),Date.new(2018,01,06)) | ||
|
|
||
| rooms_after = list_rooms.length | ||
| expect(rooms_after).must_equal 19 |
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.
Hrmm, it is returning 0. I think this is an issue in your rooms_available? method. See comment above.
test_0003_returns only the rooms that have dates available FAIL (0.00s)
Expected: 19
Actual: 0
/Users/becca/Documents/GitHub/c12-student-repos/hotel/test/hotel_organizer_test.rb:99:in `block (3 levels) in <top (required)>'
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.
Fixed method room_available? (room is now passed in as an argument and checked against the list of reservations), made new tests that are passing.
test/hotel_organizer_test.rb
Outdated
| end | ||
| end | ||
|
|
||
| describe "method add_reservation" 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.
It would be good to add tests here that test for different types of reservation conflicts and edge cases for allowed overlapping reservations.
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.
Fixed bug to make room show as available also on the last_day of reservation, made new test that's passing.
Hotel
Congratulations! You're submitting your assignment!
Comprehension Questions