-
Notifications
You must be signed in to change notification settings - Fork 46
Lindsay - Edges - hotel #44
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
base: master
Are you sure you want to change the base?
Changes from all commits
553e814
bfeac50
de1cc7b
979b2ec
3b4ce06
2cfc494
b0848aa
e9b8035
93c9f0b
bf5738f
8443257
834953a
b68d746
1d6cc99
d040ddd
e4ea61b
6365948
bfb8894
cfd2ced
6dbe8c9
4a61fd4
89e4c38
43eda8a
528cb45
a3a4a42
8792c98
276c23e
a9ecf7b
18c4d15
8af6d6a
d29022f
592640d
df9a296
c93c962
e6ac0b5
b40bea9
97d7df6
ef9e469
7263bb0
6e340c4
988f233
2f92016
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,3 +48,4 @@ build-iPhoneSimulator/ | |
|
|
||
| # unless supporting rvm < 1.11.0 or doing something fancy, ignore this: | ||
| .rvmrc | ||
| coverage | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| ## Hotel Design Activity | ||
|
|
||
| **What classes does each implementation include? Are the lists the same?** | ||
|
|
||
| Both implementations have 3 classes by the same names: CartEntry, ShoppingCart, Order. | ||
|
|
||
|
|
||
| **Write down a sentence to describe each class.** | ||
|
|
||
| CartEntry represents the addition of a given product to a cart, including its price and quantity. | ||
|
|
||
| ShoppingCart represents a collection of everything that has been added to a cart. | ||
|
|
||
| Order represents the total cost of everything in a given cart, including 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.** | ||
|
|
||
| ShoppingCart contains each CartEntry. Order calculates the total of the ShoppingCart after sales tax. | ||
|
|
||
|
|
||
| **What data does each class store? How (if at all) does this differ between the two implementations?** | ||
|
|
||
| CartEntry: both implementations store the prices and quantities of a given product. In implementation A, those values can be accessed outside of the class due to the attr_accessor whereas in Implementation B, they have to be used locally. | ||
|
|
||
| ShoppingCart: both implementations have an array that stores each CartEntry object. In Implementation A, those entries can be accessed outside of the class. Implementation B also has a value for the sum of all entries in the cart. | ||
|
|
||
| Order: both implementations store the sum/subtotal, or total price of a complete order (before sales tax). This is calculated differently in each implementation. Implementation A iterates through each price and quantity in @entries. Implementation B calls the #price method from ShoppingCart. | ||
|
|
||
|
|
||
| **What methods does each class have? How (if at all) does this differ between the two implementations?** | ||
|
|
||
| CartEntry: both implementations have an initialize method. Implementation B also has a price method that returns the subtotal of one particular item. | ||
|
|
||
| ShoppingCart: both have an initialize method. | ||
|
|
||
| Order: both have an initialize method that creates a new instance of a ShoppingCart. Both also have a total_price method that returns the total of all cart entries plus sales tax. The difference is that the method in Implementation A handles all of the logic for "sum" whereas Implementation B calls the "price" method in the ShoppingCart class. | ||
|
|
||
|
|
||
| **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?** | ||
|
|
||
| In Implementation A, the logic is retained in Order. In Implementation B, the logic relies on ShoppingCart, which is a lower level class. | ||
|
|
||
|
|
||
| **Does total_price directly manipulate the instance variables of other classes?** | ||
|
|
||
| In Implementation A, total_price reads @entries from ShoppingCart and @unit_price and @quantity from CartEntry. In Implementation B, total_price does not read instance variables of any other class. | ||
|
|
||
|
|
||
| **If we decide items are cheaper if bought in bulk, how would this change the code? Which implementation is easier to modify?** | ||
|
|
||
| You would need to add some conditional logic that lowers the :unit_price based on :quantity. Implementation A is harder to modify because that logic would need to be called into account in the CartEntry class and Order class. Implementation B is easier to modify because you would only need to modify the CartEntry class. | ||
|
|
||
|
|
||
| **Which implementation better adheres to the single responsibility principle?** | ||
|
|
||
| Implementation B. | ||
|
|
||
|
|
||
| **Bonus question once you've read Metz ch. 3: Which implementation is more loosely coupled?** | ||
|
|
||
| Implementation B. | ||
|
|
||
|
|
||
| **changes to hotel** | ||
| I had a lot of logic regarding the date within reservation_hub. I can create a separate DateRange class to validate dates and create a date array. It would make my code better because I'd be able to call the DateRange class when booking both, room blocks and reservations. It would not only dry up my code, but also help me better adhere to single responsibility. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| require 'date' | ||
|
|
||
|
|
||
| module Hotel | ||
| class DateRange | ||
|
|
||
| def initialize(start_date, end_date) | ||
|
|
||
| @start_date = start_date | ||
| @end_date = end_date | ||
|
|
||
| validate_dates | ||
|
|
||
| @date_range = create_date_array(start_date, end_date) | ||
| end | ||
|
|
||
|
|
||
| def validate_dates | ||
| raise ArgumentError.new("The end date must be after the start date") if @end_date <= @start_date | ||
| end | ||
|
|
||
|
|
||
| def create_date_array(start_date, end_date) | ||
| number_of_nights = (end_date - start_date).to_i | ||
| date_array = [] | ||
| number_of_nights.times do | ||
| date_array << start_date | ||
| start_date +=1 | ||
| end | ||
| return date_array | ||
| end | ||
|
|
||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
|
|
||
|
|
||
| module Hotel | ||
| class Reservation | ||
| attr_reader :room_id, :reservation_dates, :total_cost | ||
|
|
||
| def initialize(reservation_dates, room_id) | ||
|
|
||
| @reservation_dates = reservation_dates | ||
| @room_id = room_id | ||
| @total_cost = reservation_cost(reservation_dates) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than calculating the total cost up front and storing it in an instance variable, I would leave this as a behavior, a method is called whenever someone wants that information. That way if the number of nights or the cost changes, the total cost will automatically be correct - it will "just work". |
||
|
|
||
| end | ||
|
|
||
| def reservation_cost(reservation_dates) | ||
| nightly_cost = 200 | ||
| total_days = (reservation_dates.length).to_i | ||
| total_cost = total_days * nightly_cost | ||
| end | ||
|
|
||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| require 'date' | ||
| require 'pry' | ||
|
|
||
| require_relative 'reservation' | ||
| require_relative 'room_block' | ||
| require_relative 'date_range' | ||
|
|
||
|
|
||
| module Hotel | ||
| class ReservationHub | ||
| class NoRoomsAvailableError < StandardError; end | ||
|
|
||
| attr_reader :reservations, :room_bookings, :room_blocks | ||
|
|
||
|
|
||
| def initialize | ||
| @reservations = [] | ||
| @room_bookings = {1 => [], 2 => [], 3 => [], 4 => [], 5 => [], 6 => [], 7 => [], 8 => [], 9 => [], 10 => [], 11 => [], 12 => [], 13 => [], 14 => [], 15 => [], 16 => [], 17 => [], 18 => [], 19 => [], 20 => []} | ||
| @room_blocks = [] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would probably build the room booking list in a loop rather than hardcoding it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want to call out the way you keep track of which rooms are reserved on which dates. This strategy definitely works! However, it ends up duplicating information between this file and Another option is to have the |
||
| end | ||
|
|
||
|
|
||
| def add_reservation(start_date, end_date) | ||
|
|
||
| dates = DateRange.new(start_date, end_date) | ||
| reservation_dates = dates.create_date_array(start_date, end_date) | ||
|
|
||
| room_id = assign_room(reservation_dates) | ||
|
|
||
| reservation = Reservation.new(reservation_dates, room_id) | ||
|
|
||
| @reservations << reservation | ||
|
|
||
| return reservation | ||
| end | ||
|
|
||
|
|
||
| def add_room_block(start_date, end_date, total_rooms) | ||
|
|
||
| dates = DateRange.new(start_date, end_date) | ||
| reservation_dates = dates.create_date_array(start_date, end_date) | ||
|
|
||
| room_ids = [] | ||
|
|
||
| total_rooms.times do | ||
| room_id = assign_room(reservation_dates) | ||
| room_ids << room_id | ||
| end | ||
|
|
||
| block_id = @room_blocks.length + 1 | ||
|
|
||
| room_block = RoomBlock.new(reservation_dates, room_ids, block_id) | ||
|
|
||
| @room_blocks << room_block | ||
|
|
||
| return room_block | ||
| end | ||
|
|
||
|
|
||
| def check_available_rooms(reservation_dates) | ||
|
|
||
| available_rooms = [] | ||
|
|
||
| @room_bookings.each do |room, dates| | ||
| overlap = reservation_dates & dates | ||
|
|
||
| if overlap.length == 0 | ||
| available_rooms << room | ||
| end | ||
|
|
||
| end | ||
| return available_rooms | ||
| end | ||
|
|
||
|
|
||
| def assign_room(reservation_dates) | ||
|
|
||
| available_rooms = check_available_rooms(reservation_dates) | ||
|
|
||
| if available_rooms == nil | ||
| raise NoRoomsAvailableError, "No rooms are available." | ||
| end | ||
|
|
||
| room_id = available_rooms[0] | ||
|
|
||
| reservation_dates.each{|date| @room_bookings[room_id] << date} | ||
|
|
||
| return room_id | ||
| end | ||
|
|
||
|
|
||
| def find_reservations(date) | ||
|
|
||
| reservations_by_date = [] | ||
| index = 0 | ||
|
|
||
| @reservations.each do | ||
|
|
||
| if @reservations[index].reservation_dates.include?(date) | ||
| reservations_by_date << @reservations[index] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of keeping a manual index here, you should use a block parameter: @reservations.each do |reservation|
if reservation.reservation_dates.include?(date)
reservations_by_date << reservation
end
endOr better yet, notice that we're using the def find_reservations(date)
return @reservations.select do |reservation|
reservation.reservation_dates.include?(date)
end
end |
||
| end | ||
| index +=1 | ||
|
|
||
| end | ||
| return reservations_by_date | ||
| end | ||
|
|
||
|
|
||
| def find_room_block(id) | ||
|
|
||
| @room_blocks.each do |room_block| | ||
| if room_block.block_id == id | ||
| return room_block | ||
| end | ||
| end | ||
| end | ||
|
|
||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
|
|
||
|
|
||
| module Hotel | ||
| class RoomBlock | ||
| attr_reader :reservation_dates, :room_ids, :block_id | ||
|
|
||
| def initialize(reservation_dates, room_ids, block_id) | ||
|
|
||
| @reservation_dates = reservation_dates | ||
| @room_ids = room_ids | ||
| @block_id = block_id | ||
|
|
||
| end | ||
|
|
||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| **the name "room_bookings" seems a bit misleading. I used it to include both, reservations and room blocks for each room. For each room, I probably would have been better off creating a nested hash. One key/value pair would be confirmed_reservations => [] and one would be room_blocks => []. | ||
|
|
||
| **I would have changed the method for adding reservations so it could also be used to add a block reservation as well. The current method assigns rooms, which is a step that wouldn't be necessary if a user entered in a block id (it already would have been handled). | ||
|
|
||
| **I feel like some of my methods in reservation hub are currently doing a lot. I think the overall code would benefit by breaking some of those methods up into smaller steps. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| require_relative 'spec_helper' | ||
| require 'pry' | ||
|
|
||
| describe "DateRange class" do | ||
|
|
||
| describe "validate" do | ||
|
|
||
| it "must throw an argument error if the end date is before the start date" do | ||
|
|
||
| start_date = Date.new(2015, 03, 05) | ||
| end_date = Date.new(2015, 03, 01) | ||
|
|
||
| room_id = 4 | ||
|
|
||
| expect {Hotel::DateRange.new(start_date, end_date)}.must_raise ArgumentError | ||
|
|
||
| end | ||
| end | ||
|
|
||
|
|
||
| describe "create date array" do | ||
|
|
||
| before do | ||
| start_date = Date.new(2018,01,06) | ||
| end_date = Date.new(2018,01,10) | ||
|
|
||
| dates = Hotel::DateRange.new(start_date, end_date) | ||
|
|
||
| @date_array = dates.create_date_array(start_date, end_date) | ||
|
|
||
| @test_dates = [] | ||
|
|
||
| until start_date == end_date | ||
| @test_dates << start_date | ||
| start_date +=1 | ||
| end | ||
|
|
||
| end | ||
|
|
||
| it "returns an array of all dates" do | ||
| expect(@date_array).must_be_kind_of Array | ||
| expect(@date_array.length).must_equal @test_dates.length | ||
| end | ||
|
|
||
| it "includes all dates in the reservation, not including the end date" do | ||
| expect(@date_array).wont_include @end_date | ||
| expect(@date_array).must_equal @test_dates | ||
|
|
||
| end | ||
| end | ||
|
|
||
| end |
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.
The name
reservation_datesis a little redundant, since we're already in theReservationclass. Something likedatesordate_rangemight be a better name.