Skip to content

Melissa O'Hearn : Hotel : Edges#34

Open
melicious-dish wants to merge 20 commits intoAda-C10:masterfrom
melicious-dish:master
Open

Melissa O'Hearn : Hotel : Edges#34
melicious-dish wants to merge 20 commits intoAda-C10:masterfrom
melicious-dish:master

Conversation

@melicious-dish
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? Initially, I did not have a separate class for date range. What helped me make a final decision was help from Maddie via Dan. This made sense though as it had one responsibility - to create a date range based on check-in and check-out dates. I was able to use the methods in this class in the booking_system class
Describe a concept that you gained more clarity on as you worked on this assignment. I definitely got better at writing tests during this project. I actually found it pretty helpful to write the test before the code in most instances.
Describe a nominal test that you wrote for this assignment. I wrote a test to list reservations for a specific date, with dates that were legitimate (check-in before check-out) for instance.
Describe an edge case test that you wrote for this assignment. An edge case test was to raise a standard error if the check-in date was after the check-out date
How do you feel you did in writing pseudocode first, then writing the tests and then the code? I'm actually finding this really helpful, as it's a lot to think about at one time so breaking it down and not worrying about proper pretty code at first is helpfiul

@melicious-dish melicious-dish changed the title hotel Melissa O'Hearn : Hotel : Edges Sep 10, 2018
@droberts-sea
Copy link

Hotel

What We're Looking For

Feature Feedback
Baseline
Used git regularly yes
Answer comprehension questions yes
Design
Each class is responsible for a single piece of the program yes - good work, this feels very clean. The hint for this design may originally have come from the instructors, but you were the one who interpreted and implemented it, and I'm very happy with the results.
Classes are loosely coupled yes
Wave 1
List rooms yes
Reserve a room for a given date range yes
List reservations for a given date yes
Calculate reservation price yes
Invalid date range produces an error yes
Test coverage yes
Wave 2
View available rooms for a given date range yes
Reserving a room that is not available produces an error yes
Test coverage yes
Wave 3
Create a block of rooms no
Check if a block has rooms no
Reserve a room from a block no
Test coverage no
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 enumerables yes
Appropriately writes and utilizes classes yes
Appropriately utilizes modules as a mechanism to encapsulate functionality yes
Wrap Up
There is a refactors.txt file yes
The file provides a roadmap to future changes yes
Additional Feedback Good job overall! The structure and organization of this program is spot on, and your code is well-written throughout. I'm especially impressed with your test coverage. It would have been good to get to wave 3, but it's clear to me that the learning goals for this assignment were met regardless. Keep up the hard work!

module Hotel
class Blocks

# As an administrator, I can create a block of rooms

Choose a reason for hiding this comment

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

Since each instance of this class represents one block of rooms, we would typically use a singular name like Block.

def make_reservation(cost_per_night, check_in, check_out)
available_rooms = list_available_rooms(Hotel::DateRange.new(check_in, check_out))
if available_rooms.empty?
raise StandardError, "There's no room in the inn"

Choose a reason for hiding this comment

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

You create a DateRange here inline, then pass the check_in and check_out into the Reservation which creates another DateRange for the same dates. It might be a little cleaner if you made one instance and passed it into Reservation.new:

def make_reservation(cost_per_night, check_in, check_out)
  date_range = Hotel::DateRange.new(check_in, check_out)
  available_rooms = list_available_rooms(date_range)
  # ...
  reservation = Hotel::Reservation.new(room_number, cost_per_night, date_range)

Of course, you would have to adjust your constructor for Reservation.

# list reservations for a specific date range
def reservations_by_date_range(date_range)
res_by_date_range = @reservations.select do |res|
res.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.

I love that you broke this out as a separate method - it makes list_available_rooms much easier to read.

Choose a reason for hiding this comment

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

This is also a great example of loosely coupled code - instead of having to do math on the individual dates stored in the reservation, this code is able to call another method (DateRange#overlap?) that knows how to do the work.


# - Generate an reservation ID
def self.generate_id
rand_array = []

Choose a reason for hiding this comment

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

This is rather a lot of work, and more importantly it's not guaranteed to give you a unique ID. Another way to approach this would be to assign numeric IDs sequentially, starting at 1 and counting up - this is what Rails does. Something like this:

@@next_id = 1
def assign_id
  id = @@next_id
  @@next_id += 1
  return id
end


unless @check_out > @check_in
raise StandardError.new("Check-out can not be before check-in. Chech-out is: #{@check_out} check-in is #{@check_in}")
end

Choose a reason for hiding this comment

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

Instead of raising a StandardError, best practice is to define your own appropriately named exception type that inherits from StandardError and raise that. The reason is that StandardError is very broad, and if the code that calls this method has to rescue StandardError, that will catch things like ArgumentErrors and NameErrors that indicate true bugs.

@rooms =
[
{room_number: 1}, {room_number: 2}, {room_number: 3}, {room_number: 4}, {room_number: 5}, {room_number: 6}, {room_number: 7}, {room_number: 8}, {room_number: 9}, {room_number: 10}, {room_number: 11}, {room_number: 12}, {room_number: 13}, {room_number: 14}, {room_number: 15}, {room_number: 16}, {room_number: 17}, {room_number: 18}, {room_number: 19}, {room_number: 20}
]

Choose a reason for hiding this comment

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

This data structure is a little more complex than it needs to be - I would probably keep a list of integers rather than making them all hashes. This ends up making some of your code below more complex as well.

def overlaps?(date_range)
overlap = @check_in < date_range.check_out && date_range.check_in < @check_out
return overlap
end

Choose a reason for hiding this comment

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

Good work getting this tricky bit of logic figured out.

# puts @booking.reservations.first.reservation_id
puts @booking.reservations.first.total_cost
puts @booking.reservations
expect(@booking.reservations.length).must_equal 1

Choose a reason for hiding this comment

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

You might check that the length was 0 before making the reservation.

it "raises an error if no rooms are available" do
20.times do
@booking.make_reservation(200, "2018-02-03", "2018-02-06")
end

Choose a reason for hiding this comment

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

Good work getting this edge case!

describe "initialize" do
before do
@dates = Hotel::DateRange.new("2018-02-03", "2018-02-06")
end

Choose a reason for hiding this comment

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

You've done a good job making sure your list of test cases is exhaustive. Many of these were provided in class, but there's still something to be said for diligently making sure you've gotten each one (and for having a design where it's straightforward to implement the tests)

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