-
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
Time - Hannah #42
base: master
Are you sure you want to change the base?
Time - Hannah #42
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 wrote thorough tests for the DateRange methods. Furthermore, you eloquently worked through tricky logic to assign a room to a reservation. It is clear that the learning goal around object oriented design were met for this project. I do see some room for improvement around the thoroughness of your tests. It is important to test nominal and edge failure and success cases for each of your methods. Using TDD practices and pseudocode can help you consider all the test cases before you even implement the code. Please let me know if you have questions about the tests for this project. Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
else | ||
return true | ||
end | ||
# if date_range1.start_date <= date_range2.start_date && date_range1.end_date >= date_range2.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.
Nice work refactoring this code. You should delete the commented code. This would be a great moment to commit! git commit -m "refactored DateRange.overlap?
.
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 making this in an instance method such that it would be called like this date_range1.overlap?(date_range2)
# get that reservation_id to get the duration | ||
# after getting duration, calculate cost | ||
|
||
# found_reservation = @reservations.find do |reservation| |
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.
Remember to remove unused code before submitting.
# Assert | ||
expect (duration).must_equal 4 | ||
end | ||
describe "overlap" 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.
These tests are very comprehensive. Nice work! Consider naming them with the particular test case they test for. See the example below from the design scaffold.
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.
describe "overlap?" do
before do
start_date = Date.new(2017, 01, 01)
end_date = start_date + 3
@range = Hotel::DateRange.new(start_date, end_date)
end
it "returns true for the same range" do
start_date = @range.start_date
end_date = @range.end_date
test_range = Hotel::DateRange.new(start_date, end_date)
expect(@range.overlap?(test_range)).must_equal true
end
xit "returns true for a contained range" do
end
xit "returns true for a range that overlaps in front" do
end
xit "returns true for a range that overlaps in the back" do
end
xit "returns true for a containing range" do
end
xit "returns false for a range starting on the end_date date" do
end
xit "returns false for a range ending on the start_date date" do
end
xit "returns false for a range completely before" do
end
xit "returns false for a date completely after" do
end
end
xdescribe "include?" do
it "reutrns false if the date is clearly out" do
end
it "returns true for dates in the range" do
end
it "returns false for the end_date date" do
end
end
def find_avail_room(date_range) | ||
rooms.each do |room| | ||
room_reservation = reservations.select { |reservation| reservation.room_num == room } | ||
if !room_reservation.any? { |reservation| DateRange.overlap?(reservation.date_range, date_range) } |
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 use of an enumerable method.
expect(res_array.length).must_equal 1 | ||
end | ||
|
||
it "raises exception, if no available rooms" 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 is a clever way to test this edge case!
require_relative "test_helper" | ||
|
||
describe "System class" do | ||
it "gets lists of rooms" 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.
To increase readability and ensure you are fully testing your methods, group the tests for a single method together using a describe block.
Assignment Submission: Hotel
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection