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

Marisa Morris - Carets #23

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

Marisa Morris - Carets #23

wants to merge 24 commits into from

Conversation

marisamorris
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? Whether or not to initialize the rooms automatically or use a helper method due to not every hotel having twenty rooms. I chose to use a method to create the rooms because I figured if this project is ever built on, it should be able to be changed easily.
Describe a concept that you gained more clarity on as you worked on this assignment. Using inheritance as well as how to use the built in Date object.
Describe a nominal test that you wrote for this assignment. Being able to instantiate each class.
Describe an edge case test that you wrote for this assignment. An edge case would be if the check in date is in the past or the check out date is before the check in date for the create reservation method
How do you feel you did in writing pseudocode first, then writing the tests and then the code? Writing pseudo code then the tests were much more helpful in that it helped me figure out what i ultimately wanted each class and method to do in addition to where i needed to refactor my Hotel class.

@CheezItMan
Copy link

CheezItMan commented Sep 14, 2017

Hotel

What We're Looking For

Feature Feedback
Baseline
Used git regularly Decent number of commits, good messages
Answer comprehension questions Check
Design
Each class is responsible for a single piece of the program Most of the program is in the Hotel class. You need to break out the functionality more.
Classes are loosely coupled Everything in this program revolves around the Hotel class, The other classes don't have much functionality.
Wave 1
List rooms
Reserve a room for a given date range Check via the rooms_not_reserved method
List reservations for a given date Check via reservations_by_date.
Calculate reservation price Your reservation class has the method, see my notes about it.
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 Nope: create_reservation doesn't check to ensure the room hasn't already been reserved!
Wave 3
Create a block of rooms There' not check here for dates that are invalid.
Check if a block has rooms There's a way to reserve a room from a block, but no way to see if a block has rooms.
Reserve a room from a block Via the reserve_room_in_block method. There are no tests for the dates. See my notes on these classes.
Test coverage 99% !
Additional Feedback You are putting almost all your business logic into Hotel and not the other classes. Those other classes seem mostly to be used to store data. This makes it a very difficult design to change, modify or reuse components. For things like Reservation you test the data types of attributes and instantiating, but no tests for edge cases. This is problematic. I'd like you to go over your design with your tutor and your tests as well and talk about what you could do differently and how to write tests for a class. This was a tough project and it looks like you learned a lot. Also look at your hotel class and look at the loops in some of the methods, they look like they repeat a lot, you could do some DRYing up here.

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.

Notes on your code.

@cost = cost
end

def block_rate

Choose a reason for hiding this comment

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

Shouldn't you simply override the rate method?

Why create the same method by a different name in a subclass?


def rate
nights = @check_out_date - @check_in_date
@cost = nights.to_i * Room::RATE

Choose a reason for hiding this comment

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

You set the cost to a different value in initialize. Why?

class Room
attr_reader :room_number

RATE = 200

Choose a reason for hiding this comment

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

Good use of a constant!

class Hotel
attr_reader :rooms, :reservations, :found_reservations, :unreserved_rooms, :blocks
def initialize
@rooms = []

Choose a reason for hiding this comment

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

If you're setting @rooms to [] in initialize, why do you have an add_20_rooms method. I would suggest instead making add_20_rooms private and calling it from initialize. I would also suggest renaming it to something less specific as to the implementation (number of rooms). That way you could extend or subclass Hotel and override the method to change the number of rooms.


# creates a reservation
def create_reservation(first_name, last_name, check_in_date, check_out_date, room_number)

Choose a reason for hiding this comment

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

The Reservation class should be checking for it being created with invalid dates.



def reservations_by_date(date)
@reservations.each { |reservation|

Choose a reason for hiding this comment

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

Good use of enumerables in this project.

hellomarisamorris added 3 commits September 27, 2017 23:38
…e_reservation method with the Hotel class & updated the create block method in the Hotel class
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