-
Notifications
You must be signed in to change notification settings - Fork 46
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
hotel- Leanne Rivera #32
base: master
Are you sure you want to change the base?
Conversation
HotelWhat We're Looking For
|
|
||
module Lodging | ||
|
||
class Concierge |
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.
Love the name, but maybe describe the functionality more clearly. BookingManager
or something similar.
module Lodging | ||
|
||
class Room | ||
attr_reader :room_number, :status, :cost |
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.
Does a room need a status?
room = [@room_number, @cost, @status, Array.new] | ||
|
||
CSV.open('data/all_hotel_rooms.csv', 'a+') do |row| | ||
row << room |
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 initialize
method is reading in all the hotel rooms for each hotel room. This doesn't seem to make sense.
lib/room.rb
Outdated
end | ||
|
||
def self.available_room #to return one room with status available | ||
CSV.open('data/all_hotel_rooms.csv', 'r', headers: true) do |row| |
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 just reads in the CSV file and returns the 1st row. That doesn't make sense to me given the method name.
lib/concierge.rb
Outdated
|
||
raise ArgumentError if Date.parse(check_out) < Date.parse(check_in) #checkout cannot be earlier than check in | ||
|
||
book_this = Lodging.room_status(@rooms_info, check_in) #returns one available room |
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.
What is the purpose of Lodging
if your Concierge
is keeping track of the rooms. Why not give it the job of finding an available room.
Also why name the method room_status
instead of available_room
.
room[:room_number] == book_this[:room_number] | ||
end #finds room to be reserved | ||
|
||
update_room[:status] = "unavailable" #changes status of said room in the array of hashes as 'unavailable' |
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 room isn't going to have 1 status. A room could be available in one date range and unavailable in another.
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 thought room status checked for this with the conditional.
block.times do | ||
hold_this = Lodging.room_status(@rooms_info, check_in) #returns one available room | ||
|
||
update_room = @rooms_info.find do |room| |
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 a good candidate for a helper method.
|
||
new_cost = (update_room[:cost].to_f) - (update_room[:cost].to_f * discount) | ||
|
||
update_room[:cost] = new_cost.round(2).to_s |
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.
If you're giving a room a different cost per reservation, that value should apply to each reservation or booking, not to the room itself. A room could have a default cost, but now if a room is put into a block, the cost for past reservations go down.
require 'awesome_print' | ||
|
||
module Lodging | ||
|
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.
These methods look like they should go into Concierge
instead of a separate module.
|
||
expect(tiny_hotel.all_rooms[0][:reserved_dates].length).must_equal 7 | ||
|
||
end |
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.
What about testing if you can do multiple reservations.
What happens if you reserve rooms and no room is left available.
These things should be tested!
Hotel
Congratulations! You're submitting your assignment!
Comprehension Questions