Skip to content
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

Pipes - Sarah Read-Brown - Hotel #47

Open
wants to merge 61 commits into
base: master
Choose a base branch
from

Conversation

SRBusiness
Copy link

Hotel

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe a design decision you had to make when working on this project. What options were you considering? What helped you make your final decision? One thing we talked about doing in my group was handling the dates and date ranges in a more traditional calendar format, where we would create a whole year of dates. We ended up not doing that because we felt like it was a lot of work and that we could accomplish the same thing by searching through a collection of reservations vs having an object for each date.
Describe a concept that you gained more clarity on as you worked on this assignment. The block concept was really abstract to me in the beginning of this assignment and I didn't realize until the end how vital the room class and objects would be to its success. As I went about building the block class I was struggling with how to deal with rooms once they had been assigned to a block. After struggling for awhile I realized that I could add a state attribute to rooms and use that to store data on if rooms a block have been reserved.
Describe a nominal test that you wrote for this assignment. For reserving a room from within a block I wrote a test to make sure that the method would change the state of rooms from false (non reserved) to true (reserved)
Describe an edge case test that you wrote for this assignment. I wrote a test to make sure that user input for dates needed to be date objects.
How do you feel you did in writing pseudocode first, then writing the tests and then the code? For this project it was harder than previous ones to stick to writing the test code. I found that the sheer size of my reservations class made it hard not to get sucked into writing code first. For the smaller classes it was much easier to follow the order of operations listed in this question.

…or edge case anything that isn't a date object
@droberts-sea
Copy link

Hotel

What We're Looking For

Feature Feedback
Baseline
Used git regularly yes - good work
Answer comprehension questions yes
Design
Each class is responsible for a single piece of the program mostly - it seems like Reservations has eaten some of the responsibility that ought to be part of DateRange
Classes are loosely coupled yes - good work
Wave 1
List rooms yes
Reserve a room for a given date range yes
List reservations for a given date yes
Calculate reservation price yes
Invalid date range produces an error yes
Test coverage yes
Wave 2
View available rooms for a given date range yes
Reserving a room that is not available produces an error yes
Test coverage some - see inline comment
Wave 3
Create a block of rooms yes
Check if a block has rooms some - see inline comment
Reserve a room from a block yes
Test coverage yes
Additional Feedback

Good job overall. There are a couple places where your design could be improved, particularly around the interaction between Blocks and Rooms and in delegating responsibility to DateRange, but for the most part it seems well-thought-out and deliberate. Your specific method code is well-organized and easy to read, and your test coverage is solid. Keep up the hard work!


def valid_date?
valid_input?
if @check_in > @check_out

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: I like that these validations are called from your constructor, but I think the code would be a little more readable if they were invoked directly, rather than via make_nights_arr.

def update_room_state(rooms_to_book,num_rooms,state)
num_rooms.times do |i|
rooms_to_book[i].booked = state
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use an each loop here with the same effect.

range = DateRange.new(check_in,check_out).nights_arr
range.each do |date|
@all_reservations.each do |booking|
if booking.date_range.nights_arr.include?(date)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of looping through every date in the range, it might be a little cleaner to write a method DateRange#overlap?(other). Then this loop could be simplified to:

booked_rooms = []
range = DateRange.new(check_in,check_out)
@all_reservations.each do |booking|
  if booking.date_range.overlap?(range)
    # add rooms to booked_rooms
  end
end
return booked_rooms

It's doing the same work, but I would argue this version is more readable, and does a better job of separating responsibilities. It also makes testing easier!

@block_info = block_info
end

def rooms_available_block

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this method is already inside the Block class, you shouldn't need to add _block to the name.

@rooms.each do |room|
if room.booked == false || room.booked == nil
rooms_available << room
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method for keeping track of which rooms are reserved is an interesting idea, but I believe you will run into trouble if you have multiple blocks for different dates containing the same rooms. Because the Room instances are shared between blocks, marking a room reserved for one block will make it reserved for all blocks.

end

def reserve_from_block(block, num_rooms)
rooms_to_book = block.rooms_available_block

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like that you call a method on block here rather than manipulating its state directly. Good example of loose coupling.

arr_booked.length.must_equal arr_booked_unique.length
end
it "returns an empty array if nothing is booked" do
@hotel.check_reservations(@check_in + 1,@check_out + + 1).must_be_empty

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good coverage for this method.

@hotel.make_reservation(@check_in + 2, @check_out + 2,20)
end
describe "make a block " do
it "will create a block, " do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good use of a nested describe to organize things.

it "returns an empty array if everything is booked" do
@hotel.make_reservation(@check_in,@check_out,18)
@hotel.check_availability(@check_in,@check_out).must_be_empty
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a number of other cases I'd be interested in exploring around when a room is considered available. My full list includes:

  • Not available:
    • Same dates
    • Overlaps in the front
    • Overlaps in the back
    • Completely contained
    • Completely containing
  • Available:
    • Completely before
    • Completely after
    • Ends on the checkin date
    • Starts on the checkout date

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants