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

Dominique - Leaves #42

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

Dominique - Leaves #42

wants to merge 14 commits into from

Conversation

dtaylor73
Copy link

Hotel

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What was a design challenge that you encountered on this project? Trying to decide if I needed a rooms class or not and trying to decide if I should add in an instance variable called "available" to that room class.
What was a design decision you made that changed over time over the project? The hotel_system class originally had an array of integers for its rooms instance variable. However, I changed it to include an array of room objects. My thinking was that this would make things easier down the road if I needed to change anything.
What was a concept you gained clarity on, or a learning that you'd like to share? I gained clarity on before blocks and class variables.
What is an example of a nominal test that you wrote for this assignment? What makes it a nominal case? I wrote a nominal test in hotel_system that tested if the list_rooms method returned an array of 20 room objects. This is nominal because this it is testing that the method behaves as it is expected to.
What is an example of an edge case test that you wrote for this assignment? What makes it an edge case? I wrote a test to ensure that an argument error was raised when invalid date ranges were given. This is an edge case because it tests the user's "worst case scenario" input or the edges of what can be provided.
How do you feel you did in writing pseudocode first, then writing the tests and then the code? Very well. Pseudocode was the most helpful.

@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)
List reservations for a given date yes
Calculate reservation price yes
Invalid date range produces an error yes
Test coverage good
Wave 2
View available rooms for a given date range yes
Reserving a room that is not available produces an error yes
Test coverage good
Wave 3 DNF
Create a block of rooms
Check if a block has rooms
Reserve a room from a block
Test coverage

Code Review

Baseline Feedback
Used git regularly Good
Answer comprehension questions yes
Design
Each class is responsible for a single piece of the program No, the HotelSystem owns too much of the responsibility
Classes are loosely coupled no due to the above.
Fundamentals
Names variables, classes and modules appropriately Yes, but method names are often similar. :(
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, look for opportunities to delegate.
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!

I am particularly impressed by the way that you broke down larger problems into bite-sized methods.

I do see some room for improvement around removing code that causes warnings, better delegation among classes and less redundant method names.

@@ -0,0 +1,105 @@
require_relative "test_helper"
require "Pry"

Choose a reason for hiding this comment

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

you left in all of your require 'pry''s, which causes a warning.

found_reservations
end

def list_available_rooms (start_date, end_date)

Choose a reason for hiding this comment

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

/Users/devin/Documents/Ada/c12/hotel/lib/hotel_system.rb:52: warning: parentheses after method name is interpreted as an argument list, not a decomposed argument

This is because you put a space after list_available_rooms

start_date = Date.parse("2019-02-08")
end_date = Date.parse("2019-02-12")

result = @hotel_system.make_reservation(start_date, end_date)

Choose a reason for hiding this comment

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

/Users/devin/Documents/Ada/c12/hotel/test/hotel_system_test.rb:61: warning: assigned but unused variable - result


def initialize(start_date: , end_date:)
@start_date = start_date
@end_date = end_date

Choose a reason for hiding this comment

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

If you use a class without any methods besides initialize, you probably either don't need a class or your other classes are doing work that belongs to the empty class.


if (reservation.date_range.start_date >= start_date && reservation.date_range.start_date < end_date) ||
(reservation.date_range.end_date >= start_date && reservation.date_range.end_date <= end_date)
reserved_room_numbers.push(reservation.room.room_number)

Choose a reason for hiding this comment

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

The discussion of POODR chapter 4 probably helps here: your Hotel class should be delegating this work to the other two classes as in the example that I gave.

end

def list_available_room_numbers(start_date, end_date)
available_rooms = list_available_rooms(start_date, end_date)

Choose a reason for hiding this comment

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

list_available_room_numbers calls list_available_rooms. This is confusing. :(

in hotel system is an array of room objects, it made it harder to access the room numbers. I ended up making unnecessary methods in hotel system
like "list_available_room_numbers" because I had to extract the room numbers from the room objects. If I had no room class and
simply just made the rooms instance variable in hotel system an array of integers 1 -20, I wouldn't need the extra methods.
Also, there was no reason to list the room cost. Every room is $200 and total cost is calculated in the reservations class.

Choose a reason for hiding this comment

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

That seems wrong to me? Isn't it 200 per day?

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