-
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
Sara - Branches #36
base: master
Are you sure you want to change the base?
Sara - Branches #36
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! Overall, you did a great job of putting into action a lot of our coding practices; You have a lot of good work around making large classes that rely on each other. You used composition correctly! You use instance variables and instance methods correctly! You used However, I think I see some discomfort around organizing code, and seeing how it all connects. It feels like you were trying to organize too much at the same time, and there's lack of clarity around how your tests and how your methods are organized. For example, tests were sometimes in the wrong area of the file! Other times, methods that were named one thing were doing something else. Another example is that sometimes you wrote a method in Class A that did Functionality A, and then you wrote a method in Class B that did the same functionality! I'm leaving a lot of comments with questions to ask yourself that will help you organize your code. When you have well-organized code, then you can start to target your code and your time better. :) You didn't install Lastly, there are a lot of files that are in your project submission that don't get used. I encourage you to delete those as soon as you realize you don't need them. The less files you have, the easier it is to keep track of your code. :) One more overall thought: We went through in class that we would not have a I'd love to know how you missed this information so we can make sure you get the same design information and hints as everyone else in class. Overall, from your hotel project, I have full confidence that you have fundamental programming skills! I also see that we should spend time to make sure that you think of code organization and relationships between different pieces of code correctly. If we don't feel a little more comfortable about our file system and tests, it may give us challenges later. |
# .ruby-gemset | ||
|
||
# unless supporting rvm < 1.11.0 or doing something fancy, ignore this: | ||
.rvmrc |
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.
Hm, why did you delete the contents of this file?
attr_reader :start_date, :end_date | ||
|
||
def initialize(start_date:, end_date:) | ||
@start_date = convert_to_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.
Could we use Date.parse(start_date)
instead of using the convert_to_date
method?
end | ||
end | ||
|
||
def convert_to_date(date_str) |
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 doesn't get tested!
return night_count | ||
end | ||
|
||
def overlap?(duration) |
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 never gets tested OR used in any other file. Can we delete it, or can we refactor our code to use it?
# require_relative 'reservation' | ||
|
||
# one_reservation = Reservation.new(id:1, customer_id:2, room:303, duration:3, total_cost:300) | ||
# date_range = DateRange.new(id:, start_day:, end_day:) |
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.
Feel free to delete this file before project submission
end | ||
|
||
#list of rooms that are not reserverd | ||
def get_availbale_rooms(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.
"available" is mispelled in your method name-- watch out!
def get_availbale_rooms(date) | ||
available_rooms = @rooms.find_all { |room| room.availability == true } | ||
# rooms_date_range = available_rooms.find_all { |room| (date >= room.reservation.duration.start_date && date <= room.reservation.duration.end_date) } | ||
return available_rooms |
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 doesn't use the date
parameter. Why not? What does it mean for a room to have availability
? Isn't the requirement to find rooms that are available for a specific date? What does it mean when a room is "available"?
return total_cost | ||
end | ||
|
||
def reserve_room_on(cust_id, room_number, 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.
What is the difference between reserve_room_on
and make_reservation
? How can we change the name of these methods so that they are more clear about what they do and what the differences are?
def reserve_room_on(cust_id, room_number, start_date, end_date) | ||
room = @rooms.find { |room| room.number == room_number } | ||
if room.availability == false || room.reservation != nil | ||
raise ArgumentError.new "This room is already 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.
Minor suggestion: Instead of an ArgumentError
, could we raise a more specific kind of error, or create a new custom Error class?
end | ||
end | ||
|
||
HotelBooking.main |
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.
Feel free to delete this file before project submission
@id = id | ||
@number = number | ||
@cost = cost, | ||
@availability = availability, |
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.
Why are there commas here and the line above? Also, watch your indentation!
@reservation = reservation | ||
end | ||
|
||
def total_cost() |
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 isn't implemented or used anywhere else, so feel free to delete this method
@name = name | ||
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.
No tests! Maybe this is a result of realizing that our project doesn't require Customers being implemented, so it was hard to understand what to test a Customer to do :)
}.must_raise ArgumentError | ||
end | ||
end | ||
describe "" 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.
Don't forget to add a description to your describe
blocks. If you can't find a good name, then maybe you need to think about how you're organizing your code
end | ||
end | ||
|
||
describe "add_customer" 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.
Hm, did we have a requirement to make sure that we could add a customer?
end | ||
|
||
describe "add_customer" do | ||
it "checks the number of customers before and after" 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.
Good test though!
before do | ||
@reservation_list = @hotel.reservations | ||
end | ||
it "reservation should be a list" 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.
Maybe a more accurate sentence is "reservation_list starts as an empty array"
end | ||
end | ||
|
||
it "get the total cost for a given 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.
This test isn't in a describe
block that is accurate-- it's in the describe block named "test list of reservations"
. Could we make a different describe
block with a name that is more accurate, and put this test inside there? Or could we rename the current describe
block so it's more accurate?
cust_id = @customer.id | ||
@hotel.make_reservation(cust_id, "2019-09-01", "2019-09-05") | ||
available_rooms = @hotel.get_availbale_rooms(date) | ||
expect(available_rooms.length).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.
What happens if you make a reservation for "2999-01-01" to "2999-01-05"? Your available_rooms.length
for available rooms on date (2019-09-02) is 18
, not 19
. Your implementation doesn't use date, and your tests don't check for this case!
end | ||
it "reserve an already reserve room" do | ||
cust_id = @customer.id | ||
@hotel.make_reservation(cust_id, "2019-09-01", "2019-09-05") |
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.
Why do we use make_reservation
and reserve_room_on
? Is there a way that our code could be refactored so only one of these methods exists?
duration: @duration) | ||
end | ||
|
||
describe "cost" 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.
You can put multiple it
blocks in one describe
block. Right now, you have two describe
blocks that are both named "cost"
... instead, consider moving the it
blocks under the same describe
block.
def total_cost() | ||
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.
You have no tests for Room
:(
Hotel
Congratulations! You're submitting your assignment!
Comprehension Questions