-
Notifications
You must be signed in to change notification settings - Fork 50
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
Dominique - Leaves #42
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 broke down larger problems into bite-sized methods. I do see some room for improvement around removing code that causes warnings, better delegation among classes and less redundant method names. |
test/hotel_system_test.rb
Outdated
@@ -0,0 +1,105 @@ | |||
require_relative "test_helper" | |||
require "Pry" |
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 left in all of your require 'pry'
's, which causes a warning.
lib/hotel_system.rb
Outdated
found_reservations | ||
end | ||
|
||
def list_available_rooms (start_date, end_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.
/Users/devin/Documents/Ada/c12/hotel/lib/hotel_system.rb:52: warning: parentheses after method name is interpreted as an argument list, not a decomposed argument
This is because you put a space after list_available_rooms
test/hotel_system_test.rb
Outdated
start_date = Date.parse("2019-02-08") | ||
end_date = Date.parse("2019-02-12") | ||
|
||
result = @hotel_system.make_reservation(start_date, end_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.
/Users/devin/Documents/Ada/c12/hotel/test/hotel_system_test.rb:61: warning: assigned but unused variable - result
|
||
def initialize(start_date: , end_date:) | ||
@start_date = start_date | ||
@end_date = end_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.
If you use a class without any methods besides initialize
, you probably either don't need a class or your other classes are doing work that belongs to the empty class.
lib/hotel_system.rb
Outdated
|
||
if (reservation.date_range.start_date >= start_date && reservation.date_range.start_date < end_date) || | ||
(reservation.date_range.end_date >= start_date && reservation.date_range.end_date <= end_date) | ||
reserved_room_numbers.push(reservation.room.room_number) |
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.
The discussion of POODR chapter 4 probably helps here: your Hotel
class should be delegating this work to the other two classes as in the example that I gave.
lib/hotel_system.rb
Outdated
end | ||
|
||
def list_available_room_numbers(start_date, end_date) | ||
available_rooms = list_available_rooms(start_date, end_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.
list_available_room_numbers
calls list_available_rooms
. This is confusing. :(
in hotel system is an array of room objects, it made it harder to access the room numbers. I ended up making unnecessary methods in hotel system | ||
like "list_available_room_numbers" because I had to extract the room numbers from the room objects. If I had no room class and | ||
simply just made the rooms instance variable in hotel system an array of integers 1 -20, I wouldn't need the extra methods. | ||
Also, there was no reason to list the room cost. Every room is $200 and total cost is calculated in the reservations class. |
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.
That seems wrong to me? Isn't it 200 per day?
Hotel
Congratulations! You're submitting your assignment!
Comprehension Questions