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 - Yitgop #37

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

Leaves - Yitgop #37

wants to merge 14 commits into from

Conversation

rinostar
Copy link

@rinostar rinostar 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? Trying to balance between clear logic flow among classes and the new design knowledges from POODR.
What was a design decision you made that changed over time over the project? Over the course of the project, the DateRange class seems to serve no functions other than hold check in and check out time for reservations. Therefore, I decided to merge that function to Reservation, and got ride of DateRange class.
What was a concept you gained clarity on, or a learning that you'd like to share? I understand the interaction of different classes within the same module better now.
What is an example of a nominal test that you wrote for this assignment? What makes it a nominal case? You can't make an reservation when check out date is earlier than check in date. It is a nominal case because the parameter of check in and check out dates expect Date instances input.
What is an example of an edge case test that you wrote for this assignment? What makes it an edge case? When the argument for Booker's "inventory" parameter is non-integer, it will raise an ArgumentError. It is a edge case because the parameter doesn't not expect a integer input.
How do you feel you did in writing pseudocode first, then writing the tests and then the code? It definitely took longer in the beginning, but provided better workflow and design opportunities later on.

@beccaelenzil
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) yes
List reservations for a given date yes
Calculate reservation price yes
Invalid date range produces an error yes
Test coverage 100% - You should not commit your coverage directory to github. Add this directory to git ignore.
Wave 2
View available rooms for a given date range yes
Reserving a room that is not available produces an error yes
Test coverage booker, 93.85% -- try to see what you're missing.. is it necessary code?
Wave 3 N/A
Create a block of rooms N/A
Check if a block has rooms N/A
Reserve a room from a block N/A
Test coverage N/A

Code Review

Baseline Feedback
Used git regularly increase the frequency of your commits.
Answer comprehension questions yes
Design
Each class is responsible for a single piece of the program yes
Classes are loosely coupled 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 Enumerable methods yes
Appropriately writes and utilizes classes yes
Appropriately utilizes modules as a namespace yes
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! Your code is clear and your design helps you achieve all the requirements.

@rooms = []
@reservations = []

if @inventory.class != Integer

Choose a reason for hiding this comment

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

This error checking should go ahead of the assignment to @inventory

lib/booker.rb Outdated

end

def all_rooms

Choose a reason for hiding this comment

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

How is this different than the rooms reader method.


end

describe "all_rooms method" do

Choose a reason for hiding this comment

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

I like this inventory feature, but your hotel was supposed to have 20 rooms.


it "throws ArgumentError when there is no available rooms during the given date range" do
rooms = @new_booker.rooms
rooms[0].bookings << Hotel::Reservation.new(check_in: "2019-09-01", check_out: "2019-09-04", room: 1)

Choose a reason for hiding this comment

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

Consider storing the dates in variables so it is clear what you are testing.

Suggested change
rooms[0].bookings << Hotel::Reservation.new(check_in: "2019-09-01", check_out: "2019-09-04", room: 1)
check_in_date = "2019-09-01"
check_out_date = "2019-09-04"

expect(Hotel::Room.new(number: 3).bookings.length).must_equal 0
end

# it "adds booking to the bookings array if provided" do

Choose a reason for hiding this comment

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

remove commented code

expect(Hotel::Reservation.new(check_in: check_in, check_out: check_out).room).must_equal nil
end

it "is set up for specific attributes and data types" do

Choose a reason for hiding this comment

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

nice separation of Act/Arrange/Assert in this test.

it "sets room to nil if not provided" do
check_in = "2019-09-01"
check_out = "2019-09-04"
expect(Hotel::Reservation.new(check_in: check_in, check_out: check_out).room).must_equal nil

Choose a reason for hiding this comment

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

I think there's a .must_equal_nil method.


end

describe "make_reservation method" do

Choose a reason for hiding this comment

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

Nice, comprehensive tests.

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