forked from AdaGold/hotel
-
Notifications
You must be signed in to change notification settings - Fork 48
Leaves_Ga-Young #33
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
gyjin
wants to merge
22
commits into
Ada-C12:master
Choose a base branch
from
gyjin: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
Leaves_Ga-Young #33
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
8833393
Added SimpleCov and require_relative files
gyjin 9ede58b
Created classes and tests for their instantiation
gyjin 561b825
Changed class structure
gyjin de600af
Added parameters to DateRange class
gyjin ced3a8a
Added parameters and tests to Room class
gyjin 4c60a31
Added all room instances to ReservSystem class
gyjin d6fbdc2
Added parameters and test to Reservation class
gyjin 51dd6ae
Added exception if end time is before start time
gyjin f6d8608
Added making new reservation and tests
gyjin 8513b95
Fixed simplecov, added no room available exception
gyjin c37ee8e
Added accessing reservations by date
gyjin 3cb3eb0
Refactored: added a room.all method
gyjin fc7faba
Completed wave-1
gyjin c239f60
Completed wave-2
gyjin 1874fc6
Refactored, removed DateRange class
gyjin b7b9e6b
Added refactos.txt, tidied up code
gyjin ccc83b6
Added design activity file from revisit hotel
gyjin 1c2efb0
Added tests to overlap? method
gyjin 1e47bc2
Removed overlap? method tests
gyjin 2d3ca37
Added overlap? method to Reservation class
gyjin d9dc2c8
Refactored: removed overlap? from ReservSystem
gyjin aecacae
Added comments about refactoring Hotel
gyjin 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,54 @@ | ||
| What classes does each implementation include? Are the lists the same? | ||
|
|
||
| Implementation A: CartEntry, ShoppingCart, Order | ||
| Implementation B: CartEntry, ShoppingCart, Order | ||
| The lists are the same. | ||
|
|
||
| Write down a sentence to describe each class. | ||
|
|
||
| CartEntry is one instance that holds a unit price and quantity. | ||
| ShoppingCart is one instance that holds an array of entries. | ||
| Order is one instance that holds a new instance of ShoppingCart. | ||
|
|
||
| How do the classes relate to each other? It might be helpful to draw a diagram on a whiteboard or piece of paper. | ||
|
|
||
| When a new Order is created, a new instance of ShoppingCart is created as well. Order holds the ShoppingCart. Inside the ShoppingCart is an array of CartEntry instances, which hold specific item information. | ||
|
|
||
| What data does each class store? How (if at all) does this differ between the two implementations? | ||
|
|
||
| CartEntry holds the data for a single item and its unit price and quantity. ShoppingCart holds an array of CartEntry instances. An Order creates a new ShoppingCart. This is the same between the two implementations. | ||
|
|
||
| What methods does each class have? How (if at all) does this differ between the two implementations? | ||
|
|
||
| In Implementation A, the only class method is part of the Order class, where total price is calculated. | ||
| In Implementation B, the CartEntry class has a method that calculates the price for each item. The ShoppingCart class has a method to calculate the sum of all CartEntrys. The Order class has a method to calculate the total price including sales tax. | ||
| These two implementations have completely different methods. | ||
|
|
||
| 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: Retained in Order | ||
| B: Delegated to other classes | ||
|
|
||
| Does total_price directly manipulate the instance variables of other classes? | ||
|
|
||
| A&B: Does not manipulate instance variables of other classes | ||
|
|
||
| If we decide items are cheaper if bought in bulk, how would this change the code? Which implementation is easier to modify? | ||
|
|
||
| Implementation B is much easier to modify. Only the price method in the CartEntry class would need to be modified, instead of the entire Order#total_price method in Implementation A. | ||
|
|
||
| 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. | ||
|
|
||
|
|
||
|
|
||
| Refactoring Hotel: | ||
| One place in my Hotel project that takes on multiple roles was the ReservSystem#not_reserved_on_date_range method. It was reaching inside of each room, then each reservation of that room, then the start/end times of that reservation to determine if there was an overlap of dates between that of the existing reservation and the dates of the new reservation. The changes I need to make are moving the ReservSystem#overlap? method into the Reservation class, making sure that ReservSystem just receives a boolean back from the Reservation#overlap? method, and making minor adjustments to my test codes to fit the new syntax. Changing the tests would involve moving the overlap? tests from reserv_system_test.rb to reservations_test.rb. | ||
|
|
||
| This design change would be an improvement because ReservSystem will be less coupled with Reservation. ReservSystem would no longer be reaching through Reservation and getting info from those reservation instances. Instead, ReservSystem will be asking Reservation for info, and Reservation will be passing back a boolean. If any changes were made to reservations in the future, this decoupling now would make that easier later. |
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,44 @@ | ||
| RATE_OF_ROOM = 200 | ||
| NUM_OF_ROOMS = 20 | ||
| require 'pry' | ||
|
|
||
| module Hotel | ||
| class ReservSystem | ||
| attr_reader :rooms, :reservations | ||
|
|
||
| def initialize() | ||
| @rooms = Hotel::Room.all | ||
| @reservations = [] | ||
| end | ||
|
|
||
| def make_reservation(start_time, end_time) | ||
| not_reserved_rooms = not_reserved_on_date_range(start_time, end_time) | ||
|
|
||
| if not_reserved_rooms.length == 0 | ||
| raise ArgumentError.new("Cannot make a reservation for that date range. No rooms available.") | ||
| else | ||
| new_res = Reservation.new(start_time, end_time, not_reserved_rooms[0]) | ||
| @reservations << new_res | ||
| not_reserved_rooms[0].reservation << new_res | ||
| end | ||
| end | ||
|
|
||
| def not_reserved_on_date_range(start_time, end_time) | ||
| not_reserved_rooms = [] | ||
|
|
||
| @rooms.each do |curr_room| | ||
| if curr_room.reservation.length == 0 | ||
| not_reserved_rooms << curr_room | ||
| else | ||
| curr_room.reservation.each do |each_res| | ||
| if each_res.overlap?(start_time, end_time) == false | ||
| not_reserved_rooms << curr_room | ||
| end | ||
| end | ||
| end | ||
| end | ||
|
|
||
| return not_reserved_rooms | ||
| end | ||
| end | ||
| 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,26 @@ | ||
| module Hotel | ||
| class Reservation | ||
| attr_reader :start_time, :end_time, :room_assigned, :date_range, :cost | ||
|
|
||
| def initialize(start_time, end_time, room_assigned) | ||
| @room_assigned = room_assigned | ||
| @start_time = Date.parse(start_time) | ||
| @end_time = Date.parse(end_time) | ||
| @cost = days_between * RATE_OF_ROOM | ||
| end | ||
|
|
||
| def days_between | ||
| return @end_time - @start_time | ||
| end | ||
|
|
||
| def overlap?(inquired_start, inquired_end) | ||
| inquired_start = Date.parse(inquired_start) | ||
| inquired_end = Date.parse(inquired_end) | ||
| if @start_time < inquired_end && inquired_start < @end_time | ||
| return true | ||
| else | ||
| return false | ||
| end | ||
| end | ||
| end | ||
| 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,22 @@ | ||
| module Hotel | ||
| class Room | ||
| attr_reader :id | ||
| attr_accessor :reservation, :rate | ||
|
|
||
| def initialize(id, reservation: nil, rate: nil) | ||
| @id = id | ||
| @reservation = [] | ||
| @rate = RATE_OF_ROOM | ||
| end | ||
|
|
||
| def self.all | ||
| all_rooms = [] | ||
| room_num = 1 | ||
| NUM_OF_ROOMS.times do | ||
| all_rooms << Room.new(room_num) | ||
| room_num +=1 | ||
| end | ||
| return all_rooms | ||
| end | ||
| end | ||
| 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,23 @@ | ||
| Refactors I've done so far: | ||
|
|
||
| 1. Added a room.all method, instead of initializing all room instances | ||
| inside of the ReservSystem initialization | ||
| 2. Removed the DateRange class because it was not being used much. | ||
| Most of the string-to-Date conversion was being done elsewhere | ||
|
|
||
|
|
||
| Refactors I would like to do: | ||
|
|
||
| 1. A lot of my tests seem redundant. While I want to make sure every | ||
| possible scenario is covered and that my code is doing what I want it | ||
| to, I may be able to cut some out | ||
| - use let instead of before/do | ||
| 2. The reserv_system class has some bulky methods. Not sure at the | ||
| moment if I could remove some of the responsibilities in each to | ||
| its own method, but want to look into it more | ||
| 3. Will need to complete wave-3, adding hotel blocks | ||
| - potentially add a subclass to Reservation class, or make it its own | ||
| class with object composition | ||
| 4. I just realized I didn't put in any errors for wrong input | ||
| - incorrect start/end time formats, data types | ||
| - tests to make sure these are being caught | ||
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,75 @@ | ||
| require_relative 'test_helper' | ||
|
|
||
| describe "ReservSystem class" do | ||
| describe "ReservSystem instantiation" do | ||
| before do | ||
| @reserv_system = Hotel::ReservSystem.new | ||
| end | ||
|
|
||
| it "is an instance of ReservSystem" do | ||
| expect(@reserv_system).must_be_kind_of Hotel::ReservSystem | ||
| end | ||
|
|
||
| it "correctly saves all instances of Room" do | ||
| expect(@reserv_system.rooms).must_be_kind_of Array | ||
| expect(@reserv_system.rooms.length).must_equal 20 | ||
| expect(@reserv_system.rooms.first.id).must_equal 1 | ||
| expect(@reserv_system.rooms.last.id).must_equal 20 | ||
| expect(@reserv_system.rooms[10].id).must_equal 11 | ||
| end | ||
| end | ||
|
|
||
| describe "ReservSystem methods" do | ||
| describe "making a new reservation" do | ||
| before do | ||
| @reserv_system = Hotel::ReservSystem.new | ||
| @reserv_system.make_reservation('2019-3-1', '2019-3-6') | ||
| end | ||
|
|
||
| it "can make a new reservation" do | ||
| expect(@reserv_system.reservations).must_be_kind_of Array | ||
| expect(@reserv_system.reservations.length).must_equal 1 | ||
| end | ||
|
|
||
| it "can correctly assign a room to a new reservation" do | ||
| expect(@reserv_system.reservations[0]).must_be_kind_of Hotel::Reservation | ||
| expect(@reserv_system.reservations[0].room_assigned).must_be_kind_of Hotel::Room | ||
| expect(@reserv_system.reservations[0].room_assigned.id).must_equal 1 | ||
|
|
||
| @reserv_system.make_reservation('2019-3-4', '2019-3-8') | ||
| expect(@reserv_system.reservations.length).must_equal 2 | ||
| expect(@reserv_system.reservations[1].room_assigned.id).must_equal 2 | ||
| end | ||
|
|
||
| it "raises an error if a reservation is made when all rooms are booked" do | ||
| @reserv_system.rooms.each do |curr_room| | ||
| curr_room.reservation = [Hotel::Reservation.new('2019-1-1', '2019-1-5', curr_room)] | ||
| end | ||
| expect{@reserv_system.make_reservation('2019-1-1', '2019-1-5')}.must_raise ArgumentError | ||
| end | ||
| end | ||
|
|
||
| describe "listing all available rooms for a date range" do | ||
| before do | ||
| @reserv_system = Hotel::ReservSystem.new | ||
| end | ||
|
|
||
| it "can return all rooms as available for first-ever reservation" do | ||
| all_rooms = @reserv_system.not_reserved_on_date_range('2019-3-1', '2019-3-6') | ||
| expect(all_rooms).must_be_kind_of Array | ||
| expect(all_rooms.length).must_equal 20 | ||
| end | ||
|
|
||
| it "can correctly return all available rooms after previous reservations have been made" do | ||
| @reserv_system.make_reservation('2019-3-1', '2019-3-6') | ||
| @reserv_system.make_reservation('2019-3-5', '2019-3-8') | ||
| @reserv_system.make_reservation('2019-4-1', '2019-4-5') | ||
|
|
||
| all_rooms = @reserv_system.not_reserved_on_date_range('2019-3-2', '2019-4-3') | ||
| expect(all_rooms).must_be_kind_of Array | ||
| expect(all_rooms.length).must_equal 18 | ||
| end | ||
| end | ||
| end | ||
| 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_relative 'test_helper' | ||
|
|
||
| describe "Reservation class" do | ||
| describe "Reservation instantiation" do | ||
| before do | ||
| @room = Hotel::Room.new(3) | ||
| @reservation = Hotel::Reservation.new('2019-1-1', '2019-1-5', @room) | ||
| end | ||
|
|
||
| it "is an instance of Reservation" do | ||
| expect(@reservation).must_be_kind_of Hotel::Reservation | ||
| end | ||
|
|
||
| it "correctly saves instance variables" do | ||
| expect(@reservation.start_time).must_be_kind_of Date | ||
| expect(@reservation.cost).must_equal 800 | ||
| expect(@reservation.room_assigned).must_be_kind_of Hotel::Room | ||
| expect(@reservation.room_assigned.id).must_equal 3 | ||
| end | ||
| end | ||
|
|
||
| describe "determining date overlap" do | ||
| before do | ||
| @reserv_system = Hotel::ReservSystem.new | ||
| @room = Hotel::Room.new(1) | ||
| @date_1 = '2019-4-06' | ||
| @date_2 = '2019-4-10' | ||
| @date_3 = '2019-4-14' | ||
| @date_4 = '2019-4-18' | ||
| end | ||
|
|
||
| it "returns true if the end of one date range overlaps the beginning of another" do | ||
| new_res = Hotel::Reservation.new(@date_1, @date_3, @room) | ||
| new_res2 = Hotel::Reservation.new(@date_2, @date_4, @room) | ||
| expect(new_res.overlap?(@date_2, @date_4)).must_equal true | ||
| expect(new_res2.overlap?(@date_1, @date_3)).must_equal true | ||
| end | ||
|
|
||
|
|
||
| it "returns true if a date range is inside the other" do | ||
| new_res = Hotel::Reservation.new(@date_1, @date_4, @room) | ||
| new_res2 = Hotel::Reservation.new(@date_2, @date_3, @room) | ||
| expect(new_res.overlap?(@date_2, @date_3)).must_equal true | ||
| expect(new_res2.overlap?(@date_1, @date_4)).must_equal true | ||
| end | ||
|
|
||
| it "returns false if the end of one date range is on the same day as the start of another" do | ||
| new_res = Hotel::Reservation.new(@date_1, @date_2, @room) | ||
| new_res2 = Hotel::Reservation.new(@date_2, @date_3, @room) | ||
| expect(new_res.overlap?(@date_2, @date_3)).must_equal false | ||
| expect(new_res2.overlap?(@date_1, @date_2)).must_equal false | ||
| end | ||
|
|
||
| it "returns false if the date ranges do not overlap" do | ||
| new_res = Hotel::Reservation.new(@date_1, @date_2, @room) | ||
| expect(new_res.overlap?(@date_3, @date_4)).must_equal false | ||
| end | ||
| end | ||
| 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,20 @@ | ||
| require_relative 'test_helper' | ||
|
|
||
| describe "Room class" do | ||
| describe "Room instantiation" do | ||
| before do | ||
| @room = Hotel::Room.new(1) | ||
| end | ||
|
|
||
| it "is an instance of Room" do | ||
| expect(@room).must_be_kind_of Hotel::Room | ||
| end | ||
|
|
||
| it "correctly saves instance variables" do | ||
| expect(@room.id).must_equal 1 | ||
| expect(@room.reservation).must_be_kind_of Array | ||
| expect(@room.reservation.length).must_equal 0 | ||
| expect(@room.rate).must_equal 200 | ||
| end | ||
| end | ||
| 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 |
|---|---|---|
| @@ -1,8 +1,15 @@ | ||
| # Add simplecov | ||
| require 'simplecov' | ||
| SimpleCov.start do | ||
| add_filter 'test/' # Tests should not be checked for coverage. | ||
| end | ||
|
|
||
| require "minitest" | ||
| require "minitest/autorun" | ||
| require "minitest/reporters" | ||
|
|
||
| Minitest::Reporters.use! Minitest::Reporters::SpecReporter.new | ||
|
|
||
| # require_relative your lib files here! | ||
| require_relative '../lib/reserv_system' | ||
| require_relative '../lib/reservation' | ||
| require_relative '../lib/room' | ||
|
|
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.
Your tests are very thorough, and I appreciate it!