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 - Elizabeth #43

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

Leaves - Elizabeth #43

wants to merge 16 commits into from

Conversation

north108
Copy link

@north108 north108 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? The main challenge was in wave 3. I wanted to use inheritance for hotel_block from reservation but then I realized my reservation used one room to initialize it so I wasn't sure what to do when I needed multiple rooms.
What was a design decision you made that changed over time over the project? I started by putting methods into reservation but then I realized that almost every method needed to be in the hotel class.
What was a concept you gained clarity on, or a learning that you'd like to share? Composition. It finally clicked how instance variables can be called in other classes by using composition and how to use that information that I have access to in other classes.
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 for if available_rooms returns an empty array. This is nominal because this is expected behavior for a hotel that has no vacancies.
What is an example of an edge case test that you wrote for this assignment? What makes it an edge case? Wrote an edge case for if a hotel was initialized with 0 rooms. This is an edge case because we wouldn't expect a hotel to have 0 rooms.
How do you feel you did in writing pseudocode first, then writing the tests and then the code? I am proud of myself for almost exclusively writing tests first then code but I didn't utilize pseudocode to its full potential.

…one I created a module called Booking and a class with the corresponding file name. Created test files for each of the classes. Require_relative the test_helper on each test file.
… them as _tests instead of _test. Wrote two tests in hotel_test and wrote the initialize in hotel.rb.
…ts. Wrote make_reservation method and wrote a test. Wrote a sectionn of make_reservation that I will implement later and a corresponding test to use at a later time when I have more of an idea of how the Reservation Class will work. All tests pass.
…Wrote total_nights in DateRange to calculate the reservations total nights. Wrote tests for total_nights. DateRange passes all tests.
…ilable. Wrote two tests for it that aren't working properly yet but will be when I integrate available_rooms into make_reservation.
@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 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 often and thorough messages! love it
Answer comprehension questions yes
Design
Each class is responsible for a single piece of the program no, your hotel class does the jobs of other 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 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!

I am particularly impressed by the way that you found ways to break down bigger problems into small, readable methods

I do see some room for improvement around delegating methods, and improving your test cases.


it "creates an instance of Hotel_Block" do
start_date = Date.new(2019, 10, 10)
end_date = Date.new(2019 10, 15)

Choose a reason for hiding this comment

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

this errors out every time. Run a final rake before you commit code! Even if the test isn't complete or doing anything, never check in code that crashes.

date_range = Booking::DateRange.new(start_date, end_date)
total_nights = 5
total_cost = 1000
wedding_block = Booking::Hotel_Block.new(date_range, total_nights, total_cost, room)

Choose a reason for hiding this comment

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

room does not exist. This doesn't cause a crash but is a pretty easy-to-fix error

def find_reservation(start_date, end_date)
reservation_array = []
reservations.each do |reservation|
if reservation.date_range.start_date == start_date && reservation.date_range.end_date == end_date

Choose a reason for hiding this comment

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

this is reaching too far into your reservations and date ranges. This should be cleared up by the POODR chapter 4 discussion.

@room = room
@total_nights = total_nights
@total_cost = total_cost
end

Choose a reason for hiding this comment

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

If you have a class that has no methods besides initialize, either the class should be a struct of some sort, or some other part of your code is doing its work for it.

@@ -0,0 +1,17 @@
require_relative 'hotel'

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/reservation.rb:1: warning: loading in progress, circular require considered harmful - /Users/devin/Documents/Ada/c12/hotel/lib/hotel.rb


attr_reader :number, :cost

def initialize (number, cost: 200)

Choose a reason for hiding this comment

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

This space causes this warning:

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

20.times do
res_start_date = Date.new(2019, 2, 15)
res_end_date = Date.new(2019, 2, 20)
res = @the_grand_budapest_hotel.make_reservation(res_start_date, res_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_test.rb:111: warning: assigned but unused variable - res

res_room = Booking::Room.new(5)
res_total_nights = res_date_range.total_nights
res_total_cost = res_total_nights * 200
reservation = Booking::Reservation.new(res_date_range, res_total_nights, res_total_cost, res_room)

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/reservation_test.rb:11: warning: assigned but unused variable - reservation

require 'date'

def build_test_reservation
start_date = Date.new(2019, 2, 15)

Choose a reason for hiding this comment

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

This seems like you meant it to be a before block.

Choose a reason for hiding this comment

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

perhaps a let:

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