-
Notifications
You must be signed in to change notification settings - Fork 299
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
quin - time #36
base: master
Are you sure you want to change the base?
quin - time #36
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 reflected upon your code and the quality of the tests that do exist. I do see some room for improvement around testing edge cases and making sure to take a step back and refactor as code starts to get unwieldy. Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
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.
Good job. Some parts of your design were a little messy but it seems like you're aware of that based on your refactors.txt.
Here are some tips for how to clean up your code.
if Date.today > start_date || Date.today > end_date | ||
raise ArgumentError.new("Invalid date, you can't start or end a reservation in the past") | ||
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.
If you're going to have a check like this please make sure that your example dates in your tests are in the future. (Either by placing them far in the future or making them relative to Date.today
.)
Several of your tests fail because dates are now in the past.
describe "initialize" do | ||
it "I can access the list of all of the rooms in the hotel" do | ||
hotel = HotelSystem::Hotel.new | ||
hotel.must_be_instance_of HotelSystem::Hotel |
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.
Please use expect
when writing assertions:
hotel.must_be_instance_of HotelSystem::Hotel | |
expect(hotel).must_be_instance_of HotelSystem::Hotel |
Minitest generates lots of warnings otherwise. (And in future versions of Minitest this won't work.)
module HotelSystem | ||
class DateRange | ||
attr_accessor :start_date, :end_date | ||
|
||
def initialize(start_date, end_date) | ||
@start_date = start_date | ||
@end_date = end_date | ||
end | ||
|
||
def overlap?(other) | ||
return false | ||
end | ||
|
||
def include?(date) | ||
return false | ||
end | ||
|
||
def nights | ||
return 3 | ||
end | ||
|
||
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.
This looks like it's left over from the scaffolding.
In the future if you're not using code you should remove it.
@rooms.each do |room| | ||
if room_number == room.room_number | ||
return 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.
You can simplify this using find
:
@rooms.each do |room| | |
if room_number == room.room_number | |
return room | |
end | |
end | |
@rooms.find do |room| | |
room_number == room.room_number | |
end |
@rooms.each do |room| | ||
room.reservations.each do |reservation| | ||
if reservation.reservation_number == reservation_number | ||
return reservation | ||
end | ||
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.
If you're going to have reservations
stored in Room
you should have this functionality inside of a method there instead of here:
@rooms.each do |room| | |
room.reservations.each do |reservation| | |
if reservation.reservation_number == reservation_number | |
return reservation | |
end | |
end | |
end | |
@rooms.each do |room| | |
room.find_reservation(reservation_number) | |
end |
That properly encapsulates this functionality and means that Hotel
has to know less about Room
.
count = 0 | ||
reservation_start = reservation.start_date | ||
reservation_end = reservation.end_date | ||
while start_date < end_date | ||
while reservation_start < reservation_end | ||
if start_date == reservation_start | ||
count += 1 | ||
end | ||
reservation_start = reservation_start + 1 | ||
end | ||
reservation_start = reservation.start_date | ||
start_date += 1 | ||
end | ||
if (reservation.end_date - reservation.start_date == 1) && reservation_start == start_date | ||
return false | ||
else | ||
return count < 2 | ||
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.
This logic is very complex. Please comment the individual parts of something like this in the future.
def is_between_two_dates(range_wanted, date) | ||
return date >= range_wanted[0] && date <= range_wanted[1]; | ||
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.
This method would have been cleaner if it compared two DateRange
objects. (Instead of a two element array of Date
objects with a single date.)
def next_reservation_number | ||
max = 0 | ||
@rooms.each do |room| | ||
room.reservations.each do |reservation| | ||
if max < reservation.reservation_number | ||
max = reservation.reservation_number | ||
end | ||
|
||
end | ||
end | ||
return max + 1 | ||
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.
This can be simplified using max
:
def next_reservation_number | |
max = 0 | |
@rooms.each do |room| | |
room.reservations.each do |reservation| | |
if max < reservation.reservation_number | |
max = reservation.reservation_number | |
end | |
end | |
end | |
return max + 1 | |
end | |
def next_reservation_number | |
max = @rooms.max do |room| | |
room.reservations.max do |reservation| | |
reservation.reservation_number | |
end | |
end | |
return max + 1 | |
end |
check_dates(start_date, end_date) | ||
@start_date = start_date | ||
@end_date = end_date | ||
@total_cost = ((@end_date - @start_date)) * 200 |
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.
Extra parens:
@total_cost = ((@end_date - @start_date)) * 200 | |
@total_cost = (@end_date - @start_date) * 200 |
|
||
room = hotel.find_room(1) | ||
|
||
res1 = HotelSystem::Reservation.new(1, Date.today, Date.new(2020,03,29)) |
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 test will break after March 29th 2020:
res1 = HotelSystem::Reservation.new(1, Date.today, Date.new(2020,03,29)) | |
res1 = HotelSystem::Reservation.new(1, Date.today, Date.today + 15) |
Assignment Submission: Hotel
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection