-
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
Dionisia - Edges - Hotel #37
base: master
Are you sure you want to change the base?
Conversation
…ed with passing test
HotelWhat We're Looking For
Dionisia! You did a great job with this project. I think the code is really clean and logical -- I am impressed by how slim ReservationTracker turned out, and I'm pleased with that! I think that your ability to feel comfortable in Ruby (using I'm adding a bunch of comments that I think will amplify... elevate ... your code style. Overall, I feel that your tests are lacking. I think you acknowledge that based on your comprehension question answers, but I just wanted to say it out loud! It's just a shame because I think the code style is great, but I can't be confident that it's working as required because of the lack of tests. Otherwise, the code looks great... If the code actually works accurately, then the code looks really great. To be quite clear: I don't trust that much of this code works as you'd like it to/there are more bugs than you think! But code-wise and design-wise, I think this is a great start. Good job! |
BLOCKRATE = 100 | ||
|
||
def initialize(check_in_date, check_out_date, block_of_rooms) | ||
@id = SecureRandom.uuid |
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 think your use of the SecureRandom
gem here to generate an ID is rad!
|
||
def hotel | ||
return "hotel" | ||
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.
Whoops! You should probably delete this file and all references to it.
@check_in_date = check_in_date | ||
@check_out_date = check_out_date | ||
@room_number = room_number | ||
raise StandardError, 'The end date cannot be before the start date.' if check_out_date <= check_in_date |
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 love that the logic to check if reservation dates are valid are in the Reservation itself. Nice decision :)
|
||
# module Hotel | ||
class Reservation | ||
attr_reader :check_in_date, :check_out_date, :room_number, :nights_stayed |
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.
attr_reader
s are reserved for names of instance variables. :nights_stayed
isn't the name of an instance var, it's the name of a method -- it doesn't need to be in this list!
def initialize | ||
@rooms = create_rooms | ||
@reservations = [] | ||
@unreserved_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.
Does this instance variable @unreserved_rooms
ever get used? If it doesn't get used, can we get rid of it?
|
||
# accesses the list of reservations for a specific date | ||
def list_of_reservations(date) | ||
return @reservations.find_all { |reservation| reservation.nights_stayed == date } |
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 this code actually work? It looks fishy to me, and I can't tell because there aren't any tests! :(
# set the rooms_not_reserved method and arguments to a local variable. | ||
unreserved_rooms = rooms_not_reserved(check_in_date, check_out_date) | ||
if unreserved_rooms.length == 0 | ||
raise ArgumentError, 'No Available Rooms for Given Dates' |
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.
Instead of raising an ArgumentError, it might make more sense to make your own custom error or just raise StandardError
!
def rooms_not_reserved(check_in_date, check_out_date) | ||
# if the reservations array is empty, this returns an array of available room numbers. If there are no reservations, all rooms can be reserved. | ||
return @rooms.map { |room| room.room_number } if @reservations.empty? | ||
unreserved_rooms = @rooms.map { |room| room.room_number } |
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 how dense and juicy and exciting the above two lines are!
# checks to see if a room is available and reserves the first available room for a given date range. Uses the rooms_not reserved method. | ||
def reserve_room(check_in_date, check_out_date) | ||
# set the rooms_not_reserved method and arguments to a local variable. | ||
unreserved_rooms = rooms_not_reserved(check_in_date, check_out_date) |
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 love this line! Very clear what's going on here.
# if @reservation.empty? is not empty | ||
@reservations.each { |reservation| | ||
if (reservation.check_in_date >= check_in_date && reservation.check_in_date <= check_out_date) || (reservation.check_out_date >= check_in_date && reservation.check_out_date <= check_out_date) | ||
unreserved_rooms.delete(reservation.room_number) |
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 is a very funny assumption you're putting here:
For the call .delete
to work, this assumes that the array unreserved_rooms
is an array full room numbers (originally stemming from @rooms
) that are unique. If they aren't unique, then this call to delete
might not do what you expect... so if the requirement changes that the hotel does not have unique room numbers, this may introduce bugs here.
|
||
@blocks.each { |block| | ||
if (block.check_in_date >= check_in_date && block.check_in_date <= check_out_date) || (block.check_out_date >= check_in_date && block.check_out_date <= check_out_date) | ||
unreserved_rooms - block.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 is cool code here! I like it. However... the -
isn't destructive. It just does the subtraction, and the result isn't stored anywhere. Observe in irb:
a = [1,2,3]
b = [1,2]
a - b
# => returns [3], as expected
# a is still [1,2,3]
# b is still [1,2]
So the subtraction doesn't do anything!
def reserve_room(check_in_date, check_out_date) | ||
# set the rooms_not_reserved method and arguments to a local variable. | ||
unreserved_rooms = rooms_not_reserved(check_in_date, check_out_date) | ||
if unreserved_rooms.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.
The indentation here is off ;)
Hotel
Congratulations! You're submitting your assignment!
Comprehension Questions