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

Jan Edrozo | Carets #30

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

Jan Edrozo | Carets #30

wants to merge 18 commits into from

Conversation

JNEdrozo
Copy link

@JNEdrozo JNEdrozo commented Sep 11, 2017

Hotel

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Describe a design decision you had to make when working on this project. What options were you considering? What helped you make your final decision? During the design stages, I considered having my hotel class create my instances of rooms. However, when I thought about how the hotel staff would keep an inventory of their rooms and costs, I figured they should have that information on a spreadsheet in some sort of database, so instead, I went with a CSV file. I thought about doing this for generating a list of reservations (for the reservation instances) but, decided against it because I wasn't quite sure where reservation data would be coming from. (NOTE: I have a guest.rb and spec file that I have decided to not include as part of the lib and specs.. but kept as I may use it in the future).
Describe a concept that you gained more clarity on as you worked on this assignment. A concept that I've gained a bit more clarity on is how to use the Date and DateRange classes. For example, with the DateRange class, I learned that there are subtle distinctions between its .include? and .overlaps? methods. (Include returns false if a date range extends past its compared range, but overlaps returns true).
Describe a nominal test that you wrote for this assignment. An example of a nominal test: when I tested my attr_reader methods, I tested to see what kind of class or instance the return value would output (knowing that the value was explicitly made out in the initialize block).
Describe an edge case test that you wrote for this assignment. An edge case example test: when I was testing for an invalid date range, I tested for an end date that was older than its start date. Another example would be when I tested to see if a reservation could be made if the requested start_date was on the end_date of a previous booking.
How do you feel you did in writing pseudocode first, then writing the tests and then the code? It helped with hashing out design plans and brainstorming approaches for methods in waves 1 & 2, but it got increasingly more difficult with wave 3 as I had to reconsider how to not only redesign aspects of my code base, but also think about how my current tests would fail given new changes.

Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

Some in-code comments.

@cost = cost
end

def self.all

Choose a reason for hiding this comment

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

Nice that you did the CSV work, but wouldn't it make sense to just store the Reservations instead, the rooms are just IDs and costs, while the reservations have the interesting and complex data.


def reserve_room(first_name, last_name, room_id, room_rate, start_date, end_date, block_id = nil)

raise InvalidDateRangeError.new("Date range conflicts with room requested") if end_date <= start_date

Choose a reason for hiding this comment

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

If the Reservation deals with invalid dates, HotelAdmin doesn't have to.


if block_id.nil? #if a room doesn't have a block_id, blacklist all rooms that are blocked
raise UnavailableRoomError.new("Room is unavailable") if @block_list.any? do |block|
block.date_range.overlaps?(requested_range) && block.rooms_array.any? { |room| room.id == room_id }

Choose a reason for hiding this comment

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

This couples Block (and similarly Reservation`) very tightly. Instead you could create a method in Reservation & Block that checks to see if it contains the room/date combo.


def reserve_block(block_id, date_range, rooms_array, discount_room_rate)

if @block_list.any? { |block| block.date_range.overlaps?(date_range) }

Choose a reason for hiding this comment

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

It doesn't seem to check to see if there are reservations for the given rooms.

@CheezItMan
Copy link

Hotel

What We're Looking For

Feature Feedback
Baseline
Used git regularly A decent number of commits, but more would be better. The messages are good.
Answer comprehension questions Check, interesting that you went with the CSV.
Design
Each class is responsible for a single piece of the program You've got some business logic inside Reservation instead of HotelAdmin, which is good.
Classes are loosely coupled You've got a bit of tight coupling between HotelAdmin and both Block and Reservation. See my comments in the code.
Wave 1
List rooms Check
Reserve a room for a given date range Check, but it doesn't seem to check to see if there's an existing reservation for the room. Also see further notes in the code.
List reservations for a given date Check
Calculate reservation price Check
Invalid date range produces an error Check, good use of custom errors
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, but it's not checking existing reservations
Check if a block has rooms Check
Reserve a room from a block Check, you're having the reservation method work for both normal rooms and rooms in blocks. Interesting.
Test coverage 97%
Additional Feedback There's a lot of good here, good work with some business logic in the Reservation class and the addition of CSV files and the Date_Range class. A little less tight coupling and a few bug-fixes this would be a great project. There's a lot of growth here. Nice work!

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