forked from AdaGold/hotel
-
Notifications
You must be signed in to change notification settings - Fork 45
Jan Edrozo | Carets #30
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
Open
JNEdrozo
wants to merge
18
commits into
Ada-C8:master
Choose a base branch
from
JNEdrozo:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
7e666b7
room.rb and room_spec.rb drafted
JNEdrozo 6ddbc3e
Updated room cost to be float
JNEdrozo 4a22df2
reservation_spec and .rb arguments updated
JNEdrozo 3414dbc
Reservation: get_total_cost and initialize arguments count
JNEdrozo 78f5993
Added Guest
JNEdrozo 9fd74a9
comments update for reservation
JNEdrozo 71c37a3
reserve room method and test revision
JNEdrozo 32f99c1
reserve_room/specs and exceptions updated
JNEdrozo 9e54690
raised InvalidDateErrors for reservation
JNEdrozo 4c5a0d6
W2 rb and specs updated and reviewed
JNEdrozo 5d9d076
back up
JNEdrozo ffee567
added block_id with default value nil
JNEdrozo 9582bb3
reserve_block
JNEdrozo 5f29e40
reserve block and room revision
JNEdrozo 4e1fbc7
comments update
JNEdrozo 8e1219e
design-activity.md responses
JNEdrozo b6b3a0f
revised hotel_admin rb and spec to check for existing reservation ove…
JNEdrozo 0541e80
Revisiting Hotel Activity and decoupling changes updated
JNEdrozo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Binary file not shown.
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
| 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['specs/*_spec.rb'] | ||
| end | ||
|
|
||
| task default: :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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| # Hotel Revisited | ||
|
|
||
| ## Prompts: Responses | ||
|
|
||
| * What classes does each implementation include? Are the lists the same? | ||
|
|
||
| While both implementations A & B have the following classes: CartEntry, ShoppingCart, their list of classes are not the same. Implementation A's CartEntry class uses attr accessors for its unit_price and quantity, so its Order class can access those values when calculating the total price for each entry in the cart. Implementation B on the other hand does not use attr accessors and instead, has a price calculation method for each class such that: CartEntry has a price method to calculate each price per cart entry, the ShoppingCart has a price method to sum all cart entries in the cart (subtotal), and Order has a total_price method that adds SALES_TAX to the ShoppingCart's subtotal. | ||
|
|
||
| * Write down a sentence to describe each class. | ||
| * CartEntry defines the state (unit_price and quantity) and behavior (price in implementation B) of each cart entry requested. | ||
| * ShoppingCart defines the ShoppingCart object that's initialized as an array to store cart entries for one order. | ||
| * Order creates an order (initialized as an instance of a ShoppingCart) and handles total_price calculation (using the SALES_TAX). | ||
|
|
||
| * How do the classes relate to each other? It might be helpful to draw a diagram on a whiteboard or piece of paper. | ||
|
|
||
| The classes relate to each in that an instance of an Order is made up of an instance of a ShoppingCart (called cart), where the cart is an array that stores any number of CartEntry instances. | ||
|
|
||
| * What data does each class store? How (if at all) does this differ between the two implementations? | ||
| * What methods does each class have? How (if at all) does this differ between the two implementations? | ||
|
|
||
| * Implementation A: | ||
| CartEntry: unit_price and quantity (with attr accessors) | ||
| ShoppingCart: entries (array - with attr accessors) | ||
| Order: SALES_TAX, total_price method | ||
|
|
||
| * Implementation B: | ||
| CartEntry: unit_price, quantity, CartEntry price method | ||
| ShoppingCart: entries (array - no attr accessors), ShoppingCart subtotal price method | ||
| Order: SALES_TAX, total_price method | ||
|
|
||
| * Differences: | ||
| - Implementation A delegates all price calculation to the Order class, while B has some level of price calculated within each class. | ||
| - Implementation A makes use of attr accessors (interestingly rather than attr readers) and only uses one pricing method (total_price) in the Order class. Conversely, B does not us any attr accessors (I'm assuming to limit users ability to manipulate CartEntry data) and implements a price method for both CartEntry and ShoppingCart to access each objects' prices. | ||
|
|
||
| * Consider the Order#total_price method. In each implementation: | ||
| * Is logic to compute the price delegated to "lower level" classes like ShoppingCart and CartEntry, or is it retained in Order? | ||
| A does not delegate the price computation responsibility to "lower level" classes; it is all retained in Order. B does delegate price computation to ShoppingCart and CartEntry so that its Order class only deals with adding the SALES_TAX to the ShoppingCart price. | ||
|
|
||
| * Does total_price directly manipulate the instance variables of other classes? | ||
| A's total_price directly references the instance variables for the ShoppingCart (entries) and CartEntry (unit_price and quantity), but it takes their values and stores it in the variable sum. So in that way, while it is not directly changing the variables, there is potential to manipulate them because they are being accessed using attr accessors. B only references the ShoppingCart price (using the price method), so this implementation does not directly manipulate any instance variables. | ||
|
|
||
| * If we decide items are cheaper if bought in bulk, how would this change the code? Which implementation is easier to modify? | ||
|
|
||
| Implementation B would be easier to change because you could add additional code in CartEntry to account for cheaper cost when buying items in bulk. | ||
|
|
||
| * Which implementation better adheres to the single responsibility principle? | ||
|
|
||
| Implementation B appears to better adhere to the single responsibility principle because its classes (particularly Order and its total_price method) have less dependencies on its other classes. For example, it doesn't know/reference nearly as many objects and instance variables as A does in its total_price method. This is ideal because it reduces the potential for bugs and makes the code more adaptable. | ||
|
|
||
| Furthermore, before considering how we'd modify the code to consider bulk items, I thought A good in that it appeared DRY because it was consolidating and delegating all price calculations in Order (so you could just update price calculations all in one place), but now that I've had to also consider buying items in bulk and potential areas of price changes, it makes sense to break up the price calculation responsibilities. | ||
|
|
||
| * Bonus question once you've read Metz ch. 3: Which implementation is more loosely coupled? | ||
| Implementation B is more loosely coupled because its Order class does not reference and knows less about the other classes (e.g. it doesn't reference any CartEntry objects like A). | ||
|
|
||
|
|
||
| ## Revisiting Hotel Activity: | ||
|
|
||
| HotelAdmin is currently tightly coupled with my block and reservation classes. For example, I had date validation for reservations made in both my HotelAdmin and Reservation. I've changed it so now only my Reservation class handles date validation. Previously, I'd also planned for the HotelAdmin class to check if a block would conflict with an existing reservation before attempting to create the block. To decouple this area of logic, now the HotelAdmin class just creates a block while the block class performs the validation to decide if there is a conflict with an existing reservation. | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| ######## |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| module BookingSystem | ||
| class Guest | ||
|
|
||
| attr_reader :first_name, :last_name | ||
|
|
||
| def initialize(first_name, last_name) | ||
| raise ArgumentError.new("first name must be a string") if !first_name.is_a? String | ||
| @first_name = first_name | ||
|
|
||
| raise ArgumentError.new("last name must be a string") if !last_name.is_a? String | ||
| @last_name = last_name | ||
| end | ||
|
|
||
| end#of_Guest_class | ||
| end#of_module_BookingSystem |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| require_relative 'spec_helper' | ||
|
|
||
| describe "Guest" do | ||
| before do | ||
| @guest_test = BookingSystem::Guest.new("Jane", "Doe") | ||
| end | ||
|
|
||
| describe "initialize" do | ||
| it "can create an instance of Guess class" do | ||
| @guest_test.class.must_equal BookingSystem::Guest | ||
| end | ||
|
|
||
| it "can access a guest's first name and last name" do | ||
| @guest_test.first_name.must_equal "Jane" | ||
| @guest_test.last_name.must_equal "Doe" | ||
| end | ||
|
|
||
| it "ensures first name and last name are strings" do | ||
| proc { BookingSystem::Guest.new([], "Doe") }.must_raise ArgumentError | ||
| proc { BookingSystem::Guest.new("Jane", 2) }.must_raise ArgumentError | ||
| end | ||
|
|
||
| end | ||
|
|
||
| end#_of_"Guest" |
Empty file.
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| class UnavailableRoomError < StandardError | ||
| end | ||
|
|
||
| class InvalidDateRangeError < StandardError | ||
| end | ||
|
|
||
| class InvalidDateError <StandardError | ||
| end | ||
|
|
||
| class BlockError <StandardError | ||
| end |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
|
|
||
| require 'date' | ||
| require 'date_range' | ||
| require_relative 'BookingSystem_Errors' | ||
|
|
||
| module BookingSystem | ||
| class Block | ||
|
|
||
| attr_reader :block_id, :date_range, :rooms_array, :discount_room_rate | ||
|
|
||
| def initialize(block_id, date_range, rooms_array, discount_room_rate, reservation_list) | ||
|
|
||
| #Hotel Revisted Changes: Checks the reservation for any date range that would overlap with the block_id's date_range and throws an error if it detects an overlap | ||
| raise UnavailableRoomError.new("Room is unavailable") if reservation_list.any? {|reservation| | ||
| rooms_array.include?(reservation.room_id) && | ||
| #overlaps? same as: !(reservation.start_date >= date_range.last || reservation.end_date <= date_range.first) | ||
| reservation.date_range.overlaps?(date_range) && date_range.first < reservation.end_date | ||
| } | ||
|
|
||
| raise ArgumentError.new("Block ID must be a string") if !block_id.is_a? String | ||
| @block_id = block_id | ||
|
|
||
| raise InvalidDateRangeError.new("Date range must be a DateRange object") if !date_range.is_a? DateRange | ||
| @date_range = date_range | ||
|
|
||
| raise ArgumentError.new("Rooms collection must be an Array") if !rooms_array.is_a? Array | ||
| raise BlockError.new("Rooms array cannot have more than 5 rooms") if rooms_array.length > 5 | ||
| @rooms_array = rooms_array | ||
|
|
||
| raise ArgumentError.new("Room rate discount must be a float") if !discount_room_rate.is_a? Float | ||
| @discount_room_rate = discount_room_rate | ||
| end | ||
|
|
||
| def list_room_ids | ||
| room_ids = @rooms_array.map { |room| room.id } | ||
| return room_ids | ||
| end | ||
|
|
||
| end#of_Block_class | ||
| end#of_module_BookingSystem |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| require 'date' | ||
| require 'date_range' | ||
| require_relative 'room' | ||
| require_relative 'reservation' | ||
| require_relative 'BookingSystem_Errors' | ||
|
|
||
| module BookingSystem | ||
| class HotelAdmin | ||
|
|
||
| attr_reader :room_list, :reservation_list, :block_list | ||
|
|
||
| def initialize | ||
| #hotel admin knows all reservations (non-block and block) | ||
| @reservation_list = [] | ||
| @block_list = [] | ||
| #hotel admin knows all rooms in hotel | ||
| @room_list = BookingSystem::Room.all | ||
| end | ||
|
|
||
| def reserve_room(first_name, last_name, room_id, room_rate, start_date, end_date, block_id = nil) | ||
|
|
||
| requested_range = DateRange.new(start_date, end_date) | ||
|
|
||
| if block_id.nil? #if a room doesn't have a block_id, blacklist all rooms that are blocked | ||
| raise UnavailableRoomError.new("Room is unavailable") if @block_list.any? do |block| | ||
| block.date_range.overlaps?(requested_range) && block.rooms_array.any? { |room| room.id == room_id } | ||
| end | ||
| end | ||
|
|
||
| raise UnavailableRoomError.new("Room is unavailable") if @reservation_list.any? {|reservation| | ||
| reservation.room_id == room_id && reservation.date_range.overlaps?(requested_range) && start_date < reservation.end_date | ||
| #overlaps? same as: | ||
| # reservation.room_id == room_id && | ||
| # !(reservation.start_date >= end_date || reservation.end_date <= start_date) | ||
| } | ||
|
|
||
| reservation = BookingSystem::Reservation.new(first_name, last_name, room_id, room_rate, start_date, end_date, block_id) | ||
| @reservation_list << reservation | ||
| end | ||
|
|
||
| # Hotel Revisted Changes | ||
| def reserve_block(block_id, date_range, rooms_array, discount_room_rate, reservation_list) | ||
|
|
||
| if @block_list.any? { |block| block.date_range.overlaps?(date_range) } | ||
| @block_list.each do |block| | ||
| rooms_array.each do |room| | ||
| if block.list_room_ids.include?(room.id) | ||
| raise UnavailableRoomError.new("Room is unavailable") | ||
| end | ||
| end | ||
| end | ||
| end | ||
|
|
||
| block = BookingSystem::Block.new(block_id, date_range, rooms_array, discount_room_rate, reservation_list) | ||
| @block_list << block | ||
| end | ||
|
|
||
| def find_reservations_by_date(specific_date) | ||
| reservations_on_date = [] | ||
| @reservation_list.each do |reservation| | ||
| if reservation.date_range.include?(specific_date) | ||
| reservations_on_date << reservation | ||
| end | ||
| end | ||
|
|
||
| return reservations_on_date | ||
| end | ||
|
|
||
| def rooms_available_for_date_range(date_range) | ||
|
|
||
| raise InvalidDateRangeError.new("Range must be a DateRange object") if !date_range.is_a? DateRange | ||
|
|
||
| unavailable_room_ids = [] | ||
|
|
||
| @reservation_list.each do |reservation| | ||
| if reservation.date_range.overlaps?(date_range) | ||
| unavailable_room_ids << reservation.room_id | ||
| end | ||
| end | ||
|
|
||
| available_rooms = @room_list.reject { |room| unavailable_room_ids.include?(room.id) } | ||
|
|
||
| return available_rooms | ||
| end | ||
|
|
||
| end#of_HotelAdmin_class | ||
| end#of_module_BookingSystem | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| # REFACTORED METHODS: | ||
| # def rooms_available_for_date_range(date_range) | ||
| # raise InvalidDateRangeError.new("Range must be a DateRange object") if !date_range.is_a? DateRange | ||
| # | ||
| # unavailable_room_ids = [] | ||
| # | ||
| # @reservation_list.each do |reservation| | ||
| # if reservation.date_range.include?(date_range) | ||
| # unavailable_room_ids << reservation.room_id | ||
| # end | ||
| # end | ||
| # | ||
| # available_rooms = [] | ||
| # | ||
| # @room_list.each do |room| | ||
| # if unavailable_room_ids.include?(room.id) == false | ||
| # available_rooms << room | ||
| # end | ||
| # end | ||
| # | ||
| # return available_rooms | ||
| # end | ||
|
|
||
|
|
||
|
|
||
|
|
||
| =begin | ||
| Date notes: | ||
|
|
||
| < | ||
| source: https://stackoverflow.com/questions/3296539/comparision-of-date-in-ruby | ||
|
|
||
| .between?(start_date, end_date) | ||
| source: https://stackoverflow.com/questions/4521921/how-to-know-if-todays-date-is-in-a-date-range | ||
| =end | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| require 'date' | ||
| require 'date_range' | ||
|
|
||
| module BookingSystem | ||
| class Reservation | ||
| attr_reader :first_name, :last_name, :room_id, :room_rate, :start_date, :end_date, :total_cost, :date_range | ||
|
|
||
| def initialize(first_name, last_name, room_id, room_rate, start_date, end_date, block_id = nil) | ||
|
|
||
| @first_name = first_name | ||
| @last_name = last_name | ||
|
|
||
| @room_id = room_id | ||
| @room_rate = room_rate | ||
|
|
||
| raise InvalidDateError.new("Start date is not a valid date object") if !start_date.is_a? Date | ||
| @start_date = start_date | ||
|
|
||
| raise InvalidDateError.new("End date is not a valid date object") if !end_date.is_a? Date | ||
| @end_date = end_date | ||
|
|
||
| @date_range = get_date_range | ||
| @total_cost = get_total_cost | ||
| end | ||
|
|
||
| def get_total_cost | ||
| date_diff = @end_date - @start_date | ||
| grand_total = date_diff * @room_rate | ||
| return grand_total | ||
| end | ||
|
|
||
| def get_date_range | ||
| raise InvalidDateRangeError.new("Invalid date range") if @end_date <= @start_date #if end_date is older or equal to start_date, raise error | ||
| range = DateRange.new(@start_date, @end_date) | ||
| return range | ||
| end | ||
|
|
||
| end#of_Reservation_class | ||
| end#of_module_BookingSystem | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| =begin | ||
| NOTES: | ||
| #DATE RANGE METHOD (VERSION1) | ||
| # def make_date_range_array | ||
| # return (@start_date .. @end_date).map{|day| day} | ||
| # end | ||
| #Modify: @date_range = make_date_range_array | ||
|
|
||
| #OUTPUTS: an array of the date range | ||
| =end |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This couples
Block (and similarlyReservation`) very tightly. Instead you could create a method in Reservation & Block that checks to see if it contains the room/date combo.