Maddie Shields - Edges - Hotel

[madaleines]

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?	A decision I had to make was whether to have a Room class. Initially I had one which took a room number, I eventually removed it because it felt unnecessary to have a class with one attribute. I eventually put it back in when each room could have different rates. I made the decision based on the requirements asked for at the time.
Describe a concept that you gained more clarity on as you worked on this assignment.	A concept I received more clarity on was creating a class with helper methods which could be applied in the rest of the classes rather than having those helper methods in repeat code in those classes. I did this by creating a DateRange class which was used in the Reservation Tracker class and Reservation class to get a range, to see how many nights stayed in total, and to check if dates overlap each other as well.
Describe a nominal test that you wrote for this assignment.	When making an instance of Reservation, that it accurately calculates the total_cost based on num of nights stayed as well the rate and if it has a block_id assigned too.
Describe an edge case test that you wrote for this assignment.	To check the end_date was not before (or less than) the start_date.
How do you feel you did in writing pseudocode first, then writing the tests and then the code?	I often would begin the code portion before the tests and found as I wrote my tests I had to make changes to the code written. Over time I recognized if I wrote the tests first and then the code, I would have to make less changes and it was easier for me to understand the relationships between each class.

[droberts-sea]

Hotel

What We're Looking For

Feature	Feedback
Baseline
Used git regularly	mostly - you included the coverage directory in git, this should be ignored
Answer comprehension questions	yes
Design
Each class is responsible for a single piece of the program	mostly - 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	yes
Wave 3
Create a block of rooms	yes
Check if a block has rooms	yes
Reserve a room from a block	possibly a bug - see inline
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 - some places this is not perfect, but it's sufficient for where we're at
Appropriately writes and utilizes classes	yes
Appropriately utilizes modules as a mechanism to encapsulate functionality	yes
Wrap Up
There is a refactors.txt file	no
The file provides a roadmap to future changes	no
Additional Feedback	Great job overall! Your design is fairly clean, your method code is well-written, and your test coverage is solid. I'm way impressed by your work on the optionals too! As always there are places that things can be cleaned up, which I've tried to call out inline, but in general I am quite pleased with this submission. Keep up the hard work!

[droberts-sea]

The ternary on line 9 is redundant. You check if input[:block_id] is nil, if it is use nil explicitly otherwise use input[:block_id]. Why not just say @block_id = input[:block_id]?

[droberts-sea]

I like that you made all these custom exceptions. You probably don't want to make them private though, because then people outside this class won't be able to access it (for example to rescue those specific exceptions).

[droberts-sea]

I would argue that these checks around the dates ought to be part of the responsibility of the DateRange class.

[droberts-sea]

The convention in Ruby is that methods that end in a ? are expected to return either true or false. That's not quite what these are doing - raising exceptions is similar, but distinct, and the return value here isn't going to be a boolean.

[droberts-sea]

The name of this method is a little misleading: it finds a block by ID, it does not find a block ID. Something like find_block or find_block_by_id might be better.

[droberts-sea]

Don't you need to do something different here if this room is part of a block? For example, making sure that the block has a room, and taking that room from the block?

[droberts-sea]

You are going a little overboard here on type checking here. Trust whoever gives you this variable to give you a value you can use, and trust Ruby to throw an error if something really bad happens. See Metz ch 5 for more on this.

[droberts-sea]

I have several things to say here.

First, this is a great example of loose coupling. Rather than pulling out the dates and doing a bunch of math yourself, you call a method and let the date range take care of it. Right on!

Second, I would like to see an explicit return here (i.e. return instead of matching_reservations =, or end with return matching_reservations).

Third, I think you're somewhat misusing find_all. The block to find_all should produce a true or false value, whereas yours produces either the reservation (if the date range overlaps) or the nil (if it does not). This ends up working, but is not particularly clear. I would rewrite this loop as:

matching_reservations = @reservations.find_all do |reservation|
  reservation.date_range.overlaps?(requested_dates)
end

[droberts-sea]

Good work implementing this long list of tests, I appreciate your diligence.

[droberts-sea]

Good work getting this edge case!