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

Ports - Laneia #45

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

Ports - Laneia #45

wants to merge 5 commits into from

Conversation

laneia
Copy link

@laneia laneia commented Mar 19, 2019

Hotel

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What was a design challenge that you encountered on this project? I considered making the Calendar module functions part of RoomReservation but decided it had enough different functions to warrant its own module/separate structure.
What was a design decision you made that changed over time over the project? Initially I decided to make a Room class, but it wasn't necessary so although I like the idea I decided to leave it out.
What was a concept you gained clarity on, or a learning that you'd like to share? I tried to reduce dependencies more than what we did in past projects, like oo-ride-share, and I feel like putting the book's concepts into action helped solidify them.
What is an example of a nominal test that you wrote for this assignment? What makes it a nominal case? There is a test to make sure new_reservation in RoomReservation instantiates the Reservation class with instance variables that match the input arguments.
What is an example of an edge case test that you wrote for this assignment? What makes it an edge case? There is a test to make sure attempting to book a room when the hotel has no availability throws an error.
How do you feel you did in writing pseudocode first, then writing the tests and then the code? I am still unable to write the majority of tests without the majority of actual code written first, but it is improving with time.

Side note, I have no idea what happened between my files and Github. I believe I was pushing to the wrong branch somehow, and then had a bunch of trouble trying to transfer them over so eventually I just used file uploader in a web browser to add the files to this correct repository. Apparently those are the only commits viewable now.

@CheezItMan
Copy link

Hotel

What We're Looking For

Feature Feedback
Baseline
Used git regularly You had issues with git, so hard to judge here.
Answer comprehension questions Check, I'm glad you're improving on pseudocode and TDD, but it's normal for it to be tough.
Design
Each class is responsible for a single piece of the program Check, but see my notes on Calendar
Classes are loosely coupled Check
Wave 1
List rooms Check
Reserve a room for a given date range Check
List reservations for a given date Check
Calculate reservation price Check
Invalid date range produces an error Check
Wave 2
View available rooms for a given date range Check
Reserving a room that is not available produces an error Check
Wave 3
Create a block of rooms Check
Check if a block has rooms Check
Reserve a room from a block Check
Test coverage 98%, nice!
Fundamentals
Names variables, classes and modules appropriately Mostly, see my inline notes
Understanding of variable scope - local vs instance Check
Can create complex logical structures utilizing variables Check, I like how you used array methods to get available rooms
Appropriately uses methods to break down tasks into smaller simpler tasks Check
Understands the differences between class and instance methods Check
Appropriately uses iterators and Enumerable methods Check
Appropriately writes and utilizes classes Check
Appropriately utilizes modules as a namespace Check
Wrap Up
There is a refactors.txt file Check
The file provides a roadmap to future changes Check
Additional Feedback Well done, you hit all the learning goals for the project. Nice work! See my inline notes and the instructor reference solution, but you did a nice job. Let me know if you have questions.

def initialize(name, check_in, check_out, block, rate)
check_number_of_rooms(block)
@name = name
@check_in = check_in

Choose a reason for hiding this comment

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

You should also do something to verify the dates here.

end
end

def set_blocked_room_rate

Choose a reason for hiding this comment

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

This method doesn't make too much sense due to the fact that the room's rate only change for reservations within the block, not normal reservations.

end

def available
available_rooms = rooms.find_all { |room| room.block_reserved == nil }

Choose a reason for hiding this comment

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

Does this mean that a room can only be in one block at a time? What about two blocks on different dates?

@@ -0,0 +1,32 @@
module Calendar
def Calendar.date_range_include?(reservation, date)

Choose a reason for hiding this comment

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

It's good to have these methods, but wouldn't it make sense to make them instance methods of the Reservation class or pulled into instance method of a DateRange class? that way you could do?

my_reservation.includes(date)

my_reservation.does_not_overlap(other_reservation)


module Hotel
class RoomReservation
Room = Struct.new(:id, :cost, :block_reserved)

Choose a reason for hiding this comment

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

You don't seem to use this struct...

require_relative "full_room"

module Hotel
class RoomReservation

Choose a reason for hiding this comment

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

This class seems like it would be better named something like ReservationManager. Something that describes it's function.

return new_reservation
end

def room_unavailable?(room, check_in, check_out)

Choose a reason for hiding this comment

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

Good method, but I think it should be a private helper method.

return blocked_rooms
end

def block_available_rooms(check_in, check_out, number_of_rooms, rate)

Choose a reason for hiding this comment

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

This should also be a private helper method.

2 - Test data read from a CSV file could have been more thorough and led to better
tests

3 - I'm not sure that I like Calendar being a separate module but I'm not sure how

Choose a reason for hiding this comment

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

Check out my comment above

@reservation3 = @hotel.new_reservation(room_id3, check_in3, check_out3)
end

it "instantiates a new Reservation and adds it to master collection" do

Choose a reason for hiding this comment

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

You should also test that you can add a reservation on the same date the previous reservation ends.

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