Skip to content
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

Sockets-Chantal #29

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open
50 changes: 50 additions & 0 deletions lib/hotel.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
require 'set'

require_relative 'reservation'

class Hotel
NUM_ROOMS = 20

Choose a reason for hiding this comment

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

Should this class be in a module?


# rooms 1-20, reservations
def initialize()
@reservations = []
end

# Access the list of all of the rooms in the hotel.
def get_rooms
(1..NUM_ROOMS).to_set
end

def make_reservation(start_date, end_date)
# Validate the date range.
validate_date_range(start_date, end_date)
# Find room that's available.
available_rooms = find_available_rooms(start_date, end_date)
raise RuntimeError.new("there are no rooms available") if available_rooms.empty?
room_number = available_rooms.to_a.sample

Choose a reason for hiding this comment

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

Good call raising an error here - that seems like an appropriate way to indicate that the method can't continue.

Instead of raising a RuntimeError, best practice is to define your own appropriately named exception type that inherits from StandardError and raise that. The reason is that RuntimeError is very broad, and doesn't give much information about what went wrong. Many things could produce a RuntimeError, but a NoRoomsAvailableError tells the caller exactly what went wrong.

# Create reservation.
reservation = Reservation.new(room_number, start_date, end_date)
# Store the reservation.
@reservations << reservation
# Return the reservation.
return reservation
end

def find_available_rooms(start_date, end_date)
available_rooms = get_rooms
@reservations.each do |reservation|
if reservation.start_date < end_date && reservation.end_date > start_date
available_rooms.delete(reservation.room_number)
end

Choose a reason for hiding this comment

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

Here, Hotel reaches into the Reservation and looks at its data directly. This is both an example of tight coupling, and violation of the law of demeter (the two tend to go hand in hand).

A cleaner approach might be to define a method Reservation#overlap?(start_date, end_date) that contains the logic on line 36, and call that here:

@reservations.each do |reservation|
  if reservation.overlap?(start_date, end_date)
    available_rooms.delete(reservation.room_number)
  end
end

I know we hadn't read POODR ch 4 yet when you wrote this code, but this is what Metz is talking about when she says you should "ask for what instead of telling how".

end
return available_rooms
end

def validate_date_range(start_date, end_date)
raise ArgumentError.new("the end date is before start date") unless end_date >= start_date
end

def get_reservations_inclusive(date)
@reservations.find_all {|reservation| reservation.end_date >= date && reservation.start_date <= date}
end
end
18 changes: 18 additions & 0 deletions lib/reservation.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
class Reservation
attr_reader :room_number, :start_date, :end_date

ROOM_PRICE = 200

#room association, date range
def initialize(room_number, start_date, end_date)
@room_number = room_number #integer
@start_date = start_date #time
@end_date = end_date #time
end

def calculate_total_cost()
total_nights = @end_date - @start_date #probably convert to integer after test
total_nights = total_nights.to_i / (24 * 60 * 60)
ROOM_PRICE * total_nights

Choose a reason for hiding this comment

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

I like the decision to make this a method rather than storing the cost as an instance variable. That way if the number of nights or the cost per night were to change (say in a future version of the program), the total cost will automatically be correct.

end
end
80 changes: 80 additions & 0 deletions spec/hotel_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
require_relative 'spec_helper'

describe "hotel" do
NUM_BOOKED_ROOMS = Hotel::NUM_ROOMS - 5

before do
@seventh = Time.parse("2018-07-07")
@eleventh = Time.parse("2018-07-11")
end

let(:empty_hotel) do
empty_hotel = Hotel.new
end

let(:booked_hotel) do
booked_hotel = Hotel.new
(NUM_BOOKED_ROOMS).times do
booked_hotel.make_reservation(@seventh, @eleventh)

Choose a reason for hiding this comment

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

I really like this organization - having all of an empty, a mostly-booked and a fully-booked hotel available for your tests up front makes the process of testing much simpler.

end
booked_hotel
end

let(:fully_booked_hotel) do
fully_booked_hotel = Hotel.new
Hotel::NUM_ROOMS.times do
fully_booked_hotel.make_reservation(@seventh, @eleventh)
end
fully_booked_hotel
end

it "get rooms" do
rooms = empty_hotel.get_rooms
expect(rooms.length).must_equal Hotel::NUM_ROOMS
end

describe "makes reservation" do
it "make reservation for hotel with no reservations" do
reservation = empty_hotel.make_reservation(@seventh, @eleventh)
end

it "make reservation for hotel with several reservations" do
reservation = booked_hotel.make_reservation(@seventh, @eleventh)
end

it "make reservation for fully booked hotel" do
expect{fully_booked_hotel.make_reservation(@seventh, @eleventh)}.must_raise RuntimeError
end

Choose a reason for hiding this comment

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

There are a number of questions around when specifically a room is available that these tests don't cover. In particular, if a hotel is fully booked for one range of dates and you get another request...

No room is available if the dates:

  • Same dates
  • Overlaps in the front
  • Overlaps in the back
  • Completely contained
  • Completely containing

The room is available if the dates:

  • Completely before
  • Completely after
  • Ends on the checkin date
  • Starts on the checkout date


it "make reservation with invalid dates" do
expect{fully_booked_hotel.make_reservation(@eleventh, @seventh)}.must_raise ArgumentError
end
end

describe "finds available rooms" do
it "find available rooms when some are available" do
available_rooms = booked_hotel.find_available_rooms(@seventh, @eleventh)
expect(available_rooms.length).must_equal Hotel::NUM_ROOMS - NUM_BOOKED_ROOMS
end

it "find available rooms when none are available" do
available_rooms = fully_booked_hotel.find_available_rooms(@seventh, @eleventh)
expect(available_rooms.empty?).must_equal true
end
end

it "get reservations inclusive" do
inclusive_reservations = booked_hotel.get_reservations_inclusive(@eleventh)
expect(inclusive_reservations.length).must_equal NUM_BOOKED_ROOMS
end

describe 'validate date range' do
it 'invalid date range throws exception' do
expect{empty_hotel.validate_date_range(@eleventh, @seventh)}.must_raise ArgumentError
end

it "valid date range throws no exception" do
empty_hotel.validate_date_range(@seventh, @eleventh)
end
end
end
8 changes: 8 additions & 0 deletions spec/reservation_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
require_relative 'spec_helper'

describe "reservation" do
it 'calculate_total_cost' do
reservation = Reservation.new(2, Time.parse("2018-07-23"), Time.parse("2018-07-28"))
expect(reservation.calculate_total_cost).must_equal 1000
end
end
4 changes: 4 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,7 @@
Minitest::Reporters.use! Minitest::Reporters::SpecReporter.new

# Require_relative your lib files here!
require 'time'

require_relative '../lib/hotel'
require_relative '../lib/reservation'