Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@


*.gem
*.rbc
/.config
Expand Down Expand Up @@ -48,3 +50,4 @@ build-iPhoneSimulator/

# unless supporting rvm < 1.11.0 or doing something fancy, ignore this:
.rvmrc
coverage
22 changes: 22 additions & 0 deletions lib/date.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
require 'date'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid naming files the same things as built in Ruby gems you're using. This can confuse Ruby and cause errors.

Also, we try to make sure that if there's a single class in a file that the name matches it (but snake_case instead of CamelCase).


require_relative 'frontdesk.rb'
require_relative 'reservation.rb'
Comment on lines +3 to +4

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't require_relative files that you don't directly reference in here.

Specifically this causes a circular dependency which can confuse Ruby. (frontdesk.rb requires date.rb which requires frontdesk.rb.)

Why would you decide to keep a system that's this fragile? No idea. I hope they change it for Ruby 3.0!


module Hotel
class DateRange

def contains(date)
(date >= @start_date && date < @end_date) ? true : false

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use explicit an return; also, your condition is already a boolean so you can return it directly:

Suggested change
(date >= @start_date && date < @end_date) ? true : false
return date >= @start_date && date < @end_date

end

def no_conflict?(new_start, new_end)
if @end_date == new_start || new_start > @end_date || new_end < @start_date || new_end == @start_date
return true
elsif new_start >= @start_date && new_start < @end_date || new_end > @start_date
return false
end
end
Comment on lines +13 to +19

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to note about ifs that don't have an else. They have an implied else that produces nil.

So this winds up being this when you make that explicit:

      def no_conflict?(new_start, new_end)
        if @end_date == new_start || new_start > @end_date || new_end < @start_date || new_end == @start_date
          return true
        elsif new_start >= @start_date && new_start < @end_date || new_end > @start_date
          return false
        end
          return nil
        end
      end

Because nil is falsey this is roughly equivalent to:

      def no_conflict?(new_start, new_end)
        if @end_date == new_start || new_start > @end_date || new_end < @start_date || new_end == @start_date
          return true
        elsif new_start >= @start_date && new_start < @end_date || new_end > @start_date
          return false
        end
          return false
        end
      end

Since the elsif and the else blocks both produce false then you can omit them entirely and just return your condition:

Suggested change
def no_conflict?(new_start, new_end)
if @end_date == new_start || new_start > @end_date || new_end < @start_date || new_end == @start_date
return true
elsif new_start >= @start_date && new_start < @end_date || new_end > @start_date
return false
end
end
def no_conflict?(new_start, new_end)
return @end_date == new_start || new_start > @end_date || new_end < @start_date || new_end == @start_date
end


end
end
85 changes: 85 additions & 0 deletions lib/frontdesk.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
require 'date'

require_relative 'date.rb'
require_relative 'reservation.rb'

module Hotel
class FrontDesk

attr_reader :all_reservations, :rooms

def initialize
@all_reservations = all_reservations || []
@rooms = [*1..20]
end

def assign_room(new_reservation)
taken_rooms = []
all_rooms = @rooms.dup
@all_reservations.each do |reservation|
reservation.no_conflict?(new_reservation.start_date, new_reservation.end_date) ? taken_rooms << reservation.assigned_room : next
end
room_to_assign = (all_rooms - taken_rooms.flatten)
assigning = room_to_assign.sample(new_reservation.num_rooms)
taken_rooms.include?(assigning)
assigning = room_to_assign.sample(new_reservation.num_rooms)
end
Comment on lines +16 to +26

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simplify this using reject:

Suggested change
def assign_room(new_reservation)
taken_rooms = []
all_rooms = @rooms.dup
@all_reservations.each do |reservation|
reservation.no_conflict?(new_reservation.start_date, new_reservation.end_date) ? taken_rooms << reservation.assigned_room : next
end
room_to_assign = (all_rooms - taken_rooms.flatten)
assigning = room_to_assign.sample(new_reservation.num_rooms)
taken_rooms.include?(assigning)
assigning = room_to_assign.sample(new_reservation.num_rooms)
end
def assign_room(new_reservation)
return @all_reservations.reject do |reservation|
reservation.no_conflict?(new_reservation.start_date, new_reservation.end_date)
end.sample
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, at least reading this it looks like you're rejecting all of the rooms that don't conflict which confuses me.


def add_reservation(start_date, end_date, num_rooms)
new_reservation = Hotel::Reservation.new(start_date: start_date, end_date: end_date, num_rooms: num_rooms)
new_reservation.assigned_room = assign_room(new_reservation)
if new_reservation.num_rooms > 1
raise ArgumentError.new("Invalid room reservation request: if you would still like #{num_rooms} rooms please reserve as a block.")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Bad arguments in the ArgumentError message! 🎉

I probably would have just left num_rooms off as an argument though, since it has to be 1.

end
@all_reservations << new_reservation
end

def add_block_reservation(start_date, end_date, num_rooms, discount, block_key)
new_reservation = Hotel::Reservation.new(start_date: start_date, end_date: end_date, num_rooms: num_rooms, discount: discount, block_key: block_key)
new_reservation.assigned_room = assign_room(new_reservation)
if new_reservation.num_rooms > 1
new_reservation.block = :BLOCK
elsif new_reservation.num_rooms == 1
raise ArgumentError.new("Invalid room reservation request: if you would still like #{num_rooms} room please reserve as a single.")
end
@all_reservations << new_reservation
end

def find_reservation_by_date(date)
@all_reservations.select {|reservation| reservation.contains(date)}
end

def find_available_room_by_date(date)
taken = []
all_rooms = @rooms.dup
booked = find_reservation_by_date(date)
booked.map {|reservation| taken << reservation.assigned_room}
available_rooms = (all_rooms - taken.flatten)
end

def find_block_by_block_key(date, block_key)
reservations_on_day = find_reservation_by_date(date)
in_block = reservations_on_day.select {|reservation| reservation.block_key == block_key}
return in_block
end

def update_block(date, block_key)
found_block = find_block_by_block_key(date, block_key)
if found_block[0].num_rooms == 0
raise ArgumentError.new("Invalid room reservation request: all rooms in this block have been reserved.")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't really an ArgumentError since nothing was wrong with the arguments themselves.

I'd recommend making a custom error class like this:

module Hotel
  class BlockFullError < StandardError
  end
end

And then using it like this:

Suggested change
raise ArgumentError.new("Invalid room reservation request: all rooms in this block have been reserved.")
raise BlockFullError.new("Invalid room reservation request: all rooms in this block have been reserved.")

else
found_block[0].num_rooms -= 1
end
return found_block[0]
end

# * I can reserve a specific room from a hotel block
# * I can only reserve that room from a hotel block for the full duration of the block
def reserve_from_block(date, block_key)
book_room = update_block(date, block_key)
book_room.assigned_room = book_room.assigned_room[0]
book_room.num_rooms = 1
return book_room
end
end
end
53 changes: 53 additions & 0 deletions lib/reservation.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
require 'date'


require_relative 'frontdesk.rb'
require_relative 'date.rb'

module Hotel
class Reservation < DateRange
attr_accessor :start_date, :end_date, :num_rooms, :assigned_room, :discount, :block, :block_key

COST = 200.00

def initialize(start_date:, end_date:, num_rooms:, discount: nil, block: :SINGLE, block_key: nil)
@start_date = start_date
@end_date = end_date
@num_rooms = num_rooms
@assigned_room = []
@discount = discount
@block = block
@block_key = block_key

raise ArgumentError.new("Invalid times: #{@start_date} comes after #{@end_date}") if (@start_date > @end_date || @start_date == @end_date)

raise ArgumentError.new("Invalid room request: provided #{@num_rooms} instead of an Integer") if @num_rooms.class != Integer

raise ArgumentError.new("Invalid room request: requested #{@num_rooms} rooms, #{@num_rooms-5} too many.") if @num_rooms > 5

raise ArgumentError.new("Invalid block key. Not valid for single room reservations") if @num_rooms == 1 && block_key != nil

raise ArgumentError.new("Status and room argument: #{@num_rooms} room, should not be noted as a block") if (@num_rooms == 1 && @block == :BLOCK)

raise ArgumentError.new("Invalid status. Please choose: single or block.") unless [:SINGLE, :BLOCK].include? block
end

def total_cost
@num_rooms == 1 ? final_bill = (@end_date - @start_date)*@num_rooms*COST : final_bill = (@end_date - @start_date)*@num_rooms*COST*(1 - @discount)
return final_bill
end
Comment on lines +35 to +38

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You seem very fond of ternary operators but you got a little carried away here:

Suggested change
def total_cost
@num_rooms == 1 ? final_bill = (@end_date - @start_date)*@num_rooms*COST : final_bill = (@end_date - @start_date)*@num_rooms*COST*(1 - @discount)
return final_bill
end
def total_cost
if @num_rooms == 1
return (@end_date - @start_date) * @num_rooms * COST
else
return (@end_date - @start_date) * @num_rooms * COST * (1 - @discount)
end
end

When code is complex breaking it up into multiple lines can help make it much easier to read.

If you're going to do something like this as a ternary I'd recommend factoring out the assignment like this (since it can be easy to overlook and changes state):

Suggested change
def total_cost
@num_rooms == 1 ? final_bill = (@end_date - @start_date)*@num_rooms*COST : final_bill = (@end_date - @start_date)*@num_rooms*COST*(1 - @discount)
return final_bill
end
def total_cost
final_bill = @num_rooms == 1 ? (@end_date - @start_date) * @num_rooms * COST : (@end_date - @start_date) * @num_rooms * COST * (1 - @discount)
return final_bill
end

In fact, looking at this code more makes it clear there's a lot of duplication between the two branches so it would be reasonable to use a ternary to calculate the multiplier though I'd still want to assign it to an intermediate variable:

Suggested change
def total_cost
@num_rooms == 1 ? final_bill = (@end_date - @start_date)*@num_rooms*COST : final_bill = (@end_date - @start_date)*@num_rooms*COST*(1 - @discount)
return final_bill
end
def total_cost
multiplier = @num_rooms == 1 ? (1 - @discount) : 1
return (@end_date - @start_date) * @num_rooms * COST * multiplier
end


#DateRange class originally lived here as methods
# def contains(date)
# (date >= @start_date && date < @end_date) ? true : false
# end

# def conflict?(new_start, new_end)
# if new_start >= @start_date && new_start < @end_date || new_end > @start_date
# return false
# elsif @end_date = new_start || new_start > @end_date || new_end < @start_date || new_end == @start_date
# return true
# end
# end
end
end
17 changes: 17 additions & 0 deletions refactors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
A few things to change moving forward:

Tighter code
1. Refactor code with Enumerable methods where possible
2. Consider where ternary format could be used
3. Reconsider where other helper methods could be created

Block class
1. Consider pulling out a separate Block class and using Reservation as it's super class -- that way, if you used super in intitialize, you'd only have to add on block: and block_key:

DateRange class
1. Reconsider DateRange as it's own class (has no independent behaviors)

Tighten up variable naming conventions
1. Make sure method naming conventions follow a pattern
2. Make sure method naming conventions logically build on one another IE reserve_from_block relies on update_block
3. Review and update variable names for clarity and replication
40 changes: 40 additions & 0 deletions test/date_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
require_relative "test_helper"

describe "DateRange" do

describe "contains" do
before do
@reservation = Hotel::Reservation.new(start_date: Date.new(2020, 5, 1), end_date: Date.new(2020, 5, 4), num_rooms: 1)
end

it "returns true if requested date is contained in reservation dates" do
expect(@reservation.contains(Date.new(2020, 5, 2))).must_equal true
end

it "returns true if requested date is first day in reservation dates" do
expect(@reservation.contains(Date.new(2020, 5, 1))).must_equal true
end

it "returns false if requested date is last day in reservation dates" do
expect(@reservation.contains(Date.new(2020, 5, 4))).must_equal false
end
end

describe "conflict?" do
before do
@reservation = Hotel::Reservation.new(start_date: Date.new(2020, 5, 1), end_date: Date.new(2020, 5, 4), num_rooms: 1)
end

it "returns true if requested dates are contained in current reservation dates" do
expect(@reservation.no_conflict?(Date.new(2020, 5, 2), Date.new(2020, 5, 4))).must_equal false
end

it "returns true if requested start date is the same as current end date" do
expect(@reservation.no_conflict?(Date.new(2020, 5, 4), Date.new(2020, 5, 5))).must_equal true
end

it "returns false if requested start date is earlier than current end date" do
expect(@reservation.no_conflict?(Date.new(2020, 5, 2), Date.new(2020, 5, 5))).must_equal false
end
end
end
Loading