Open
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 cleaned up your code for submission and made lots of small, solid commits! You seem to have double checked and gotten rid of any warnings that your code threw, which is really nice to see! I also love that you were constantly committing code as you worked on it! It will totally save your bacon in the future! I do see some room for improvement around separating class responsibilities. Your code has a lot of classes that only initialize themselves. Thing is, if all a class does is initialize itself, why couldn't it just be a hash? See my comments for where this could be improved! Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
dHelmgren
reviewed
Mar 22, 2020
Comment on lines
+4
to
+29
| class DateRange | ||
| attr_reader :start_date, :end_date, :range | ||
|
|
||
| def initialize(start_date:, end_date:) | ||
| raise ArgumentError.new("start and end dates must be Date objects") if start_date.class != Date && end_date.class != Date | ||
| @start_date = start_date | ||
| @end_date = end_date | ||
| @range = (@start_date .. @end_date).to_a | ||
| raise ArgumentError.new("End date can not be before start date") if @end_date < @start_date | ||
| raise ArgumentError.new("Reservation can not be before today") if @end_date < Date.today || @start_date < Date.today | ||
|
|
||
| end | ||
|
|
||
| def overlap?(date_range) | ||
| # if the array of the combined arrays is empty, there is no overlap, so return false | ||
| # if the array is not empty, return true | ||
| overlap = (self.range & date_range.range).empty? || (date_range.start_date >= self.end_date) || (date_range.end_date <= self.start_date) ? false : true | ||
| return overlap | ||
| end | ||
|
|
||
| def nights | ||
| return @range.count - 1 | ||
| end | ||
|
|
||
|
|
||
| end |
There was a problem hiding this comment.
A thought you might enjoy chewing on: how would your code change if class DateRange < Range?
Comment on lines
+2
to
+18
| class HotelBlock | ||
| attr_reader :block_count, :date_range, :discount_cost | ||
| attr_accessor :rooms | ||
|
|
||
| def initialize(block_count:, date_range:, discount_cost:) | ||
| @block_count = block_count # I interpreted the user story "collection of rooms" to mean I can pass it a | ||
| #count and the count determines how many rooms to request for the block. | ||
| @date_range = date_range | ||
| @discount_cost = discount_cost | ||
| @rooms = [] | ||
|
|
||
| raise ArgumentError.new("Block count must be an integer between 2 and 5 inclusive") if @block_count.between?(2,5) == false | ||
|
|
||
| end | ||
|
|
||
| end | ||
|
|
There was a problem hiding this comment.
I tend to get suspicious when a class only has initialize as a method. Usually, this means that another class is managing the data that belongs to this class.
Comment on lines
+56
to
+99
| def check_block_status(date_range) | ||
| available_rooms = available_rooms(date_range) | ||
| @hotel_blocks.each do |hotel_block| | ||
| if hotel_block.date_range.overlap?(date_range) == true | ||
| hotel_block.rooms.each do |block_room| | ||
| available_rooms.delete(block_room) | ||
| end | ||
| end | ||
| end | ||
| return available_rooms | ||
| end | ||
|
|
||
| def request_block(block_count, date_range, discount_cost) | ||
| available_rooms = check_block_status(date_range) | ||
| raise NoAvailableRoomError.new("Not enough available rooms to fulfill block") if available_rooms.count < block_count | ||
|
|
||
| hotel_block = Hotel::HotelBlock.new(block_count: block_count, date_range: date_range, discount_cost: discount_cost) | ||
|
|
||
| x = 0 | ||
| until x == block_count do | ||
| available_room = available_rooms[x] | ||
| available_room.change_cost(discount_cost) | ||
| hotel_block.rooms << available_room | ||
| x += 1 | ||
| end | ||
| @hotel_blocks << hotel_block | ||
| return hotel_block | ||
| end | ||
|
|
||
| def available_rooms_in_block(hotel_block) | ||
| available_rooms = hotel_block.rooms.select{|room| room.reservations.empty? == true || room.reservations.any?{|reservation| reservation.date_range.overlap?(hotel_block.date_range) == false}} | ||
| return available_rooms | ||
| end | ||
|
|
||
| def add_reservation_to_room_in_block(hotel_block) | ||
| available_rooms = available_rooms_in_block(hotel_block) | ||
| raise NoAvailableRoomError.new("there are no available rooms left in this block")if available_rooms.empty? == true | ||
| chosen_room = available_rooms[0] | ||
| new_reservation = Hotel::Reservation.new(date_range: hotel_block.date_range, room: chosen_room) | ||
| new_reservation.block_reservation = true | ||
| @reservations << new_reservation | ||
| chosen_room.add_room_reservation(new_reservation) | ||
| return new_reservation | ||
| end |
There was a problem hiding this comment.
At least a couple of these methods probably belong to HotelBlock
Comment on lines
+31
to
+52
| it "returns true if there is overlap between date ranges" do | ||
| date_range2 = Hotel::DateRange.new(start_date: Date.today + 2, end_date: Date.today + 4) | ||
| date_range3 = Hotel::DateRange.new(start_date: Date.today + 3, end_date: Date.today + 6) | ||
| date_range4 = Hotel::DateRange.new(start_date: Date.today + 1, end_date: Date.today + 9) | ||
| date_range5 = Hotel::DateRange.new(start_date: Date.today + 1, end_date: Date.today + 6) | ||
| expect(@date_range.overlap?(date_range2)).must_equal true | ||
| expect(@date_range.overlap?(date_range3)).must_equal true | ||
| expect(@date_range.overlap?(date_range4)).must_equal true | ||
| expect(@date_range.overlap?(date_range5)).must_equal true | ||
| end | ||
|
|
||
| it "returns false if the date is not within the range" do | ||
| date_range2 = Hotel::DateRange.new(start_date: Date.today + 6, end_date: Date.today + 9) | ||
| date_range3 = Hotel::DateRange.new(start_date: Date.today + 7, end_date: Date.today + 13) | ||
| date_range4 = Hotel::DateRange.new(start_date: Date.today + 1, end_date: Date.today + 2) | ||
| date_range5 = Hotel::DateRange.new(start_date: Date.today + 19, end_date: Date.today + 20) | ||
| date_range5 = Hotel::DateRange.new(start_date: Date.today , end_date: Date.today + 1) | ||
| expect(@date_range.overlap?(date_range2)).must_equal false | ||
| expect(@date_range.overlap?(date_range3)).must_equal false | ||
| expect(@date_range.overlap?(date_range4)).must_equal false | ||
| expect(@date_range.overlap?(date_range5)).must_equal false | ||
| end |
There was a problem hiding this comment.
I would give these date ranges more explicit names, so that others can check your work more easily.
Comment on lines
+4
to
+9
| before do | ||
| @front_desk = Hotel::FrontDesk.new | ||
| @dates = Hotel::DateRange.new(start_date: Date.today + 2, end_date: Date.today + 6) | ||
| @block_count = 3 | ||
| @discount_cost = 0.2 | ||
| end |
There was a problem hiding this comment.
Are you using all of these for each test?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Assignment Submission: Hotel
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection