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

Sockets-Chantal #29

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

Conversation

ChantalDemissie
Copy link

@ChantalDemissie ChantalDemissie commented Mar 18, 2019

Hotel

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What was a design challenge that you encountered on this project? It took me a long time to decide what to turn into classes. Many flow charts+pseudo-code+lists....
What was a design decision you made that changed over time over the project? I wanted to use arrays but ended up using sets and then converting to arrays when i needed too. besides that my design decisions stayed on path (after the long amount of design planning time!).
What was a concept you gained clarity on, or a learning that you'd like to share? I gained clarity on the usefulness of constants in test cases. Also the before clause was immensely helpful to dry up my code no matter what test i wrote.
What is an example of a nominal test that you wrote for this assignment? What makes it a nominal case?
What is an example of an edge case test that you wrote for this assignment? What makes it an edge case? fully booked as well as empty hotel is an edge case test. its unlikely that any hotel is fully booked or fully empty but there is a small chance it could happen so i believe that makes it an edge case
How do you feel you did in writing pseudocode first, then writing the tests and then the code? I did not find it helpful to write the tests and then the code. I need more practice doing that, I spent too much time writing tests that did not lead me to writing concise code. So i switched to pseduo coding, code and then testing for wave 2. Wave 3 i will try again by writing tests first and then code.

@droberts-sea
Copy link

droberts-sea commented Mar 27, 2019

Hotel

What We're Looking For

Feature Feedback
Baseline
Used git regularly This is getting better, but I still see some room for improvement. Your first two commits in particular are a little larger than I would like.
Answer comprehension questions Yes - great examples of edge cases
Design
Each class is responsible for a single piece of the program yes
Classes are loosely coupled mostly - see inline
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 mostly - see inline
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 Enumerable methods yes
Appropriately writes and utilizes classes yes
Appropriately utilizes modules as a namespace no
Wrap Up
There is a refactors.txt file no
The file provides a roadmap to future changes
Additional Feedback Great job overall. This is a solid, well-tested implementation of waves 1 and 2. I especially like the 2-class design, you've managed to keep it very clean. Keep up the hard work!

require_relative 'reservation'

class Hotel
NUM_ROOMS = 20

Choose a reason for hiding this comment

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

Should this class be in a module?

# Find room that's available.
available_rooms = find_available_rooms(start_date, end_date)
raise RuntimeError.new("there are no rooms available") if available_rooms.empty?
room_number = available_rooms.to_a.sample

Choose a reason for hiding this comment

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

Good call raising an error here - that seems like an appropriate way to indicate that the method can't continue.

Instead of raising a RuntimeError, best practice is to define your own appropriately named exception type that inherits from StandardError and raise that. The reason is that RuntimeError is very broad, and doesn't give much information about what went wrong. Many things could produce a RuntimeError, but a NoRoomsAvailableError tells the caller exactly what went wrong.

@reservations.each do |reservation|
if reservation.start_date < end_date && reservation.end_date > start_date
available_rooms.delete(reservation.room_number)
end

Choose a reason for hiding this comment

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

Here, Hotel reaches into the Reservation and looks at its data directly. This is both an example of tight coupling, and violation of the law of demeter (the two tend to go hand in hand).

A cleaner approach might be to define a method Reservation#overlap?(start_date, end_date) that contains the logic on line 36, and call that here:

@reservations.each do |reservation|
  if reservation.overlap?(start_date, end_date)
    available_rooms.delete(reservation.room_number)
  end
end

I know we hadn't read POODR ch 4 yet when you wrote this code, but this is what Metz is talking about when she says you should "ask for what instead of telling how".

let(:booked_hotel) do
booked_hotel = Hotel.new
(NUM_BOOKED_ROOMS).times do
booked_hotel.make_reservation(@seventh, @eleventh)

Choose a reason for hiding this comment

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

I really like this organization - having all of an empty, a mostly-booked and a fully-booked hotel available for your tests up front makes the process of testing much simpler.

def calculate_total_cost()
total_nights = @end_date - @start_date #probably convert to integer after test
total_nights = total_nights.to_i / (24 * 60 * 60)
ROOM_PRICE * total_nights

Choose a reason for hiding this comment

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

I like the decision to make this a method rather than storing the cost as an instance variable. That way if the number of nights or the cost per night were to change (say in a future version of the program), the total cost will automatically be correct.


it "make reservation for fully booked hotel" do
expect{fully_booked_hotel.make_reservation(@seventh, @eleventh)}.must_raise RuntimeError
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 questions around when specifically a room is available that these tests don't cover. In particular, if a hotel is fully booked for one range of dates and you get another request...

No room is available if the dates:

  • Same dates
  • Overlaps in the front
  • Overlaps in the back
  • Completely contained
  • Completely containing

The room is available if the dates:

  • 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