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

Jocelyn Gonzalez -- Carets #27

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

Jocelyn Gonzalez -- Carets #27

wants to merge 14 commits into from

Conversation

jocegonz
Copy link

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? I originally want to add dates directly into my room instances. This made sense for a while, until it didn't. Talking to other people helped me realize that what I needed was a new class that would initialize my rooms and date ranges together.
Describe a concept that you gained more clarity on as you worked on this assignment. Initializing a class from a method in another class was really cool to me, but it was a really hard concept to grasp. It pushed me to recognize how all these classes work together and call each other.
Describe a nominal test that you wrote for this assignment. available_room should grab the first instance of @hotel that it recognizes as available. If there are no rooms reserved, it should grab the first room.
Describe an edge case test that you wrote for this assignment. date_range takes a check_in date and check_out date as arguments. If the date range was entered so that the check_in date occurred after the check_out date, it would be dinged as an invalid order.
How do you feel you did in writing pseudocode first, then writing the tests and then the code? It makes sense. I could have done a better job at outlining everything before I started, but I do like the idea of writing out what I want first, then implementing it.

@CheezItMan
Copy link

Hotel

What We're Looking For

Feature Feedback
Baseline
Used git regularly A few more commits would be better. The messages are good.
Answer comprehension questions Check, good answers. I like, "until it didn't."
Design
Each class is responsible for a single piece of the program Very much is done in Reservation. It also seems to act like the hotel.
Classes are loosely coupled They're loosely coupled because only Reservation seems to do any of the work.
Wave 1
List rooms Check
Reserve a room for a given date range Check
List reservations for a given date Check
Calculate reservation price This seems to calculate the price for all the reservation.
Invalid date range produces an error It only check to see if check-in is earlier than today. Nothing checks to make sure checkin is before check-out, except with the date_range method. You can make a reservation with invalid dates however.
Wave 2
View available rooms for a given date range Check
Reserving a room that is not available produces an error It picks an available room, not a specific room. No problem.
Wave 3
Create a block of rooms I don't see this implemented.
Check if a block has rooms ditto
Reserve a room from a block ditto
Test coverage 96%
Additional Feedback Your design had a lot of problems, you probably saw some of that in the review. You have one class doing it all, rather than splitting roles between classes. I think that made the later waves harder for you.

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

require "pry"

module Hotel
class AssignRoom

Choose a reason for hiding this comment

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

I'm not sure what purpose this method really serves. A date range and a room #, would be part of a reservation, but why make a class of the two with no business logic?

It seems to act as a Reservation. That would make a better name.


module Hotel
class Reservation
DATE = /^[0-9]{1,2}-[0-9]{1,2}-[0-9]{4}$/

Choose a reason for hiding this comment

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

Great that you're using a Regex!

#strptime could be added all at once here instead?
# used for adding array in reserve_room
# will use for checking availability in available_room
def date_range(check_in, check_out)

Choose a reason for hiding this comment

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

Your Reservation class seems like it acts as a collection of reservations, and wouldn't Hotel make a better name for this class.

end
@rooms.each do |room|
unless res_nums.include? room.num
return room #returns first instance

Choose a reason for hiding this comment

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

👍

end

def total
return "$#{(stay_array.length-1) * 200}.00"

Choose a reason for hiding this comment

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

This returns the total for all reservations, rather than for a particular reservation.

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