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

Lindsay - Edges - hotel #44

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

Conversation

elle-terch
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? I was originally trying to figure out how many classes to create. I started with a room class. Once I realized that the room class wasn't necessary for anything, I deleted it and handled all room assignments in the reservation_hub.
Describe a concept that you gained more clarity on as you worked on this assignment. I definitely gained more clarity on creating tests for methods. Before this assignment, I wasn't quite sure how to initialize objects so they can be tested. I learned how to rely on before-do blocks.
Describe a nominal test that you wrote for this assignment. I initialized 4 new reservations, with valid dates, and made sure that they would be added to the array containing all reservations.
Describe an edge case test that you wrote for this assignment. I wrote a test to confirm that the program would raise an error if the hotel was fully booked. So I started by creating 20 new reservations for the same date. Then I wrote a test that would expect an error if another reservation was added.
How do you feel you did in writing pseudocode first, then writing the tests and then the code? At first, it was incredibly challenging because it felt like I was coding blind. But once it started becoming habit, it got more comfortable. By the end, I found myself relying on my tests more than the implementation with binding.pry.

…he reservation class to the reservation spec class
@droberts-sea
Copy link

Hotel

What We're Looking For

Feature Feedback
Baseline
Used git regularly yes
Answer comprehension questions yes
Design
Each class is responsible for a single piece of the program some - see inline
Classes are loosely coupled yes
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 missing some - see inline
Wave 3
Create a block of rooms yes
Check if a block has rooms yes
Reserve a room from a block no - you can create a block of rooms, but there's no way to reserve a room from a block
Test coverage yes
Fundamentals
Names variables, classes and modules appropriately yes
Understanding of variable scope - local vs instance yes
Can create complex logical structures utilizing variables yes
Appropriately uses methods to break down tasks into smaller simpler tasks yes
Understands the differences between class and instance methods yes
Appropriately uses iterators and enumerables mostly
Appropriately writes and utilizes classes yes
Appropriately utilizes modules as a mechanism to encapsulate functionality yes
Wrap Up
There is a refactors.txt file yes
The file provides a roadmap to future changes yes
Additional Feedback

Good job overall! Your design for how to keep track of reservations is a little odd but it gets the job done, and experimenting with design is one of the goals for this project. It is important to me that you understand why I find it odd, and that you're comfortable dividing up responsibilities between classes - as is, everything landed in ReservationHub. It would be worthwhile to take a look at the instructor implementation of this project, and compare it with your own.

Outside of design, I like what I see. Your method code is solid, your test coverage is pretty close to where we want it to be, and it's clear to me that the learning goals for this assignment were met. Be sure to review my inline comments, and keep up the hard work!


@reservation_dates = reservation_dates
@room_id = room_id
@total_cost = reservation_cost(reservation_dates)

Choose a reason for hiding this comment

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

Rather than calculating the total cost up front and storing it in an instance variable, I would leave this as a behavior, a method is called whenever someone wants that information. That way if the number of nights or the cost changes, the total cost will automatically be correct - it will "just work".

def initialize
@reservations = []
@room_bookings = {1 => [], 2 => [], 3 => [], 4 => [], 5 => [], 6 => [], 7 => [], 8 => [], 9 => [], 10 => [], 11 => [], 12 => [], 13 => [], 14 => [], 15 => [], 16 => [], 17 => [], 18 => [], 19 => [], 20 => []}
@room_blocks = []

Choose a reason for hiding this comment

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

I would probably build the room booking list in a loop rather than hardcoding it.

Choose a reason for hiding this comment

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

I want to call out the way you keep track of which rooms are reserved on which dates. This strategy definitely works! However, it ends up duplicating information between this file and Reservation. If you needed to, for example, change the dates on a reservation, or delete a reservation, you would also have to remember to do work on the @room_bookings list here. This is a dependency, albeit a subtle one, and it will make your code more difficult to change.

Another option is to have the Reservations be the single source of truth regarding which rooms are reserved when. This file would have to loop through the reservations to find ones that overlap with the requested dates.

def add_reservation(start_date, end_date)

@start_date = start_date
@end_date = end_date

Choose a reason for hiding this comment

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

You've done a great job of using vertical whitespace (empty lines) to break up the code in this file, which makes it very easy to read.

@total_rooms = total_rooms
validate_dates

reservation_dates = create_date_array(start_date, end_date)

Choose a reason for hiding this comment

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

There are a couple of behaviors here that have to do with working with dates (validate_dates, create_date_array), which make me think that a separate class to encapsulate this behavior might be appropriate. You could even include the logic to check for an overlap.

@reservations.each do

if @reservations[index].reservation_dates.include?(date)
reservations_by_date << @reservations[index]

Choose a reason for hiding this comment

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

Instead of keeping a manual index here, you should use a block parameter:

@reservations.each do |reservation|
   if reservation.reservation_dates.include?(date)
     reservations_by_date << reservation
  end
end

Or better yet, notice that we're using the select pattern:

def find_reservations(date)
  return @reservations.select do |reservation|
    reservation.reservation_dates.include?(date)
  end
end


def initialize(reservation_dates, room_id)

@reservation_dates = reservation_dates

Choose a reason for hiding this comment

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

The name reservation_dates is a little redundant, since we're already in the Reservation class. Something like dates or date_range might be a better name.

it "raises an error if the hotel is fully booked" do

start_date = Date.new(2018,01,05)
end_date = Date.new(2018,01,06)

Choose a reason for hiding this comment

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

Nice, good work catching this edge case!

it "won't duplicate room ids if two reservations share the same dates" do

reservation_dates = [1,2,3]
room_id = @reservation_hub.assign_room(reservation_dates)

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 test cases around whether two ranges of dates overlap that you're missing here. See the slack message posted in #general.

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