-
Notifications
You must be signed in to change notification settings - Fork 45
Pipes - Iuliia - Hotel #21
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
094e967
e4bdaeb
6f6baa7
a9371e0
0954149
3757434
f6859ad
44584d1
8f9f4a8
56ea850
f6da1fd
c1f3160
82c0bde
093781d
94cc7eb
d812013
040ea78
6b1e338
d4b29ec
3c24b40
e9d8977
3e26440
f58da33
d68577e
5eb6807
cf544f3
9b63ba1
9f867c4
45a7b2e
2edc08c
22bad22
4d28a71
61b8116
3f98f11
f4ea488
6820589
521417e
478ef7f
63a2ad9
6457100
662cb25
3574c02
1ac37c0
8f32951
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,9 @@ | ||
| require 'rake/testtask' | ||
|
|
||
| Rake::TestTask.new do |t| | ||
| t.libs = ["lib"] | ||
| t.warning = false | ||
| t.test_files = FileList['specs/*_spec.rb'] | ||
| end | ||
|
|
||
| task default: :test |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| ## What classes does each implementation include? Are the lists the same? | ||
| Implementations A and B include same three classes: CartEntry, ShoppingCart, Order. | ||
|
|
||
| ## Write down a sentence to describe each class. | ||
| Class CartEntry stores the information about product and its price. It is responsible for the price for each particular product. | ||
| Class ShoppingCart contains all products (several cart entries). | ||
| Class Order creates new instance of class ShoppingCart and is responsible for the final price of the whole order include taxes. | ||
|
|
||
| ## How do the classes relate to each other? It might be helpful to draw a diagram on a whiteboard or piece of paper. | ||
| Class Order is depends on the ShoppingCart class which is closely related to CartEntry class. Every time customer pick a new product(cart entry), it is getting stored to the instance of class ShoppingCar which is initialized in class Order. | ||
|
|
||
| ## What data does each class store? How (if at all) does this differ between the two implementations? | ||
| Class CartEntry stores price for product(unit_price) and quantity. | ||
| Class ShoppingCart stores all products that customer picked. | ||
| Class Order stores the total price for the order. | ||
| The difference between two implementations is that in implementation A all calculation logic is in Order class whereas in implementation B it is located in each class (CartEntry, ShoppingCart) where it belongs. | ||
|
|
||
| ##What methods does each class have? How (if at all) does this differ between the two implementations? | ||
| Class CartEntry has methods: initialize(A), and initialize and price(B). The difference between the two implementations is that in implementation B unit_price and quantity can't be accessed from the outside of class and instead it has a price method. | ||
| Class ShoppingCart has a similar difference between two implementations. In implementation A it has only one method initialize and can be accessed from outside of class. In implementation B private data is encapsulated. | ||
| Class Order in both implementations has two methods: initialize and total_price. The only difference is how it accesses data from two other classes. In implementation A, it uses data from CartEntry and ShoppingCart classes and calculates total_price, whereas in implementation B, it uses methods of these classes. | ||
|
|
||
| ##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 #total_price method is completely retained in Order. In implementation B it delegated to "lower level" classes by having #price methods in them. | ||
|
|
||
| ##Does total_price directly manipulate the instance variables of other classes? | ||
| In implementation A #total_price method directly manipulates the instance variables of "lower level" classes such as entries, unit_price and quantity. | ||
|
|
||
| ##If we decide items are cheaper if bought in bulk, how would this change the code? Which implementation is easier to modify? | ||
| The implementation B is easier to modify because each class has a single responsibility. To add a discount for items in bulk I would add it to #price method of CartEntry class. | ||
|
|
||
| ##Which implementation better adheres to the single responsibility principle? | ||
| The implementation B is better adheres to the single responsibility principle. Each class has a certain logic calculations and can be describe in one sentence. | ||
|
|
||
| ##Bonus question once you've read Metz ch. 3: Which implementation is more loosely coupled? | ||
| The implementation B is more loosely coupled. The private data of classes is encapsulated and classes do not know much about each other. | ||
|
|
||
| ##My improvements: | ||
|
|
||
| To improve design in Hotel project I added new method #reserve_room to class Block and changed method #make_reservation_from_block in class Hotel. It made my classes Hotel and Block more loosely coupled. In my new implementation the Hotel class knows less information about the Block class. This change did not break any of my tests but I still refactored them a little to check some new functionality. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| module BookingSystem | ||
| class Block | ||
|
|
||
| @@id_count = 1 | ||
|
|
||
| attr_reader :date_range, :rooms, :id | ||
|
|
||
| def initialize(date_range, rooms) | ||
| @id = @@id_count | ||
| @@id_count += 1 | ||
| @date_range = date_range | ||
| @rooms = rooms #hash | ||
|
|
||
| end #end of initialize | ||
|
|
||
| def has_available_rooms? | ||
| return @rooms.length > 0 | ||
| end #end of method | ||
|
|
||
| def reserve_room(room) | ||
| if !has_available_rooms? | ||
| raise NoRoomAvailableError.new("No available rooms in block") | ||
| end | ||
| if !rooms.keys.include?(room) | ||
| raise NoRoomAvailableError.new("Requested room is unavailable") | ||
| end | ||
|
|
||
| @rooms.delete(room) | ||
|
|
||
| end #end of method | ||
|
|
||
|
|
||
| end #end of class | ||
| end #end of module |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| require 'date' | ||
| require_relative 'invalid_date' | ||
|
|
||
| module BookingSystem | ||
|
|
||
| class DateRange | ||
|
|
||
| attr_reader :check_in, :check_out | ||
|
|
||
| def initialize(check_in, check_out) | ||
| @check_in = check_in | ||
| @check_out = check_out | ||
|
|
||
| if @check_in.class != Date || @check_out.class != Date || @check_in > @check_out || @check_in < Date.today | ||
| raise InvalidDate.new("Invalid date or date range") | ||
| end | ||
| end #end of initialize | ||
|
|
||
| def dates_within_range | ||
| dates = (@check_in .. @check_out - 1).map { |date| date } | ||
| return 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. Here you're mapping the list of dates to itself. You could instead say: return (@check_in..@check_out-1).to_a |
||
| end #end of method | ||
|
|
||
| end #end of class | ||
|
|
||
| end #end of module | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,123 @@ | ||
| require_relative 'reservation' | ||
| require_relative 'date_range' | ||
| require_relative 'no_room_available' | ||
| require_relative 'block' | ||
| require 'csv' | ||
|
|
||
| module BookingSystem | ||
| class Hotel | ||
|
|
||
| DEFAULT_ROOMS = {1 => 150, 2 => 200, 3 => 350, 4 => 120, 5 => 150, 6 => 170, 7 => 180, 8 => 150, 9 => 190, 10 => 200, 11 => 220, 12 => 135, 13 => 200, 14 => 190, 15 => 220, 16 => 200, 17 => 220, 18 => 250, 19 => 200, 20 => 250} | ||
|
|
||
| DEFAULT_DISCOUNT = 10 | ||
|
|
||
| DEFAULT_RESERVATIONS = "support/reservations.csv" | ||
|
|
||
| DEFAULT_RATES = "support/rates.csv" | ||
|
|
||
| attr_reader :rooms, :all_reservations, :block_discount, :all_blocks | ||
|
|
||
| #didn't finish this part, method get_rates is unused | ||
| def get_rates(file=DEFAULT_RATES) | ||
| rates = {} | ||
| CSV.open(file, "r").each do |line| | ||
| rates[line[0]] = line[1] | ||
| end | ||
| return rates | ||
| end #end of method | ||
|
|
||
| def initialize(rooms=DEFAULT_ROOMS, block_discount=DEFAULT_DISCOUNT) | ||
|
|
||
| @all_reservations = [] #array of instances of class Reservation | ||
| @all_blocks = [] #array of instances of class Block | ||
| @rooms = rooms | ||
| @block_discount = block_discount | ||
|
|
||
| end #end of initialize | ||
|
|
||
| def room_unavailable(room) | ||
| dates = [] | ||
| @all_reservations.each do |reservation| | ||
| if reservation.room == room | ||
| reservation.date_range.dates_within_range.each do |date| | ||
| dates << date | ||
| end | ||
| end | ||
| end | ||
| @all_blocks.each do |block| | ||
| block.rooms.each do |block_room, price| | ||
| if block_room == room | ||
| block.date_range.dates_within_range.each do |date| | ||
| dates << date | ||
| end | ||
| end | ||
| end | ||
| end | ||
| return dates #array of dates on which this room is unavailable | ||
| end #end of method | ||
|
|
||
| def list_of_available_rooms(date_range) | ||
| available_rooms = {} | ||
| @rooms.each do |room, price| | ||
| booked_dates = room_unavailable(room) #array of 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. This certainly solves the problem, but the logic is pretty complex, especially when you factor in the call to The general strategy is to start with an empty list and add rooms to it one by one, and the outer loop is through the set of rooms. An alternative approach that I think produces cleaner code is to start with a list of all the rooms, and have the outer loop go through reservations and blocks. When you find one that matches the desired date, you remove the room(s) from the list. In pseudocode: Note that we're able to remove the inner loop entirely. |
||
| count = 0 | ||
| date_range.dates_within_range.each do |date| | ||
| if !booked_dates.include?(date) | ||
| count += 1 | ||
| 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 having this loop here, you might write a method |
||
| end | ||
| if count == date_range.dates_within_range.length | ||
| available_rooms[room] = price | ||
| end | ||
| end | ||
| if available_rooms.length == 0 | ||
| raise NoRoomAvailableError.new("No room is available on given dates") | ||
| 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'm not sure I agree that an exception is justified here if there are no available rooms. In that case there's a way to tell the caller what happened using the existing channels: return an empty array. Once the caller has that information, they can raise an error or not as they see fit. |
||
| return available_rooms #hash of rooms | ||
| end #end of method | ||
|
|
||
| def make_reservation(date_range, room, file=DEFAULT_RESERVATIONS) | ||
| if !list_of_available_rooms(date_range).include?(room) | ||
| raise NoRoomAvailableError.new("Requested room is unavailable") | ||
| end | ||
| cost = @rooms[room] | ||
| new_reservation = Reservation.new(date_range, room, cost) | ||
| @all_reservations << new_reservation | ||
|
|
||
| new_reservation.add_reservation(file) | ||
|
|
||
| return new_reservation #new instance of class Reservation | ||
| end #end of method | ||
|
|
||
| def list_of_reservations(date) | ||
| list = @all_reservations.map { |reservation| reservation if reservation.date_range.dates_within_range.include?(date) } | ||
|
|
||
|
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. The |
||
| return list #array of reservations | ||
| end #end of method | ||
|
|
||
| def create_block(date_range, number_of_rooms) | ||
| if number_of_rooms > 5 | ||
| number_of_rooms = 5 | ||
| end | ||
| if list_of_available_rooms(date_range).length < number_of_rooms | ||
| raise NoRoomAvailableError.new("Not enough available rooms on given dates") | ||
| end | ||
| block_rooms = Hash[list_of_available_rooms(date_range).sort_by { |k, v| k }[0..number_of_rooms-1]] | ||
| new_block = Block.new(date_range, block_rooms) | ||
| @all_blocks << new_block | ||
|
|
||
| return new_block #has hash of rooms and prices | ||
| end #end of method | ||
|
|
||
| def make_reservation_from_block(block, room) | ||
| date_range = block.date_range | ||
| block.reserve_room(room) | ||
| cost = @rooms[room] * (1 - @block_discount / 100.0) | ||
| new_reservation_from_block = Reservation.new(date_range, room, cost) | ||
| return new_reservation_from_block #instance of class Block | ||
|
|
||
| end #end of method | ||
|
|
||
| end #end of class | ||
|
|
||
| end #end of module | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| module BookingSystem | ||
| class InvalidDate < ArgumentError | ||
| end #end of class | ||
| end #end of module |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| module BookingSystem | ||
| class NoRoomAvailableError < ArgumentError | ||
| end | ||
| end #end of module |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| require_relative 'date_range' | ||
| require 'csv' | ||
|
|
||
| module BookingSystem | ||
| class Reservation | ||
|
|
||
| @@id_count = 1 | ||
|
|
||
| attr_reader :id, :date_range, :room, :cost, :total_cost | ||
|
|
||
| def initialize(date_range, room, cost) | ||
|
|
||
| @id = @@id_count | ||
| @@id_count += 1 | ||
| @date_range = date_range | ||
| @room = room | ||
| @cost = cost | ||
| @total_cost = (cost * date_range.dates_within_range.length).to_f | ||
|
|
||
| end #end of initialize | ||
|
|
||
| #method seems to be working fine, but don't have any tests for loading data back | ||
| def add_reservation(file) | ||
| CSV.open(file, "a") do |line| | ||
| line << ["#{self.id}","#{self.room}","#{self.date_range.check_in.strftime("%m/%d/%Y")}","#{self.date_range.check_out.strftime("%m/%d/%Y")}","#{self.total_cost}"] | ||
| end | ||
| end #end of method | ||
|
|
||
| end #end of class | ||
|
|
||
| end #end of module |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| require_relative 'spec_helper' | ||
|
|
||
| describe "Block" do | ||
| before do | ||
| @five_rooms = {1 => 150, 2 => 200, 3 => 350, 4 => 120, 5 => 150} | ||
| @check_in = Date.new(2017,10,15) | ||
| @check_out = Date.new(2017,10,17) | ||
| @date_range = BookingSystem::DateRange.new(@check_in, @check_out) | ||
| @new_block = BookingSystem::Block.new(@date_range, @five_rooms) | ||
| end | ||
| describe "#initialize" do | ||
| it "ID should be an integer" do | ||
| @new_block.id.must_be_kind_of Integer | ||
| end | ||
| it "Date_range can be called by .date_range" do | ||
| @new_block.date_range.must_equal @date_range | ||
| end | ||
| end | ||
|
|
||
| describe "#has_available_rooms?" do | ||
| before do | ||
| @five_room_hotel = BookingSystem::Hotel.new(@five_rooms) | ||
| @new_block = @five_room_hotel.create_block(@date_range, 3) | ||
| end | ||
| it "Returns true if there are available rooms in given block" do | ||
| @new_block.has_available_rooms?.must_equal true | ||
| end | ||
| it "Returns false if there is no available rooms in given block" do | ||
| 3.times do |i| | ||
| @five_room_hotel.make_reservation_from_block(@new_block, i+1) | ||
| end | ||
| @new_block.has_available_rooms?.must_equal false | ||
| end | ||
|
|
||
| describe "#reserve_room" do | ||
| before do | ||
| @block = @five_room_hotel.create_block(@date_range, 2) | ||
| @block.reserve_room(4) | ||
| end | ||
| it "Booked room should be removed from rooms in block" do | ||
| @block.rooms.length.must_equal 1 | ||
| end | ||
| it "Raise an error if requested room is unavailable" do | ||
| proc { @block.reserve_room(4) }.must_raise BookingSystem::NoRoomAvailableError | ||
| end | ||
| it "Raise an error if no available rooms for reservation from block" do | ||
| @block.reserve_room(5) | ||
| proc { @block.reserve_room(4) }.must_raise BookingSystem::NoRoomAvailableError | ||
| end | ||
|
|
||
|
|
||
| end | ||
| end | ||
|
|
||
| end #end of describe |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| require_relative 'spec_helper' | ||
|
|
||
| describe "DateRange" do | ||
| before do | ||
| @check_in = Date.new(2017,10,15) | ||
| @check_out = Date.new(2017,10,17) | ||
| @new_date_range = BookingSystem::DateRange.new(@check_in, @check_out) | ||
| @past_date = Date.new(2015,5,3) | ||
| end | ||
|
|
||
| describe "#initialize" do | ||
| it "Check_in and check_out values are types of Date class" do | ||
| @new_date_range.check_in.must_be_kind_of Date | ||
| @new_date_range.check_out.must_be_kind_of Date | ||
| end | ||
| it "Raise an error if check_in or check_out values are not types of Date class" do | ||
| proc { BookingSystem::DateRange.new("2017-9-15", "2017-9-17") }.must_raise BookingSystem::InvalidDate | ||
| end | ||
| it "Raise an error if given check_out date is before check_in date" do | ||
|
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. Good failure cases! |
||
| proc { BookingSystem::DateRange.new(@check_out, @check_in) }.must_raise BookingSystem::InvalidDate | ||
| end | ||
| it "Raise an error if given date is in the past" do | ||
| proc { BookingSystem::DateRange.new(@past_date, @check_out) }.must_raise BookingSystem::InvalidDate | ||
| end | ||
| end | ||
|
|
||
| describe "dates_within_range" do | ||
| it "Returns an array" do | ||
| @new_date_range.dates_within_range.must_be_kind_of Array | ||
| end | ||
| it "Returns the array of right length for short ranges" do | ||
| @new_date_range.dates_within_range.length.must_equal 2 | ||
| end | ||
| it "Returns the array of right length for long ranges" do | ||
| check_in = Date.new(2017,10,15) | ||
| check_out = Date.new(2017,11,15) | ||
| BookingSystem::DateRange.new(check_in, check_out).dates_within_range.length.must_equal 31 | ||
| end | ||
| end | ||
|
|
||
| end #end of discribe | ||
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.
Good use of a custom exception here