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

Lindsey Deuel - Pipes - Hotel #24

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
a3521bf
setting up classes and spec files
VirtualLindsey Sep 6, 2017
cbcc6fd
pushing work
VirtualLindsey Sep 7, 2017
8060426
saving work
VirtualLindsey Sep 7, 2017
5fe51fe
adding blocks of rooms
VirtualLindsey Sep 7, 2017
9d73ba1
forgot to save hotel
VirtualLindsey Sep 7, 2017
9fef4c3
adding the skeleton of the other classes
VirtualLindsey Sep 7, 2017
6cd989d
adding the reserve_room function with useof dateRange
VirtualLindsey Sep 7, 2017
38f2ad9
adding method for get all rooms
VirtualLindsey Sep 7, 2017
9424f30
adding method for getting all open rooms given a date range
VirtualLindsey Sep 7, 2017
068f91f
saving work
VirtualLindsey Sep 7, 2017
98ab841
pushing current work of wave 3
VirtualLindsey Sep 8, 2017
098a9cd
adding method to check if block has availablities
VirtualLindsey Sep 8, 2017
fae2b03
pushing method for reserving a room from a block. Moving onto testing…
VirtualLindsey Sep 8, 2017
16418fc
fixing logic for blocks and reservations
VirtualLindsey Sep 8, 2017
05e7f37
fixing more block logic
VirtualLindsey Sep 8, 2017
d499a87
updating logic and doing testing of methods
VirtualLindsey Sep 8, 2017
775cbd0
fixing logic and adding tests
VirtualLindsey Sep 8, 2017
fe80a6c
adding tests for reservation
VirtualLindsey Sep 8, 2017
92e9aa6
adding more tests for reservation
VirtualLindsey Sep 8, 2017
560ab87
adding tests for room
VirtualLindsey Sep 8, 2017
947684e
updating tests for date range
VirtualLindsey Sep 8, 2017
2b041a6
adding tests for blocks
VirtualLindsey Sep 8, 2017
52cecc9
saving tests for hotel_spec
VirtualLindsey Sep 8, 2017
0632aa2
adding more tests for hotel
VirtualLindsey Sep 8, 2017
4b71bf8
adding comments and finishing up wave 3
VirtualLindsey Sep 8, 2017
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
Binary file added .DS_Store
Binary file not shown.
9 changes: 9 additions & 0 deletions Rakefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
require 'rake/testtask'

Rake::TestTask.new do |t|
t.libs = ["lib"]
t.warning = true
t.test_files = FileList['spec/*_spec.rb']
end

task default: :test
17 changes: 17 additions & 0 deletions lib/block.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
require_relative 'room'
module Hotels
class Block_Of_Rooms < Room
Copy link

Choose a reason for hiding this comment

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

In general with Ruby we name classes using CamelCase rather than (capitalized) Snake_Case.

attr_reader :available_rooms, :date, :room_booked, :id

def initialize(rooms, date, id)
@available_rooms = rooms
@room_booked = []
@id = id
@available_rooms.length.times do |i|
@room_booked << false
end
@date = date
@cost = 175
end
end
end
14 changes: 14 additions & 0 deletions lib/daterange.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
require 'date'

class DateRange
attr_reader :start_date, :end_date

def initialize(start_date, end_date)
if start_date > end_date
raise ArgumentError, "The start date must come before the end_date"
else
@start_date = start_date
@end_date = end_date
end
end
end
184 changes: 184 additions & 0 deletions lib/hotel.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
require_relative 'room'
require_relative 'reservation'
require_relative 'daterange'
require_relative 'block'
require 'date'

module Hotels
class Hotel
attr_reader :rooms, :reservations_by_room, :reservation_by_date, :room_blocks

def initialize
#this array will hold room objects for each room in the hotel
@rooms = []
#this is a hash that maps room numbers to reservations for that room
@reservations_by_room = {}
#this is a hash that maps dates to reservations on that date
@reservations_by_date = {}
#this array holds all of the blocks
@room_blocks = []
#this array holds all of the rooms that are currently a part of some block
@rooms_currently_in_block = []
Copy link

Choose a reason for hiding this comment

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

We should avoid creating several instance variables that are essentially re-arrangements of the same data into different data structures.

For example, @reservations_by_room and @reservations_by_date both include the same data (all of the reservations) they just expose that data in a different structure. Instead we could keep a single @reservations array and create two methods, reservations_by_room and reservations_by_date. This would be pretty straight forward with the Enumberable#group_by method:

def reservations_by_room
  by_room = Hash.new([]) # this hash returns an empty array for any keys that don't exist, instead of nil
  by_room.merge(@reservations.group_by(&:room_number))
end

def reservations_by_date
  @reservations.group_by(&:start_date)
end

Similarly @rooms_currently_in_block could be replaced with a (private) method:

def rooms_currently_in_block
  room_blocks.map(&:available_rooms).flatten
end

The primary reason for doing this is that we then do not need to ensure that our code keeps all of these related data structures synchronized whenever the underlying dataset changes. A significant source of the complication in some of your Hotel methods comes from making sure that all of these variables are updated appropriately.

Keeping these correlated data structures also means that we need to write a lot more tests as we need to check that all of them maintain their invariants whenever one of them changes.

20.times do |i|
@rooms << Hotels::Room.new(i)
@reservations_by_room[i] = []
end
end

def reserve_room(room_number, date)
#this will check if the given room number is a memeber of any block
if @rooms_currently_in_block.include?(room_number)
#iterate through each block
@room_blocks.each do |current_block|
#iterate through this blocks room variable
current_block.rooms.each do |current_room_in_block|
Copy link

Choose a reason for hiding this comment

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

While I don't think we should concern ourselves with performance-based decision making at Ada, because all of the data sets we're using are small enough to have good performance regardless, this is a place where I might make a different choice.

Specifically, I would first filter all of the blocks to only have the set which overlaps the given date, and afterwards filter that further by looking any of those blocks includes the given room number.

My reasoning here is that over time a Hotel is much more likely to have more block reservations which include a given room than block reservations overlapping a particular date. So we should use the more strict filter first.

#if this room number matches the room number in question
if current_room_in_block.room_number == room_number
#checks if the start dates and end dates conflict,
#raises errors accordingly
if date.start_date > current_block.start_date &&
date.start_date < current_block.end_date
raise ArgumentException, "This room is part of a block. Change your start date"
elsif date.end_date > current_block.start_date
raise ArgumentException, "This room is part of a block. Change your end date"
end
end
end
end
end

#pulls all array of all reservations of a given room
current_reservations = @reservations_by_room[room_number]
current_reservations.each do |i|
#if the requested start date is before the current reservations start date
if date.start_date < i.start_date
#checks to see if the end date is also before the start date
if (date.end_date - i.start_date).to_i > 0
raise ArgumentError, "This room is booked during the dates requested"
end
#if the start dates are the same, or the start date is less than the end date
elsif date.start_date == i.start_date || (date.start_date - i.end_date).to_i < 0
Copy link

Choose a reason for hiding this comment

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

This date checking code, which is related to the date checking code in the block reservation code above should be abstracted out from this class and put in either Reservation or (ideally) DateRange.

These are the only places in our codebase that do this date checking, so we don't get much DRY benefit from moving them. However, we would increase the readability of this already very long method substantially.

raise ArgumentError, "This room is booked during the dates requested"
Copy link

Choose a reason for hiding this comment

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

This would be a good place for a custom error class.

end
end

#makes new reservation
new_reservation = Hotels::Reservation.new(room_number, date)
@reservations_by_room[room_number] << new_reservation

#if this day already has a reservation
if @reservations_by_date.key?(date.start_date)
#add it to the array
@reservations_by_date[date.start_date] << new_reservation
else
#other wise make a new array and add it
@reservations_by_date[date.start_date] = [new_reservation]
end

end


def get_all_rooms
Copy link

Choose a reason for hiding this comment

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

This method is exactly equivalent to the method that attr_reader :rooms gets us, so we can safely remove this (and change any references to it to rooms instead).

return @rooms
end

def get_open_rooms(date)
open_rooms = []
#makes a boolean array flagging all rooms as available
20.times do |i|
open_rooms[i] = true
Copy link

Choose a reason for hiding this comment

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

There are a few places in the codebase where we are using this technique of creating an array of booleans that maps to an array of other values, then we loop through the values and set the booleans according to some conditional checks, and finally we create a new array from all of the values which are now mapped to a true boolean.

This is a pretty common pattern in programming, generally referred to filtering, and Ruby's Enumerable class has a method for it: Enumerable#select.

We could reduce the code needed for this method by using it thusly:

def get_open_rooms(date)
  available_rooms = reservations_by_date.values.flatten.select do |res|
    # This should be a valid check for overlapping date ranges
    (res.start_date <= date.start_date && res.end_date > date.start_date) ||
    (date.start_date <= res.start_date && date.end_date > res.start_date)
  end.map(&:room_number)

  return available_rooms
end

As per my other comments, the above implementation could be simplified further by using a single @reservations array, and by implementing the date check as something like DateRange#overlap?.

end

current_date = date.start_date
while current_date < date.end_date do
#if the current_date has a reservation
if @reservations_by_date.key?(current_date)
current_reservations = @reservations_by_date[current_date]
#iterate over each reservation
current_reservations.each do |this_reservation|
#set value of the boolean array (where the index == room number) to false
open_rooms[this_reservation.room_number] = false
end
end
current_date += 1
end

#this will hold the actual avaialble rooms
available_rooms = []
index = 0

open_rooms.each do |value|
#if the value of the boolean array is true
if value == true
#append the room at the index to the available_rooms array
available_rooms << @rooms[index]
end
index += 1
end

return available_rooms
end

def make_new_block(number_of_rooms, date, id)
available_rooms = get_open_rooms(date)
if number_of_rooms > 5
raise ArgumentException, "Room blocks must be 5 rooms or less"
elsif number_of_rooms > available_rooms.length
raise ArgumentException, "There are only #{available_rooms.length} available for the dates selected"
Copy link

Choose a reason for hiding this comment

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

Because both of these first two cases end in a raise statement we can assume that if either of them are hit we won't continue running the rest of this method. This means we don't need to contain the actual "meat" of the method's logic inside of an else block.

else
rooms = []
number_of_rooms.times do |i|
rooms << available_rooms[i]
@rooms_currently_in_block << available_rooms[i]
end
new_block = Hotels::Block_Of_Rooms.new(rooms, date, id)
@room_blocks << new_block
end
end

#just looks for at least 1 avaialble room in a given block
def check_block_for_availablity(block_id)
@room_blocks.each do |block|
if block.id == block_id
block.room_booked.each do |is_room_booked|
if is_room_booked == false
return true
end
end
end
end

return false
end

def reserve_room_from_block(block_id)
if @room_blocks == []
raise ArgumentError, "There are no current room blocks"
else
@room_blocks.each do |current_block|
if current_block.id == block_id
if check_block_for_availablity(block_id)
room = get_open_room_from_block(current_block)
new_reservation = Hotels::Reservation.new(room, current_block.date, 175)
else
puts "There are no rooms avaiable for this block"
Copy link

Choose a reason for hiding this comment

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

This should be raising an error of some kind (probably a custom one), instead of printing to the screen with puts.

end
end
end
end
end

#private helper method to return an open room from a given block
def get_open_room_from_block(block)
rooms = block.room_booked
rooms.each_with_index do |value, index|
if value == false
block.room_booked[index] = true
Copy link

Choose a reason for hiding this comment

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

We should avoid doing something like this as much as possible. Specifically what we're doing here is accessing a "read-only" attribute from the Block_Of_Rooms class (room_booked) and then modifying it.

Technically Ruby lets us modify the room_booked array because all arrays in Ruby can always be modified, but the intent implied by attr_reader :room_booked is that code from outside of the Book_Of_Rooms class will not be changing it. Basically this represents a case of very tight coupling between these two classes, and breaks the principle of encapsulation that is fundamental to Object Oriented Programming.

Instead we should add a method to Block_Of_Rooms which takes the index variable and internally updates the room_booked array. That way, if we need to refactor Block_Of_Rooms in the future, possibly to not use an array for room_booked at all, our code here will not break.

return block.available_rooms[index]
end
end
end

private :get_open_room_from_block
end
end
23 changes: 23 additions & 0 deletions lib/reservation.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
module Hotels
class Reservation
attr_reader :room_number, :start_date, :end_date, :cost

def initialize(room_number, date, cost=nil)
@room_number = room_number
@start_date = date.start_date
@end_date = date.end_date

if cost == nil
cost = 200
else
cost = 175
end

if date.start_date == date.end_date
@cost = cost
else
@cost = cost * (@end_date - @start_date).to_i
end
end
end
end
8 changes: 8 additions & 0 deletions lib/room.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
module Hotels
class Room
attr_reader :room_number
def initialize(room_number)
@room_number = room_number
end
end
end
28 changes: 28 additions & 0 deletions specs/block_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
require_relative 'spec_helper'
require_relative '../lib/hotel'

describe "Blocks" do
describe "initialize" do
it "checks initialize" do
start_date = Date.new(2017,1,1)
end_date = Date.new(2017,1,3)
rooms = [1,2]
date = DateRange.new(start_date, end_date)
block = Hotels::Block_Of_Rooms.new(rooms, date, 1)

block.must_respond_to :available_rooms
block.available_rooms.must_equal rooms
block.available_rooms.must_be_kind_of Array

block.must_respond_to :date
block.date.must_equal date
block.date.must_be_kind_of DateRange

block.must_respond_to :room_booked
block.room_booked.must_equal [false, false]

block.must_respond_to :id
block.id.must_equal 1
end
end
end
5 changes: 5 additions & 0 deletions specs/coverage/.last_run.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"result": {
"covered_percent": 100.0
}
}
7 changes: 7 additions & 0 deletions specs/coverage/.resultset.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"RSpec": {
"coverage": {
},
"timestamp": 1504853574
}
}
Empty file.
Loading