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.
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.
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.
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.
attr_readers 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.
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.
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.
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.
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.
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.
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.
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 |
Hotel
Congratulations! You're submitting your assignment!
Comprehension Questions