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- Roxanne Agerone - Hotel #41

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

Conversation

RAgerone
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? There was an option to do a date range class. There was also an option to make my mixins classes. I could have done that. It may have made things easier, but I was excited to learn new concepts.
Describe a concept that you gained more clarity on as you worked on this assignment. Isolation. I found it hard to change features when they weren't isolated.
Describe a nominal test that you wrote for this assignment. I checked to see that the reservations were being made into instances of reservations.
Describe an edge case test that you wrote for this assignment. I tested to see that there were enough rooms. If the rooms were all booked, an Argument Error was raised.
How do you feel you did in writing pseudocode first, then writing the tests and then the code? I felt good about it, but when all of these concepts are better cemented in my mind, I'll feel more confident

@PilgrimMemoirs
Copy link

Hotel

What We're Looking For

Feature Feedback
Baseline
Used git regularly Well Done - avoid commit messages like 'minor changes', especially in a sequence of 3
Answer comprehension questions Well Done
Design
Each class is responsible for a single piece of the program Good Choice in selection of classes. The only one that is unnecessary is room (see note below). The modules do add complexity to this program that result in tracing functionality between classes more difficult. See notes below on use of modules.
Classes are loosely coupled Mostly Good - Block doesn't seem to fit as a direct inheritor of Reservations. There are just enough differences in attributes where I would keep the classes separated.
Wave 1
List rooms Well Done
Reserve a room for a given date range Well Done - though method should be defined directly in Admin class and be tested in admin_spec (see note on when to use modules below).
List reservations for a given date Well Done
Calculate reservation price Well Done
Invalid date range produces an error ❗️ Does not do
Test coverage Tests for classes should test all functionality, including methods from modules. You want to test to make sure those instance methods are being successfully included into it. So you should still have methods defined in modules tested on an instance of Admin within admin_spec. (instead of having within reservable_spec)
Wave 2
View available rooms for a given date range Well Done
Reserving a room that is not available produces an error Well Done
Test coverage Well Done
Wave 3
Create a block of rooms Well Done
Check if a block has rooms ❗️Method does not exist
Reserve a room from a block ❗️ Method does not exist
Test coverage Good Start - Looks like every method does have a single test, but that one test will only test part of that functionality. Often you will want to have several tests for one method. ex: change_discount, also make sure the cost of rooms booked in that block have that discount applied. For nights reserved, make sure that it has all nights by length or checking that all dates are included.
Additional Feedback
tests some tests handling too much, good rule is that only one testing method, like .must_equal, should be called per it block. Individual tests should be as specific as possible.
When to Use Modules Only use a module (as a wrapper for methods) if those methods are going to be used in more than one class and if inheritance does not make sense. In the case of the Hotel::Reserveable::InstanceMethods and Hotel::Blockable::InstanceMethods, the methods defined inside of those modules are not being used anywhere except in the Admin class - therefore those methods should be defined directly in the Admin class itself. A good use of a module is for the finder method, since it can be used across many classes. Having a lot of the methods split between modules also makes it more difficult to know what functionality a class has. There were several times I had to do a lot of work to track down where a method was defined.
How to use Modules When you do have methods inside of a module that is included into a class (like Hotel::Dateable::InstanceMethods into the Reservation class), that class then has those methods available to call on instances. Meaning that the method populate_date_range, is not necessary because it is not doing anything differently than calling get_date_range directly.
When to use a class Is the Room class necessary? It is only storing a single attribute, number and has no additional functionality, or methods. Is there any advantage to having a room represented as a object vs just the integer? In this case, no. Because nothing beyond storing the number is being done.
Block Should specify what rooms (by their number) are to be included in block - not just number of rooms. If it's setup like this, there is no way to reserve a specific room from a block or a room would get booked anywhere in the hotel and not necessary next to another room in this block. The idea is that a block should group rooms close to each other for a group to reserve individually.

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