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

Leaves - Emily Vomacka #26

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

Conversation

emilyvomacka
Copy link

Hotel

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What was a design challenge that you encountered on this project? Deciding which class should do the bulk of the work. At first I thought Reservation would do most heavy lifting, with Hotel's only function being to check room availability, but I ended up shifting most work to Hotel as it involved less messaging between classes.
What was a design decision you made that changed over time over the project? I thought that Block would be a type of/child of Reservation, but realized as I got deeper into the functionality required that this didn't make much sense--the two classes are too different.
What was a concept you gained clarity on, or a learning that you'd like to share? I discovered how powerful it is to combine enumerables for advanced filtering. I also had the rewarding moment of writing a bunch of logic to determine whether two ranges overlap, deciding there must be an already-existing function, and discovering grep in the Ruby documentation.
What is an example of a nominal test that you wrote for this assignment? What makes it a nominal case? I tested to make sure a Reservation could be created under completely normal circumstances--unproblematic date range, no conflicting reservations. It's nominal because I'm testing whether the program functions correctly with expected and "correct" input.
What is an example of an edge case test that you wrote for this assignment? What makes it an edge case? I tested whether a reservation could be made when a previous reservation existed that ended on the same day the new reservation would begin. It's an edge case because it's borderline to data that should cause the program to produce an error.
How do you feel you did in writing pseudocode first, then writing the tests and then the code? I did well on the pseudocode front, but could be more rigorous in writing my test first and code second.

@kaidamasaki
Copy link

kaidamasaki commented Sep 16, 2019

Hotel

What We're Looking For

Test Inspection

Workflow yes / yes but no test / no
Wave 1
List rooms 👍🏼 (but no test)
Reserve a room for a given date range 👍🏼
Reserve a room (edge case) 👍🏼
List reservations for a given date 👍🏼
Calculate reservation price 👍🏼
Invalid date range produces an error 👍🏼
Wave 2
View available rooms for a given date range 👍🏼
Reserving a room that is not available produces an error 👍🏼
Wave 3
Create a block of rooms 👍🏼
Check if a block has rooms 👍🏼
Reserve a room from a block 👍🏼

Code Review

Baseline Feedback
Used git regularly 👍🏼
Answer comprehension questions 👍🏼
Test coverage 👍🏼
Design
Each class is responsible for a single piece of the program 👍🏼
Classes are loosely coupled 👍🏼
Fundamentals
Names variables, classes and modules appropriately 👍🏼
Understanding of variable scope - local vs instance 👍🏼
Can create complex logical structures utilizing variables 👍🏼
Appropriately uses methods to break down tasks into smaller simpler tasks 👍🏼
Appropriately uses iterators and Enumerable methods 👍🏼
Appropriately writes and utilizes classes 👍🏼
Appropriately utilizes modules as a namespace 👎🏼 (Didn't use a module.)
Wrap Up
There is a refactors.txt file 👍🏼
The file provides a roadmap to future changes 👍🏼

Overall Feedback

Great work overall! You've built your first project with minimal starting code. This represents an incredible milestone in your journey, and you should be proud of yourself!

I am particularly impressed by the way that you called out your edge cases specifically in your tests and were very thoughtful about splitting code up into methods.

I do see some room for improvement around modules, and also your date handling is a bit wonky. But really, those are quite small things, well done!

class DateRange
attr_reader :start_date, :end_date, :range

def initialize(start_year, start_month, start_day, end_year, end_month, end_day)

Choose a reason for hiding this comment

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

It's reasonable to just accept to Date objects here and require the user to do the parsing/building themselves.

end
block_rooms = available_rooms(block_range).sample(room_quantity)
if block_rooms.length < room_quantity
raise ArgumentError, "The hotel does not have a sufficient number of empty rooms to book this block."

Choose a reason for hiding this comment

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

Since there's nothing wrong with the arguments themselves this shouldn't be an ArgumentError; the user can't change their arguments to make a room available.

This should be a custom error or something like a StandardError.

-Track reservations in a hash using name as a key, to make look up faster
-Figure out how to eliminate reservations from past dates, so the system doesn't become overloaded as time progresses.
-Allow more aspects of a reservation to be edited. (change stay duration, for example)
-Eliminate or reduce dependencies as I understand them more

Choose a reason for hiding this comment

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

It's impossible to eliminate dependencies unless you put everything in one file. Dependencies are okay, it's just important to be mindful when you decide to add one since they have a cost.

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