Naheed - Hotel - Edges by arangn · Pull Request #43 · Ada-C10/hotel

arangn · 2018-09-10T17:26:49Z

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 decided to make an additional block_reservations class to mimic the reservations class after struggling with where to put the information for each block reservation. Looking back at what I had designed already and staying consistent with my design helped me decide on this.
Describe a concept that you gained more clarity on as you worked on this assignment.	I gained much more clarity on referencing classes in my tests and was able to troubleshoot much faster when I did make mistakes
Describe a nominal test that you wrote for this assignment.	A nominal test I wrote is "assigns reservation_id to reservation", which checks the new instance of reservation to ensure it has an id
Describe an edge case test that you wrote for this assignment.	An edge case I wrote is "raises an Argument Error if reservation_id is not unique" to ensure that although unlikely, rand(1..100000) does not generate the same number twice
How do you feel you did in writing pseudocode first, then writing the tests and then the code?	I didn't do a great job of starting with pseudocode, but I found that when I was having trouble conceptualizing a method, pseudocode and white-boarding helped.

droberts-sea · 2018-09-20T02:44:14Z

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	some - see inline
Classes are loosely coupled	mostly - see inline
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	no
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, missing some edge cases, see inline
Wave 3
Create a block of rooms	yes
Check if a block has rooms	yes
Reserve a room from a block	yes
Test coverage	yes
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 - great work!
Understands the differences between class and instance methods	yes
Appropriately uses iterators and enumerables	yes - see inline, but this is is a good place
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	Great work overall! This code is easy to read and does a good job of solving the problem. There are a few places where the design could be cleaned up or test coverage expanded, which I've tried to call out inline below, but in general you are right where we want you to be at this point in your journey. Keep up the hard work!

droberts-sea · 2018-09-20T02:01:51Z

lib/block_reservation.rb

+  class BlockReservation
+    attr_accessor :block_id, :reservation_id, :room, :start_date, :end_date, :price_per_night
+
+    def initialize(block_id:,  reservation_id:, room:, start_date:, end_date:, price_per_night:)


The only difference between this and a regular reservation is that a block reservation has a block ID. One option that might simplify your design is to consolidate both in the Reservation class, and add in an optional block ID, which would be nil if this reservation isn't part of a block.

droberts-sea · 2018-09-20T02:04:18Z

lib/booking_system.rb

+
+    def make_reservation(start_date, end_date)
+      reservation = Hotel::Reservation.new(reservation_id: generate_id, room: assign_available_room(start_date, end_date), start_date: start_date, end_date: end_date, price_per_night: 200)
+      @reservations << reservation


I love the way you've broken each of these bits of functionality into separate methods! However, it's a little difficult to read all on one line like this. You might split it across multiple lines:

reservation = Hotel::Reservation.new( reservation_id: generate_id(), room: assign_available_room(start_date, end_date), start_date: start_date, end_date: end_date, price_per_night: 200 )

I also like adding parentheses to the call to generate_id(), even though there are no arguments - this makes it a little clearer that this is a method call.

droberts-sea · 2018-09-20T02:05:44Z

lib/booking_system.rb

+    def make_block(start_date, end_date, number_of_rooms)
+      if number_of_rooms > 5
+        raise ArgumentError, "Cannot reserve more than 5 rooms"
+      end


What if the number of rooms is less than 1?

droberts-sea · 2018-09-20T02:06:00Z

lib/booking_system.rb

+      @blocks << block
+
+      number_of_rooms.times do
+        block_reservation = Hotel::BlockReservation.new(block_id: id,  reservation_id: nil, room: assign_available_room(start_date, end_date), start_date: start_date, end_date: end_date, price_per_night: 150)


Same comment about line lengths on lines 36 and 40.

droberts-sea · 2018-09-20T02:14:03Z

lib/booking_system.rb

+      @reservations.each do |reservation|
+        if
+          reservation.start_date < end_date && start_date < reservation.end_date
+          booked_rooms << reservation.room


Here your BookingSystem is taking on some extra responsibility: determining whether a reservation overlaps a set of dates. I would argue that that is really the reservation's responsibility. You could write a method called something like Reservation#overlap?(start_date, end_date) that takes care of this logic.

That simplifies this code, and makes it easier to test the logic in isolation. You also repeat this code on line 72, which means that if the requirements change or you discover a bug, you'll have to change it in two places.

This also an example of tight coupling, since BookingSystem needs to know about how a Reservation stores its dates.

Why not call reservations_within_date here to get the list of booked_rooms?

droberts-sea · 2018-09-20T02:33:41Z

lib/booking_system.rb

+      reservations_within_date = []
+      @reservations.each do |reservation|
+        if
+          reservation.start_date < end_date && start_date < reservation.end_date


This and the above method could be streamlined a little using the select enumerable:

return @reservations.select do |reservation| reservation.start_date < end_date && start_date < reservation.end_date end

droberts-sea · 2018-09-20T02:37:37Z

spec/booking_system_spec.rb

+
+      reservation3 = Hotel::Reservation.new(reservation_id: 3, room: @system.assign_available_room(Date.new(2018, 1, 6), Date.new(2018, 1, 9)), start_date: Date.new(2018, 1, 6), end_date: Date.new(2018, 1, 9), price_per_night: 200)
+      @system.reservations << reservation3
+      expect(reservation3.room).must_equal 2


Rather than creating a whole new reservation here, I would probably just call .assign_available_room and check the result directly:

room = @system.assign_available_room(Date.new(2018, 1, 6), Date.new(2018, 1, 9)) expect(room).must_equal 2

droberts-sea · 2018-09-20T02:38:50Z

spec/booking_system_spec.rb

+    it "makes a reservation" do
+      @system.make_reservation(Date.new(2018,1,1), Date.new(2018,1,5))
+      reservation = Hotel::Reservation.new(reservation_id: 1, room: 1, start_date: Date.new(2018, 1, 1), end_date: Date.new(2018, 1, 5), price_per_night: 200)
+      expect(@system.reservations.length).must_equal 1


You have a lot of Date.new(...) in this file, which is a little difficult to read. Instead you might create a couple in a before block and put them in instance variables to use later:

@start_date = Date.new(2018, 1, 1) @end_date = Date.new(2018, 1, 5)

droberts-sea · 2018-09-20T02:39:22Z

spec/booking_system_spec.rb

+    it "raises argument error if no rooms are available" do
+
+      20.times do
+        reservation = Hotel::Reservation.new(reservation_id: 1, room: @system.assign_available_room((Date.new(2018, 1, 1)), (Date.new(2018, 1, 8))), start_date: Date.new(2018, 1, 1), end_date: Date.new(2018, 1, 8), price_per_night: 200)


I like this test case! However, it should probably be in the describe block for make_reservation, since that's what it's testing.

droberts-sea · 2018-09-20T02:40:40Z

spec/booking_system_spec.rb

+  describe "search reservation dates" do
+    before do
+
+      reservation1 = Hotel::Reservation.new(reservation_id: 1, room: 1, start_date: Date.new(2018, 1, 1), end_date: Date.new(2018, 1, 8), price_per_night: 200)


There are several other interesting edge cases around date overlaps that you're missing here. I see you've got the list copy-pasted below, so I'm guessing it was just a matter of time.

Putting the overlap logic into its own method on Reservation would make these tests much easier to write.

arangn added 14 commits September 4, 2018 11:25

initialized guard, initial tests pass

6716534

load rooms and create instance of reservation

96bc8b6

implement make_reservation method, tests pass

56c493e

serach reservation method and tests in progress

6f7affe

search reservation method all tests passing

e52bc34

implement more tests for search reservations by date, all tests passing

3850726

implement find reservation and total cost method, tests pass

f7ed4a7

refactor assign available rooms method

eaacfbb

implement generate id and check method, tests pass

5d5c4a3

initiate block class

4ac7d30

implement block_reservaton class and methods

d5e7a4b

add search for empty rooms in block and tests

1139f5c

cleanup

8555dd5

refactors

93d59e4

droberts-sea reviewed Sep 20, 2018

View reviewed changes

arangn added 2 commits October 1, 2018 00:08

design-activity

0d19193

refactor total_cost method

dfdb23d

Conversation

arangn commented Sep 10, 2018

Hotel

Comprehension Questions

Uh oh!

droberts-sea commented Sep 20, 2018

Hotel

What We're Looking For

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants