-
Notifications
You must be signed in to change notification settings - Fork 46
Katricia - Edges - Hotel #35
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
7acc072
308e03f
7230317
89ea0f1
8e0c9de
3d424bf
f1954c1
25e3199
78e0065
9c891a6
650f522
a8c928f
127b6cb
5a6f954
8602647
fcceb7b
5a0c061
0e405f9
e090bb6
cfba273
c9a97ae
8891e1a
281659c
af5e592
fbfaa8e
45df5ed
ea97fad
65d9326
50a7a42
58fa804
65c9062
285d676
89b9670
bba8e60
e2cd784
84f7cfb
45316b4
4efbbc5
2713f1a
9a59539
bf1b240
62a4374
729f3f8
bde0226
0025f00
82b3c87
f9862e9
565cfea
aa20313
eb91759
5fb51ec
b34e6f0
c962098
9369c77
aac994e
5edf06e
8c6d95f
4b7d2eb
fe06dda
1aa1985
c25a906
ad7a1ba
e04e5a6
85ce565
b08a5b5
6182750
263dca1
d261b58
41af7f5
46c1597
d8a5ede
b9047e2
304fb98
1d0f3cc
a4cfa15
abf9c23
7e47e4c
c57e111
30cfe70
3f0e697
3005254
835f757
0b72d5c
3db140a
320840c
e2a1ab2
d2cbed8
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,30 @@ | ||
| ## Evaluating Responsibility | ||
|
|
||
| * What classes does each implementation include? Are the lists the same? | Each has the same classes, CartEntry, ShoppingCart, and Order, but the methods have been reorganized in the second implementation. | ||
| * Write down a sentence to describe each class. | | ||
| * CartEntry handles an item added, taking in as parameters its unit price and quantity. It also calculates the price of that CartEntry instance. | ||
| * ShoppingCart manages the list of entries and price of all entries combined. | ||
| * Order stores the constant SALES_TAX and totals the sum of the ShoppingCart plus 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. | ||
| * Order totals the cost of the ShoppingCart which has a collection of instances of CartEntry. | ||
| * What data does each class store? How (if at all) does this differ between the two implementations? | ||
| * CartEntry stores the instance variables unit_price and quantity, which are only used in CartEntry. In the second implementation, it does not have an attribute accessor. | ||
| * ShoppingCart stores the instance variable entries which is a collection of CartEntry instances. In the second implementation, it does not have an attribute accessor. | ||
| * Order stores the constant SALES_TAX. It stores the instance of ShoppingCart. | ||
| * What methods does each class have? How (if at all) does this differ between the two implementations? | ||
| * Each has an initialize, but they no longer as attribute reader/writers. The Order total_price method no longer calls instance variables from other classes. It only calls an instance variable of ShoppingCart, which was initialized in Order. | ||
| * 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? | ||
| * Computing the price is retained in Order. | ||
| * Does total_price directly manipulate the instance variables of other classes? | ||
| * No, only those directly called in Order. | ||
| * If we decide items are cheaper if bought in bulk, how would this change the code? Which implementation is easier to modify? | ||
| * This would be easier to change in Implementation B because we would only need to change what is passed into CartEntry unit price and/or quantity. | ||
| * Which implementation better adheres to the single responsibility principle? | ||
| * Implementation B better adheres to single responsibility because each class only operates on and calculates its own instance variables. | ||
| * Bonus question once you've read Metz ch. 3: Which implementation is more loosely coupled? | ||
| * Implementation B is more loosely coupled. It has less instances of directly calling/naming an instance variable or attribute from other classes or variables. | ||
|
|
||
| ## Revisiting Hotel | ||
| Identify one place in your Hotel project where a class takes on multiple roles, or directly modifies the attributes of another class. Describe in design-activity.md what changes you would need to make to improve this design, and how the resulting design would be an improvement. | ||
| BookingManager class takes on multiple roles, some of which are acting on (or creating) attributes of class Reservation. I would need to move several of the functions from BookingManager to Reservation. Making this change would allow my code to more so follow Single Responsibility. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,187 @@ | ||
| require_relative 'room' | ||
| require_relative 'reservation' | ||
| # require 'pry' | ||
|
|
||
| module Hotel | ||
| class BookingManager | ||
| attr_accessor :rooms, :reservations, :room_calendar | ||
|
|
||
| def initialize(number_rooms) | ||
| @rooms = populate_room_list(number_rooms) #(20) | ||
| @reservations = make_reservation_list | ||
| @room_calendar = make_room_calendar(number_rooms) | ||
| end | ||
|
|
||
| # Create list of rooms as list of room Instances | ||
| def populate_room_list(number_rooms) | ||
| rooms = [] | ||
| num = 1 | ||
|
|
||
| number_rooms.times do |room| | ||
| room = Room.new(num) | ||
| # | ||
| rooms << room | ||
| num += 1 | ||
| end | ||
|
|
||
| return rooms | ||
| end | ||
|
|
||
|
|
||
| # Create array to store all of reservations | ||
| def make_reservation_list | ||
| reservations = [] | ||
| return reservations | ||
| end | ||
|
|
||
|
|
||
| # Create list of rooms with reserved dates. | ||
| def make_room_calendar(number) | ||
| @room_calendar = {} | ||
|
|
||
| @rooms.each do |room| | ||
| dates_reserved = {} | ||
| @room_calendar[room] = dates_reserved | ||
| end | ||
|
|
||
| return @room_calendar | ||
| end | ||
|
|
||
|
|
||
| # Method to add a reservation to list of reservations | ||
| # def add_reservation(reservation) #combine with add_reservation_to_calendar? | ||
| # @reservations << reservation | ||
| # end | ||
|
|
||
|
|
||
| # Add reservation date range to hash of room reserved dates | ||
| def add_reservation_to_calendar(reservation) | ||
| date = reservation.start_date | ||
| @reservations << reservation | ||
|
|
||
| reservation.number_nights.times do | ||
| @room_calendar[reservation.room][date] = reservation # dependency | ||
| date += 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. I like the idea of using a hash of hashes to keep track of the reservations for each date for each room. This definitely works to solve the problem. However, this ends up being an example of tight coupling. The information about what dates a reservation is for is repeated here in the The way to resolve this would be to make the reservations the final source of truth about what dates they're for. When the
Author
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. Are you saying instead of having BookingManager have a calendar of Rooms with Reservations, that that information should only be accessible by ..one instance variable?Where else would the reservation date be changed besides in the add_reservation/add_reservation_to calendar method? (I have now combined those two. And maybe only need one.) Each reservation does have info on what date it is for. |
||
|
|
||
| # binding.pry | ||
| return @room_calendar | ||
| end | ||
|
|
||
|
|
||
| # Check if date range given is valid - start must be before end | ||
| def check_dates(start_date, end_date) | ||
| if start_date > end_date | ||
| raise ArgumentError.new "Invalid date range. Start date must be before end date, both in format of 'Month dd, yyyy'. " | ||
| end | ||
| end | ||
|
|
||
|
|
||
| # Create array of all dates from start date to end date | ||
| def determine_date_range(start_date, end_date) | ||
| check_dates(start_date, end_date) | ||
|
|
||
| res_start_date = Date.parse(start_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. You do a great job of using vertical whitespace (newlines) to break up the ideas in your code. This makes the whole program much easier to read. |
||
| res_end_date = Date.parse(end_date) | ||
|
|
||
| date_range = [] | ||
| search_date = res_start_date | ||
|
|
||
| until search_date > res_end_date | ||
| date_range << search_date | ||
| search_date += 1 | ||
| end | ||
| return date_range | ||
| end | ||
|
|
||
| # Method to list all reservation instances | ||
| def list_reservations | ||
| return @reservations | ||
| 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. This does exactly the same thing as |
||
|
|
||
|
|
||
| # Method to list all rooms in hotel | ||
| def list_rooms | ||
| return @rooms | ||
| end | ||
|
|
||
| # # Method to get total cost of reservation | ||
| # def get_reservation_cost(nights, cost_per_night) | ||
| # total_cost = nights * cost_per_night | ||
| # return total_cost | ||
| # end | ||
|
|
||
|
|
||
| # Return array reservations with matching date from room calendar hash | ||
| def find_reservations_on_date(date, calendar) | ||
| search_date = Date.parse(date) | ||
| found_reservations = [] | ||
|
|
||
| calendar.each do |room, info| | ||
| info.each do |date, reservation| | ||
| if date == search_date | ||
| found_reservations << reservation | ||
| else | ||
| next | ||
| end | ||
| end | ||
| end | ||
| # Add return message for no reservations found? | ||
| return found_reservations | ||
| end | ||
|
|
||
| def find_vacancies_on_date(date, calendar) | ||
| search_date = Date.parse(date) | ||
| found_vacancies = [] | ||
|
|
||
| calendar.each do |room, info| | ||
| if info == nil | ||
| found_vacancies << room | ||
| else | ||
| info.any? {|date, reservation| date == search_date}? next : found_vacancies << room | ||
| 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 would probably not use a ternary here - it feels like too many things on one line. |
||
| end | ||
|
|
||
| return found_vacancies | ||
| #.empty? ? no_vacancies_message: found_vacancies | ||
| 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. In general, it's considered bad practice to return a string in a failure case like this. One of the main reasons is that whoever calls this code will have to do a lot of work to determine whether or not it failed. Instead you should either raise an exception, or (since this is supposed to return a collection) return an empty array. A simple implementation of the second option would be replacing line 143 with return found_vacancies |
||
|
|
||
|
|
||
| def find_vacancies_in_date_range(start_date, end_date) | ||
| check_dates(start_date, end_date) | ||
| date_range = determine_date_range(start_date, end_date) | ||
| rooms_available_in_date_range = [] | ||
|
|
||
| @room_calendar.each do |room, reserved_dates| | ||
| if reserved_dates.empty? | ||
| rooms_available_in_date_range << room | ||
| else | ||
| if (reserved_dates.keys && date_range).length > 0 | ||
| next | ||
| else | ||
| rooms_available_in_date_range << room | ||
| end | ||
| end | ||
| end | ||
|
|
||
| return rooms_available_in_date_range | ||
| end | ||
|
|
||
|
|
||
| # def no_vacancies_message | ||
| # return "There are no vacancies for the given date range." | ||
| # end | ||
|
|
||
|
|
||
| def reserve_available_room(guest_name, start_date, end_date) | ||
| check_dates(start_date, end_date) | ||
| available_rooms = find_vacancies_in_date_range(start_date, end_date) | ||
|
|
||
| new_reservation = Reservation.new(available_rooms.first, guest_name: guest_name, start_date: start_date, end_date: end_date) | ||
| # add_reservation(new_reservation) | ||
| add_reservation_to_calendar(new_reservation) | ||
| return new_reservation #unneeded? | ||
| end | ||
|
|
||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| module Hotel | ||
| class Reservation | ||
| attr_reader :room, :number_nights | ||
| attr_accessor :guest_name, :start_date, :end_date, :cost_per_night | ||
|
|
||
| def initialize(room, guest_name:, start_date:, end_date:, cost_per_night: 200.00) | ||
| @room = room | ||
| @guest_name = guest_name | ||
| check_dates(start_date, end_date) | ||
| @start_date = Date.parse(start_date) | ||
| @end_date = Date.parse(end_date) | ||
| @cost_per_night = cost_per_night | ||
| @number_nights = (@end_date - @start_date).to_i | ||
| end | ||
|
|
||
| def check_dates(start_date, end_date) | ||
| if start_date > end_date | ||
| raise ArgumentError.new "Invalid date range. Start date must be before end date, both in format of 'Month dd, yyyy'. " | ||
| end | ||
| end | ||
|
|
||
| # # Method to get total cost of reservation | ||
| def get_reservation_cost | ||
| total_cost = @number_nights * @cost_per_night | ||
| return total_cost | ||
| end | ||
|
|
||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| module Hotel | ||
| class Room | ||
| attr_reader :number | ||
|
|
||
| def initialize(number) | ||
| @number = number # should this be a string ? | ||
| end | ||
|
|
||
| # def check_availability(start_date, end_date) | ||
| # # Do..something with dates. | ||
| # # if date of check is within range | ||
| # # return boolean | ||
| # availability = true | ||
| # return availability | ||
| # end # of check_availability | ||
|
|
||
| end # of class Room | ||
| end # of module Hotel |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| Possible changes to Hotel: | ||
| - Reduce dependency of method for making Reservation on reserve instance variables? | ||
| - Split some of Hotel into smaller methods (like reserve_available_room) | ||
| - Combine methods that have overlapping but contradictory options? Combine or link. (ex: @reservations & @room_calendar) | ||
| - Change name for BookingManager instance(s) in tests? Sometimes listed as "hotel", other times "manager", etc. Change all to BookingSystem? | ||
| - Check more of edge cases | ||
| - Too many variables with 'reservation' in title? | ||
| - Add check that rooms are not overwritten |
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.
I like that you've broken each of these initialization helpers out as separate methods - the pattern is very clear.