-
Notifications
You must be signed in to change notification settings - Fork 45
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
Carets- Julia Meier- Hotel #43
base: master
Are you sure you want to change the base?
Conversation
… started outlining rooms.rb
… initialize. Removed self.all method. Added list_room method- passes tests
…es first test for initialization
… a new reservation instance
… reservations in the reservations_array
…stance variable of hotel that can be used for all the tests
… of a reservation. Added a Custom_Exceptions file
…. It passes the first two tests
…ount for search dates that fell exactly on the checkin and checkout dates of existing reservations.
…e an available room was incorrect).
… available to reserve
…arranged the order of methods
….added a status attribute to the reservations class and altered find_rooms_available method in the hotel class to consider the status attribute in assessing a room's availability
…osnideration of the status in the find_rooms_available method.
…hich there are no block reservations
…or a block. I added a new error if the user attempts to make a block reservation for greater than 5 rooms. Passes initial tests
…ation- the change of the reservation status from unassigned to assigned is not sticking.
…otential matches for block partynames (since we need to know the exact name and spelling to look up block information)
HotelWhat We're Looking For
|
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.
A few in-code notes.
lib/reservation.rb
Outdated
|
||
attr_accessor :check_in_date, :check_out_date, :room, :HOTEL, :cost, :status, :block_reserved | ||
|
||
HOTEL = Hotel_Chain::MyHotel.new |
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.
This looks like you can only have ONE hotel EVER.
module Hotel_Chain | ||
class Reservation | ||
|
||
attr_accessor :check_in_date, :check_out_date, :room, :HOTEL, :cost, :status, :block_reserved |
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.
You have a lot of attr_accessor
methods here, but they don't all get set values, like cost
. How does that get calculated?
You should either have a method that does the calculation for the cost of the reservation or set the cost in the initialize
method.
lib/hotel.rb
Outdated
reservations_on_date.each do |reservation| | ||
array << "Room #{reservation.room.room_id} is reserved from #{reservation.check_in_date} to #{reservation.check_out_date}" | ||
end | ||
ap array |
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.
When you turn things in, it's best to take out extraneous puts
or ap
commands. It makes running things more messy than it would be otherwise.
lib/hotel.rb
Outdated
require 'pry' | ||
|
||
module Hotel_Chain | ||
class MyHotel |
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.
Why not call this Hotel
instead of MyHotel
? MyHotel
sounds like it's an individual Hotel
instance rather than a class.
lib/hotel.rb
Outdated
module Hotel_Chain | ||
class MyHotel | ||
|
||
attr_reader :array_of_rooms, :reservations_array, :blocks_array |
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.
You should probably just name these, rooms
, reservations
, or blocks
because you might in the future change how you implement these data structures and you want to separate implementation from how they're used.
Also do you want to give the user direct access to the rooms and reservations and blocks, instead of making them go through your methods. With this they could change them directly bypassing MyHotel
methods.
lib/hotel.rb
Outdated
def find_reservations_by_date(date) | ||
reservations_on_date = [] | ||
@reservations_array.each do |reservation| | ||
if (reservation.check_in_date...reservation.check_out_date).cover?(Date.strptime(date, "%m/%d/%Y")) |
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.
Just a note, this makes a new Date
object each time the loop iterates. You instead create one before the loop and then use it here.
Just a bit of optimization.
More seriously, this method works directly with the Reservation object's data attributes, instead this logic should go into the Reservation class and Hotel should simply ask, ""Hey does this date overlap with you?" using an instance method and should get back a true/false response.
This is an example of tight coupling.
lib/hotel.rb
Outdated
new_reservation.room = @array_of_rooms[0] | ||
@reservations_array << new_reservation | ||
return new_reservation | ||
elsif @reservations_array.length > 0 |
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.
This elseif is unnecessary, else
would be enough since if it's not 0 it's greater than 0.
end | ||
|
||
def find_rooms_available(check_in_date, check_out_date) | ||
unavailable_rooms = [] |
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.
This method seems overly complicated.
|
||
def reserve_block(party_name, check_in, check_out, no_of_rooms, room_rate) | ||
if no_of_rooms > 5 | ||
raise ExceededRoomLimitForBlocksError |
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 Error.
else | ||
no_of_rooms.times do |room| | ||
new_reservation = store_reservation(check_in, check_out) | ||
new_reservation.room.rate = room_rate |
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.
room rate should be passed in when you create the reservation.
Hotel
Congratulations! You're submitting your assignment!
Comprehension Questions