-
Notifications
You must be signed in to change notification settings - Fork 46
goeun - edges - hotel #22
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
a79e8c4
73609dc
44059e3
d0ba0bf
d4e5b6d
62b0679
b054817
aaedaaf
fe5d223
bfbb588
fe6c4f2
caaf57b
c0cee89
b0fc136
a431409
4859efe
e8de7fe
4f89d83
abeee86
5a90808
c4d7e68
a963903
e781d16
eca8b38
1be5ba3
8278260
108afe7
df6bd13
821a8db
96f44ed
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 |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| ### What classes does each implementation include? Are the lists the same? | ||
|
|
||
| Each implementation includes the classes CartEntry, ShoppingCart, and Order. In Implementation A, CartEntry and ShoppingCart both have initialize as only methods and class Order has initialize and total_price. In Implementation B, CartEntry and ShoppingCart has price method in addition. | ||
|
|
||
| ### Write down a sentence to describe each class. | ||
|
|
||
| CartEntry initializes with the unity price and quantity of the entry. ShoppingCart initializes with an empty array of entries. Order initializes by creating a new instance of ShoppingCart and has a method to calculate the total price. In Implementation B, each class can calculate their own price on their own (whether it's price for entry, price for shopping cart, or total price in order including tax). | ||
|
|
||
| ### How do the classes relate to each other? It might be helpful to draw a diagram on a whiteboard or piece of paper. | ||
|
|
||
| CartEntry is added to an array in ShoppingCart. Order can calculate the total price from the shopping cart's entries. In implementation B, the price is calculated in every class so that Order doesn't need to do the calculating logic using CartEntry and ShoppingCart's instance variable and can instead get the subtotal by using @cart.price. | ||
|
|
||
| ### What data does each class store? How (if at all) does this differ between the two implementations? | ||
|
|
||
| CartEntry stores the unit_price and quantity, ShoppingCart stores an array of CartEntry objects. Order stores one instance of ShoppingCart. There are additional methods in implementation B (the price method) but those calculate and return the sum, not store the data. | ||
|
|
||
| ### What methods does each class have? How (if at all) does this differ between the two implementations? | ||
|
|
||
| Other than initialize, CartEntry and ShoppingCart have a price method that can return the price of each entry or cart in implementation B. In implementation A, only Order had a total_price method which did not adhere to single responsibility and used variables from ShoppingCart and CartEntry. | ||
|
|
||
|
|
||
| ### 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, it's retained in Order. In implementation B, it's delegated to CartEntry and ShoppingCart. | ||
|
|
||
| ### Does total_price directly manipulate the instance variables of other classes? | ||
|
|
||
| In A, yes. in B, no. | ||
|
|
||
| ### If we decide items are cheaper if bought in bulk, how would this change the code? Which implementation is easier to modify? | ||
|
|
||
| Implementation B makes the change easier to modify the code in CartEntry (such as a conditional loop) instead of trying to make that logic work in Order. | ||
|
|
||
| ### 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. The total_price in Order doesn't require knowing the unit price and quantity of each CartEntry. | ||
|
|
||
| ### Changes made in Hotel | ||
|
|
||
| In my original HotelBooker class, I had a method for make_block and directly changed the cost of the Room as such: | ||
|
|
||
| ` available[i].cost = discount ` | ||
|
|
||
| To make this code less coupled, I created a wrapping method in Room `set_discount` so HotelBooker wouldn't directly handle the instance variable from Room. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| require 'date' | ||
|
|
||
| module Hotel | ||
| class DateRange | ||
| attr_reader :check_in, :check_out, :dates_booked | ||
|
|
||
| def initialize(check_in, check_out) | ||
| if check_in >= check_out | ||
| raise StandardError, "Your check in and check out date is not valid!" | ||
| elsif !check_in.is_a?(Date) || !check_out.is_a?(Date) | ||
| raise ArgumentError, "You must input Dates in the date range!" | ||
| end | ||
| @check_in = check_in | ||
| @check_out = check_out | ||
| @dates_booked = (check_in...check_out).to_a | ||
| end | ||
|
|
||
| def overlaps?(range) | ||
| @dates_booked & range.dates_booked == [] ? false : true | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| require 'date' | ||
| require_relative 'room' | ||
| require_relative 'reservation' | ||
| require_relative 'date_range' | ||
|
|
||
| module Hotel | ||
| class HotelBooker | ||
| attr_accessor :rooms, :reservations, :unreserved_block, :reserved_block | ||
| NUM_ROOMS = 20 | ||
| BLOCK_MAX = 5 | ||
|
|
||
| def initialize() | ||
| @unreserved_block = [] | ||
| @reserved_block = [] | ||
| @reservations = [] | ||
| @rooms = [] | ||
|
|
||
| NUM_ROOMS.times do |i| | ||
| @rooms << Room.new(i+1) | ||
| end | ||
| end | ||
|
|
||
| def available?(available_rooms) | ||
| if available_rooms.empty? | ||
| raise StandardError, "There are no more available rooms for this date range!" | ||
| end | ||
| end | ||
|
|
||
| def range(check_in, check_out) | ||
| DateRange.new(check_in, check_out) | ||
| end | ||
|
|
||
| # arguments for check_in and check_out are Strings | ||
| def make_reservation(id, check_in, check_out) | ||
| check_in = Date.parse(check_in) | ||
| check_out = Date.parse(check_out) | ||
| reservation = Reservation.new(id, range(check_in, check_out)) | ||
|
|
||
| available = unreserved_rooms(check_in, check_out) | ||
| available?(available) | ||
|
|
||
| reservation.room = available[0] | ||
| @reservations << reservation | ||
| end | ||
|
|
||
|
|
||
| def make_block(rooms, discount, check_in, check_out) | ||
| check_in = Date.parse(check_in) | ||
| check_out = Date.parse(check_out) | ||
| date_range = range(check_in, check_out) | ||
| available = unreserved_rooms(check_in, check_out) | ||
| if rooms > BLOCK_MAX | ||
| raise StandardError, "Five is the maximum number of rooms in a block." | ||
| elsif available.length < rooms | ||
| raise StandardError, "There are not enough available rooms to create a block." | ||
| end | ||
|
|
||
| rooms.times do |i| | ||
| reservation = Reservation.new("BLOCK ROOM #{i+1}", date_range) | ||
| available[i].set_discount(discount) | ||
| reservation.room = available[i] | ||
| @unreserved_block << reservation | ||
| end | ||
|
|
||
| return @unreserved_block | ||
| end | ||
|
|
||
|
|
||
| def make_block_reservation(id) | ||
| available?(@unreserved_block) | ||
| @reserved_block << @unreserved_block[0] | ||
| @unreserved_block.delete_at(0) | ||
| end | ||
|
|
||
|
|
||
| # arguments for check_in and check_out are Dates | ||
| def unreserved_rooms(check_in, check_out) | ||
| reserved_rooms = [] | ||
| unreserved_block_rooms = [] | ||
| reserved_block_rooms = [] | ||
| new_range = range(check_in, check_out) | ||
|
|
||
| @reservations.each do |reservation| | ||
| if reservation.date_range.overlaps?(new_range) | ||
| reserved_rooms << reservation.room | ||
| end | ||
| end | ||
|
|
||
| @unreserved_block.each do |reservation| | ||
| if reservation.date_range.overlaps?(new_range) | ||
| unreserved_block_rooms << reservation.room | ||
| end | ||
| end | ||
|
|
||
| @reserved_block.each do |reservation| | ||
| if reservation.date_range.overlaps?(new_range) | ||
| reserved_block_rooms << reservation.room | ||
| end | ||
| end | ||
|
|
||
| unreserved = @rooms - reserved_rooms - unreserved_block_rooms - reserved_block_rooms | ||
|
|
||
| return unreserved | ||
| end | ||
|
|
||
| def unreserved_block_rooms | ||
| unreserved_block_rooms = | ||
| @unreserved_block.map { |reservation| reservation.room } | ||
| return unreserved_block_rooms | ||
| end | ||
|
|
||
|
|
||
| def find_reservations(date) | ||
| matching_reservations = [] | ||
| date = Date.parse(date) | ||
|
|
||
| @reservations.each do |reservation| | ||
| reservation_dates = reservation.date_range.dates_booked | ||
| if reservation_dates.include?(date) | ||
| matching_reservations << reservation | ||
| end | ||
| end | ||
|
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 using .each, might be good opportunity to use |
||
|
|
||
| @reserved_block.each do |reservation| | ||
| reservation_dates = reservation.date_range.dates_booked | ||
| if reservation_dates.include?(date) | ||
| matching_reservations << reservation | ||
| end | ||
| end | ||
|
|
||
| return matching_reservations | ||
| end | ||
|
|
||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| require_relative 'room' | ||
|
|
||
| module Hotel | ||
| class Reservation | ||
| attr_reader :id, :date_range | ||
| attr_accessor :room | ||
|
|
||
| def initialize(id, date_range) | ||
| @id = id | ||
| @date_range = date_range | ||
| # placeholder room initation | ||
| @room = Room.new(0) | ||
| end | ||
|
|
||
| def cost | ||
| @cost = calculate_total_cost | ||
| end | ||
|
|
||
| def calculate_total_cost | ||
| total_days = @date_range.dates_booked.length | ||
| total_cost = @room.cost * total_days | ||
| return total_cost | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| module Hotel | ||
| class Room | ||
| attr_reader :id | ||
| attr_accessor :cost | ||
|
|
||
| def initialize(id) | ||
| @id = id | ||
| @cost = 200 | ||
| end | ||
|
|
||
| def set_discount(new_cost) | ||
| @cost = new_cost | ||
| end | ||
|
|
||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| 1. I...lowkey cheated and made a collection of Reservations instead of a collection of Rooms. I would rewrite this part by making a subclass under Room called BookedRoom? | ||
|
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. Not really sure what you mean by this tbh! |
||
|
|
||
| 2. HotelBooker has a lot of WET code because I have to loop through several data structures (@unreserved_block, @reserved_block, @reservations)!! If I had focused the Wave 3 blocks of rooms on the ROOMS instead of reservations, probably wouldn't have created this mess ¯\_(ツ)_/¯ | ||
|
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. 👍 |
||
|
|
||
| 3. there are certain check_in and check_out arguments that are strings (as in Hotel::HotelBooker.new.make_reservation) and some that are an instance of Date (as in Hotel::HotelBooker.new.unreserved_rooms). Other arguments pass in a Hotel::DateRange (such as Hotel::Reservations). note to future goeun: make this less terrible | ||
|
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. Would absolutely agree with this |
||
|
|
||
| 4. How many ways can you say "range"? ;__; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| require_relative 'spec_helper' | ||
|
|
||
| describe "DateRange Class" do | ||
| before do | ||
| @date1 = Date.parse('2018-09-05') | ||
| @date2 = Date.parse('2018-09-09') | ||
| @date3 = Date.parse('2018-09-10') | ||
| @date4 = Date.parse('2018-09-11') | ||
|
|
||
| @date_range1 = Hotel::DateRange.new(@date1, @date2) | ||
| @date_range2 = Hotel::DateRange.new(@date1, @date3) | ||
| @date_range3 = Hotel::DateRange.new(@date2, @date3) | ||
| @date_range4 = Hotel::DateRange.new(@date3, @date4) | ||
| end | ||
|
|
||
| describe "Date Range initation " do | ||
| it "is an instance of Date Range" do | ||
| expect(@date_range1).must_be_kind_of Hotel::DateRange | ||
| end | ||
|
|
||
| it "passes Dates as arguments" do | ||
| expect(@date_range1.check_in).must_be_kind_of Date | ||
| expect(@date_range2.check_out).must_be_kind_of Date | ||
| end | ||
|
|
||
| it "raises a StandardError if invalid date range is provided" do | ||
| expect{ Hotel::DateRange.new(@date1, @date1) }.must_raise StandardError | ||
| expect{ Hotel::DateRange.new(@date3, @date1) }.must_raise StandardError | ||
| end | ||
| it "raises an ArgumentError if user do not put in a Date" do | ||
| expect{ Hotel::DateRange.new(@date1, '12/08/23') }.must_raise ArgumentError | ||
| expect{ Hotel::DateRange.new('12/06/30', @date1) }.must_raise ArgumentError | ||
| expect{ Hotel::DateRange.new('12/06/30', '12/08/23') }.must_raise ArgumentError | ||
| end | ||
|
|
||
| it "creates an array of Dates between check_in and check_out & excluding check_out" do | ||
| expect(@date_range3.dates_booked).must_be_kind_of Array | ||
| expect(@date_range3.dates_booked.length).must_equal 1 | ||
| expect(@date_range3.dates_booked[0]).must_equal @date2 | ||
| expect(@date_range2.dates_booked.length).must_equal 5 | ||
| expect(@date_range2.dates_booked[0]).must_equal @date1 | ||
| end | ||
| end | ||
|
|
||
| describe "overlaps? method to check if ranges overlap" do | ||
| it "returns true if date ranges are the same" do | ||
| expect(@date_range1.overlaps?(@date_range1)).must_equal true | ||
| end | ||
|
|
||
| it "returns true if date ranges overlaps in the front" do | ||
| expect(@date_range1.overlaps?(@date_range2)).must_equal true | ||
| end | ||
|
|
||
| it "returns true if date ranges overlaps in the back" do | ||
| expect(@date_range2.overlaps?(@date_range1)).must_equal true | ||
| end | ||
|
|
||
| it "returns true if one range is completely contained in another range" do | ||
| expect(@date_range2.overlaps?(@date_range3)).must_equal true | ||
| end | ||
|
|
||
| it "returns false if one range is completely after" do | ||
| expect(@date_range1.overlaps?(@date_range4)).must_equal false | ||
| end | ||
|
|
||
| it "returns false if one range is completely before" do | ||
| expect(@date_range4.overlaps?(@date_range1)).must_equal false | ||
| end | ||
| it "returns false if a range starts on another's check in date" do | ||
| expect(@date_range1.overlaps?(@date_range3)).must_equal false | ||
| expect(@date_range2.overlaps?(@date_range4)).must_equal false | ||
| end | ||
| end | ||
| end | ||
|
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 love this DateRange class because it isolated the overlapping date logic to this one class, which is complex and tested in isolation... Beautiful :* truly inspiring :* |
||
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.
YES CONSTANTS