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

Daniela Sanchez - Leaves #28

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

Daniela Sanchez - Leaves #28

wants to merge 8 commits into from

Conversation

dnsanche
Copy link

@dnsanche dnsanche commented Sep 9, 2019

Hotel

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What was a design challenge that you encountered on this project? A design challenge was to write flexible code that could change later in the next waves.
What was a design decision you made that changed over time over the project? At first, I thought to create a rooms class, but later, I decided to delete it as I didn't really use it for anything.
What was a concept you gained clarity on, or a learning that you'd like to share? I gained more clarity on the use of instance variables, methods over the classes.
What is an example of a nominal test that you wrote for this assignment? What makes it a nominal case? A nominal test would be the one I did to check the rooms assigned after making two reservations for same date range. It was a nominal case because I was expecting to have assigned the room 1 to the first reservation and 2 to the second reservation and that was supposed to happen.
What is an example of an edge case test that you wrote for this assignment? What makes it an edge case? An edge case I wrote was when the user tries to make a reservation and all rooms are reserved for that date-range. It should raise an argument error.
How do you feel you did in writing pseudocode first, then writing the tests and then the code? Good, as it gave me the idea of how I should implement my code and what my tests should check.

@dHelmgren
Copy link

Hotel

What We're Looking For

Test Inspection

Workflow yes / yes but no test / no
Wave 1
List rooms yes
Reserve a room for a given date range yes
Reserve a room (edge case) no.
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
Wave 3
Create a block of rooms yes
Check if a block has rooms yes
Reserve a room from a block no/didn't see one
Test coverage missing a line with an exception

Code Review

Baseline Feedback
Used git regularly I cannot stress this enough: you need to use git more. A single check in for a whole wave in hotel is not sufficient. Too few git check-ins can seriously harm your future projects, particularly something with a large scope like bEtsy.
Answer comprehension questions yes
Design
Each class is responsible for a single piece of the program no, your hotel manager is reading data from inside Block and Reservation and making calculations for those classes.
Classes are loosely coupled No, due to the above.
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 Mostly. Your Hotel class does the work, and your other two classes are essentially just data holders.
Appropriately utilizes modules as a namespace yes
Wrap Up
There is a refactors.txt file no
The file provides a roadmap to future changes no

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 broke down these large problems into bite-sized methods. I can very clearly see your thinking and follow along with your code!

I do see some room for improvement around your git usage, and more thorough testing. You seem to be missing a couple of important cases, and it might be easier to test those cases from within something like the block or reservation class. Also, use what we talked about in our discussion of POODR chapter 4 to use delegation more extensively, so that Hotel doesn't do all the work.

block.rooms
end
else
raise ArgumentError.new("Invalid block")

Choose a reason for hiding this comment

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

This line doesn't get hit by your tests.

attr_reader :check_in, :check_out, :rooms, :discounted_rate

def initialize(check_in, check_out, rooms = [], discounted_rate)
@check_in = check_in

Choose a reason for hiding this comment

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

When a class only has instance variables and no methods, either it doesn't need to be a class, or you're doing that class's work somewhere else. This class should be able to make its own reservations, and report information about when it is available.

(check_in...check_out).each do |date|
if @reservations_by_date[date] != nil
@reservations_by_date[date].each do |reservation|
available_rooms[reservation.room - 1] = false

Choose a reason for hiding this comment

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

This code is looking inside each of the reservations to make its decision, so it is probably doing something outside its responsibilities.

def validate_less_6_rooms(num_rooms)
unless num_rooms <= 5
raise ArgumentError.new("A block can contain a maximum of 5 rooms")
end

Choose a reason for hiding this comment

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

You're validating for the block here.

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