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

Michelle - Edges - Hotel #31

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

Conversation

kangazoom
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 originally had a Room class that kept track of the room number, maybe cost (I waffled on this a lot), and a list of Reservation instances connected to that particular room. It was instantiated in the BookingSystem class. However, I soon realized that the Room class didn't DO very much and really only kept track of its state (no behavior). I ultimately made the decision to cut it loose and instead add rooms as integers that could be assigned to reservations (and later, room blocks) in BookingSystem.
Describe a concept that you gained more clarity on as you worked on this assignment. I had heard that other people created a DateRange type class after speaking with Dan. This occurred around the same time that I had started to realize that BookingSystem was doing too much. There were a lot of date-related activities it was keeping track of, which aided its booking behaviors but were not directly part of those booking behaviors. I spent a night trying to implement it, but ultimately reverted my commit back to erase my new code since it wasn't working out and things had gotten messy. A few days later, I tried again, and very intently only added one functionality and test at a time. Working in this way was helpful. So two doses of clarity in one!
Describe a nominal test that you wrote for this assignment. A lot! Like, if we are checking new dates but they fall squarely within already-established check-in/check-out dates, then we would expect there to be an overlap.
Describe an edge case test that you wrote for this assignment. I wrote a few edge cases, like what happens when we have a one-night reservation or a reservation that begins when another reservation ends?
How do you feel you did in writing pseudocode first, then writing the tests and then the code? Not great. I don't think I understand how to write in pseudocode in a way that helps me. A lot of times, I'll use some pseudocode/pre-planning/sketching to figure out a game plan, only to have a bunch of hindering surprises ruin my plan once I set to work. I'm not sure how much of it is actually my under-preparing vs the inability to properly grasp the issues without getting my hands dirty first.

…so added a method to find reservations that match a certain date
…pt hash-like input + began similar input-related work on Reservation#initialize
…rved. Also added error handling for invalid end date
…ember now :( prob Reservation or BookingSystem
@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
Classes are loosely coupled
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 yes
Wave 3
Create a block of rooms yes
Check if a block has rooms yes
Reserve a room from a block no - given a block, there's no way to reserve a single room from it and have that room appear in the list of reservations
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 yes
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 Great job overall! Your design feels clean, your method code is well-written, and your test coverage is in a good place. There are as always a few places where things could be cleaned up, which I've tried to call out inline, but in general I am quite happy with what I see. Keep up the hard work!

def list_res_for_date(check_date)
matching_res = @reservations.select { |reservation| reservation.has_date?(check_date) }

return matching_res.empty? ? nil : matching_res

Choose a reason for hiding this comment

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

Good use of an enumerable here.

For the return value, I think that an empty array fits better than nil here. An empty array is typically used to indicate "I found no matches" when you're returning a collection (it's a collection with no members). nil is used when you're supposed to return one instance.

def list_avail_rooms_for_range(check_in:, check_out:)
date_range = construct_cal_checker(check_in: check_in, check_out: check_out)

booked_rooms = @reservations.select { |reservation| reservation.overlap?(date_range) }.map { |reservation| reservation.room_num }

Choose a reason for hiding this comment

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

I would probably break this complex string of enumerables out across multiple lines.

Choose a reason for hiding this comment

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

This method has a couple of excellent examples of where your design allowed you to have loose coupling between classes. For example, when you loop through the list of reservations or blocks, instead of BookingSystem knowing about the details of how a Reservation stores dates, it calls a method and lets Reservation take care of the details.


if !held_rooms.empty?
held_rooms = held_rooms[0] # list within a list
end

Choose a reason for hiding this comment

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

This doesn't quite do what you want. If you have two overlapping blocks, then you'll end up with an array like this: [[1, 2, 3], [7, 8, 9]], but your code will only grab the first sub-array.

The array method .flatten does what you're looking for.

def create_reservation(check_in:, check_out:)
id = generate_res_id() #<-- create new reservation id
avail_rooms = list_avail_rooms_for_range(check_in: check_in, check_out: check_out) #<-- grab first available room

Choose a reason for hiding this comment

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

I like that you've broken out finding the list of available rooms as a separate helper method. That makes reading this method much easier.

class Calendar
attr_reader :check_in, :check_out

def initialize(check_in:, check_out:)

Choose a reason for hiding this comment

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

I like that you've made this its own class, but I'm not sure I agree with the name Calendar. Calendar implies that it's tracking a list of events, when this just tracks a start and end date. Something like DateRange or CalendarBooking might be a better name.

def overlap?(other_dates) # param: Calendar obj
if !(other_dates.check_in <= @check_out-1) || !(other_dates.check_out-1 >= @check_in)
return false
else

Choose a reason for hiding this comment

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

Instead of subtracting 1 here and using <=, you can use <.

Choose a reason for hiding this comment

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

Since the expression on line 31 will always produce true or false, you don't need an if statement. You could write: return !(other_dates.check_in <= @check_out-1) || !(other_dates.check_out-1 >= @check_in)


unless @check_in < @check_out
raise StandardError, "Invalid date range: end date must occur after start date."
end

Choose a reason for hiding this comment

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

Instead of raising a StandardError, best practice is to define your own appropriately named exception type that inherits from StandardError and raise that. The reason is that StandardError is very broad, and if the code that calls this method has to rescue StandardError, that will catch things like ArgumentErrors and NameErrors that indicate true bugs.

it "returns nil if no rooms are available" do
20.times do |i|
res = Hotel::Reservation.new(id:1, room_num: i+1, check_in: "1999-1-1", check_out: "1999-12-31")
booking_system.reservations << res

Choose a reason for hiding this comment

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

Good, I like this edge case.

describe "#list_res_for_date" do
# see Calendar#has_date? for additional tests
let(:res_1) {Hotel::Reservation.new(
id: "1",

Choose a reason for hiding this comment

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

What about reservations that came through a block?

describe "#overlap?" do
it "returns false for no overlap: other dates begin and end before reserved dates" do
other_dates = Hotel::Calendar.new(check_in: "1986-07-01", check_out: "1986-07-15")

Choose a reason for hiding this comment

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

Good work implementing these test cases. The list may have come from us originally, but you are the one who did the work of turning them into real tests, and that diligence is appreciated.

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