Space - Lee #28
Space - Lee #28
Conversation
… test for readability
…servations are being returned, refactors tests
…art_date before the range_start, adds test
…ity checking logic respects room blocks
…ncorrect, comments out erroneous functions and tests
…it also checks Blocks
… individual Reservations
…available for the given date range
…eservations_by_date
…low for reservation of specific rooms in blocks
…antiation, changes the #total_cost method
…h a default value of 200.0
…tions made from a hotel block
…n when a new block includes a date in an existing block
…rown if a new block includes a room from an existing block
…es the cost for rooms reserved from a hotel block
…ent rooms in the #reserve_room method
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 put so much thought into your design, that was really apparent from your answers to the comprehension questions. I was also impressed by how you set up your unit tests. I do see some room for improvement around being a little too clever for your own good sometimes. Clever solutions are great but first make sure that a simple one won't do. Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
kaidamasaki
left a comment
kaidamasaki
left a comment
There was a problem hiding this comment.
Great job! I've noted a few small things you can fix but this is an incredibly solid solution overall.
| @@ -1,3 +1,4 @@ | |||
| .DS_Store | |||
There was a problem hiding this comment.
You can ignore these everywhere by creating a global gitignore file.
| end_date: end_date | ||
| ) | ||
|
|
||
| raise ArgumentError.new("Block must contain between 2 and 5 rooms!") if @rooms.length < 2 || @rooms.length > 5 |
There was a problem hiding this comment.
I'd generally recommend raising errors like this before initializing fields. It doesn't matter here but can be helpful as constructors get more complex.
Also, it's helpful to include the bad argument when you raise an ArgumentError:
| raise ArgumentError.new("Block must contain between 2 and 5 rooms!") if @rooms.length < 2 || @rooms.length > 5 | |
| raise ArgumentError.new("Block must contain between 2 and 5 rooms! Got: #{@rooms}") if @rooms.length < 2 || @rooms.length > 5 |
| @@ -0,0 +1,17 @@ | |||
| module Hotel | |||
| class Block | |||
There was a problem hiding this comment.
This is too much indentation. You should configure VS Code to indent by two spaces.
| low = 0 | ||
| high = self.nights | ||
|
|
||
| while low <= high | ||
| mid = (low + high) / 2 | ||
| if (@start_date + mid) == date | ||
| return true | ||
| elsif (@start_date + mid) > date | ||
| high = mid - 1 | ||
| elsif (@start_date + mid) < date | ||
| low = mid + 1 | ||
| end | ||
| end | ||
|
|
||
| return false |
There was a problem hiding this comment.
This is cool but I'm pretty sure binary search is overkill here.
I think you can just do something like return @start_date < date && date < @end_date (after fixing the off by one error that's probably there...)
| def find_available_room(date_range) | ||
| unavailable_rooms = @reservations.map { |reservation| | ||
| reservation.room if reservation.date_range.start_date < date_range.end_date && reservation.date_range.end_date > date_range.start_date | ||
| } | ||
| @blocks.each { |block| | ||
| (block.rooms).each { |room| unavailable_rooms << room } if block.date_range.start_date < date_range.end_date && block.date_range.end_date > date_range.start_date | ||
| } # This method is very expensive. See refactors.txt for details. | ||
| available_rooms = @rooms - unavailable_rooms | ||
|
|
||
| raise ArgumentError.new("No rooms available for that date range!") if available_rooms.empty? | ||
| return available_rooms | ||
| end |
There was a problem hiding this comment.
This method isn't as expensive as you think. Or at least it's not much worse than an optimal solution.
For space complexity you can't do better than O(n) (because you might need to return an array of every room if they're all available).
For time complexity it's a little trickier. Using a clever data structure you can get individual reservation comparison down to O(log(n)) but that still only gets you O(n*log(n)).
If you use a data structure that O(r) where r is the number of days in all reservations then you can get the time down to O(rd) where d is the number of days in the input range but that uses a lot of space.
Off the top of my head I can't think of a solution that performs well in both space and time so this overall seems pretty reasonable. Some operations are just inherently expensive no matter how well you code your solution.
| start_date: date_range.start_date, | ||
| end_date: date_range.end_date |
There was a problem hiding this comment.
I probably would have just stored the DateRange object itself here instead of breaking it apart.
| end_date: end_date | ||
| ) | ||
|
|
||
| raise ArgumentError.new("Invalid rate!") if rate.class != Float |
There was a problem hiding this comment.
If you're going to check types you should probably use kind_of? to allow subclassing.
| raise ArgumentError.new("Invalid rate!") if rate.class != Float | |
| raise ArgumentError.new("Invalid rate!") unless rate.kind_of?(Float) |
| it "is set up for specific attributes and data types" do | ||
| [:id, :rooms, :rate, :date_range].each do |attribute| | ||
| expect(@block).must_respond_to attribute | ||
| end | ||
|
|
||
| expect(@block.id).must_be_kind_of Integer | ||
| expect(@block.rooms).must_be_kind_of Array | ||
| expect(@block.rate).must_be_kind_of Float | ||
| expect(@block.date_range).must_be_kind_of Hotel::DateRange | ||
| end |
There was a problem hiding this comment.
Something confusing happened with your indentation here.
| it "is set up for specific attributes and data types" do | ||
| [:start_date, :end_date].each do |attribute| | ||
| expect(@date_range).must_respond_to attribute | ||
| end | ||
|
|
||
| expect(@date_range.start_date).must_be_kind_of Date | ||
| expect(@date_range.end_date).must_be_kind_of Date | ||
| end |
There was a problem hiding this comment.
Ah, I think that you're mixing tabs and spaces and that's why weird things are happening.
There was a problem hiding this comment.
Yeah, I've been having trouble with this for a while... I have the Tab Size set to 2 in VS Code, so it looks fine there. But when I commit my files, they are always formatted like this.
Oh wait, I just poked around in the VS Code settings and found a checkbox that says it'll override tabs with spaces. Hopefully that'll do the trick!
@kaidamasaki Thank you for all your feedback! By the way, your comment about making sure that there isn't a simpler solution first is so spot on. I tend to create extra challenges for myself in order to keep my mind engaged, so it's been a tough habit to break. I really appreciate your thoughtfulness and will remind myself of that in future projects! |
2 participants
Assignment Submission: Hotel
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection
accurately calculates the total cost for single rooms with the default rate. This is a nominal case because it tests a behavior that I expect during most calls of the #total_cost method.throws an exception if there are no rooms available. This is an edge case because it tests a behavior that will only occur when the #find_available_method is called under extreme circumstances (i.e., when all the rooms in the hotel are booked during the given date range).