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

Branches - Amal #39

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

Branches - Amal #39

wants to merge 25 commits into from

Conversation

ashassan
Copy link

@ashassan ashassan 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? At first, I was confused about when I should be making a class. I ended up making too many classes, and I had to revaluate my design. I then started to get confused about where to place methods and the composition of the classes.
What was a design decision you made that changed over time over the project? The number of classes I created changed over time.
What was a concept you gained clarity on, or a learning that you'd like to share? I gained more clarity on writing tests and using simplecov to make sure that my tests were covering everything.
What is an example of a nominal test that you wrote for this assignment? What makes it a nominal case? A nominal test I wrote is that my method create a reservation returns a reservation. It is a nominal test because I am checking that the method works the way I expect with normal inputs.
What is an example of an edge case test that you wrote for this assignment? What makes it an edge case? An edge case test I wrote is for invalid date ranges. It's an edge case because the input is not the normal input and we are checking if it handles anyways.
How do you feel you did in writing pseudocode first, then writing the tests and then the code? It was still hard for me to visualize my code when writing the tests. I did not write tests first the whole time, but when I did it felt less weird. I want to get better at writing tests before code. Perhaps writing good pseudocode will help me overcome my visualization problems when writing tests.

@jmaddox19
Copy link

Hotel

What We're Looking For

Test Inspection

Workflow yes (X) / yes but no test / no
Wave 1
List rooms X
Reserve a room for a given date range X
Reserve a room (edge case) X
List reservations for a given date X
Calculate reservation price X
Invalid date range produces an error X
Test coverage Great!
Wave 2
View available rooms for a given date range Yes, but there's one weird edge case in the test that doesn't seem to be working right. (See code comments)
Reserving a room that is not available produces an error Yes but no test
Test coverage Good for the most part. One notable test missing. (See above)
Wave 3
Create a block of rooms No
Check if a block has rooms No
Reserve a room from a block No
Test coverage N/A

Code Review

Baseline Feedback
Used git regularly Good!
Answer comprehension questions Yes
Design
Each class is responsible for a single piece of the program X
Classes are loosely coupled X
Fundamentals
Names variables, classes and modules appropriately X
Understanding of variable scope - local vs instance X
Can create complex logical structures utilizing variables X
Appropriately uses methods to break down tasks into smaller simpler tasks X
Understands the differences between class and instance methods X
Appropriately uses iterators and Enumerable methods X
Appropriately writes and utilizes classes X
Appropriately utilizes modules as a namespace No but not a bid deal :)
Wrap Up
There is a refactors.txt file Yes
The file provides a roadmap to future changes Yes

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 tested lots of specific details about the results from methods.

hotel.create_reservation(4, Date.new(2019, 2, 11), Date.new(2019, 2, 14))
hotel.create_reservation(5, Date.new(2019, 2, 11), Date.new(2019, 2, 14))
available_rooms = hotel.find_available_room(Date.new(2019, 2, 5), Date.new(2019, 2, 9))
expect(available_rooms[0]).must_equal 3

Choose a reason for hiding this comment

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

I love how thorough your tests are here with very specific expectations!
I'm confused though why room 3 is returned when it's already reserved starting on the 8th and the request has a checkout date of the 9th.

expect(hotel.reservations[1].length).must_equal 1
expect(hotel.reservations[2].length).must_equal 1
end
it "checks that new reservations can start on checkout days" do

Choose a reason for hiding this comment

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

I wanna point out that there's some inconsistency around naming. Some of your test names describe what the source code should do and some describe what the test does. Conventionally, the former is preferred.

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