Amy Cash - Pipes - Hotel by cashmonger · Pull Request #33 · Ada-C8/hotel

cashmonger · 2017-09-11T13:38:33Z

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 there should be a room class. I thought a lot about what I needed from that class -- would it just give me a list of rooms? If so, then that function could probably be part of hotel/reservation instead. We discussed this in our group, but I still didn't feel happy with that set up until I started actually working on it and couldn't see what extra use having a separate room class would be. I'm not really happy with how I set up blocks. The class I set up isn't really doing much of anything and if I had more time, I'd rethink how to set this up.
Describe a concept that you gained more clarity on as you worked on this assignment.	Keyword Arguments! I struggled with using them in methods where one method was calling another method, usually in another class, so learning to set that up was an informative struggle.
Describe a nominal test that you wrote for this assignment.	Testing whether each class could be instantiated
Describe an edge case test that you wrote for this assignment.	I'm not sure I did write any? I'm realizing in looking over my tests, that I'm not sure what "edge case" means in this context. It's easier for me to picture in terms of a user interface (e.g. "what happens if the administrator enters nothing", but I wasn't making that part here. I did test for things like their being no rooms available in a block, but does that count as an edge case?
How do you feel you did in writing pseudocode first, then writing the tests and then the code?	I found this very helpful, both in keeping straight what I was a trying to do with each method and clarifying my thinking about how methods and classes would/could interact

droberts-sea · 2017-09-13T00:58:09Z

Hotel

What We're Looking For

Feature	Feedback
Baseline
Used git regularly	yes - good work!
Answer comprehension questions	yes
Design
Each class is responsible for a single piece of the program	some - While your other classes keep track of state, it seems like your `Hotel` class has ended up responsible for most of the behavior of your application.
Classes are loosely coupled	mostly - I've pointed out areas inline where I noticed tight coupling.
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	missing some around finding reservations
Wave 2
View available rooms for a given date range	yes
Reserving a room that is not available produces an error	yes
Test coverage	missing some about checking reservation conflicts
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
Additional Feedback

Good work overall. The big thing I notice missing is test coverage around checking which rooms are reserved when. The logic is quite complex and it's also hidden within other functionality, which makes it difficult to test explicitly. This sort of scenario is where design work starts to pay off - by isolating behaviors like comparing two date ranges, you can test them without all the overhead of hotels and reservations, as the foundational units of your application. For an example of how design can make testing easier, you should check out our reference implementation.

That being said, I'm pretty happy with what you've submitted. Your design is clear and intentional, and your code is well-organized and easy to read. Keep up the good work!

droberts-sea · 2017-09-12T22:47:05Z

lib/date_range.rb

+
+    def self.check_dates(first, last)
+      if Date.parse(first) > Date.parse(last)
+        raise ArgumentError, "Check Out Date is earlier than Check in Date"


Is it valid if first == last?

droberts-sea · 2017-09-12T22:48:21Z

lib/date_range.rb

+
+  class DateRange
+
+    attr_reader :check_in, :check_out, :date_range


You never instantiate this class! You're basically using it as a library of methods to work with dates, which means it might be clearer as a module.

droberts-sea · 2017-09-12T23:43:16Z

lib/hotel.rb

+
+    def make_reservation(id, day_in, day_out, discount: 0, block_name: nil, room: 0)
+      available = open_rooms(day_in, day_out)
+      if room == 0


Since room number is an integer, it might make more sense to use a non-integer sentinel value here. nil is usually a good choice.

droberts-sea · 2017-09-12T23:45:07Z

lib/hotel.rb

+      DateRange.check_dates(day_in, day_out)
+      DateRange.create_range(day_in, day_out).each do |date|
+        found_reservations << all_reservations.find_all { |reservation| reservation.nights_reserved.include?(Date.parse(date)) }
+      end #each date


This is an example of tight coupling between Hotel and Reservation. Peeking into reservation.nights_reserved means that if that implementation ever changes, this code will need to change too. Instead, it might be cleaner to write a method Reservation#includes_date, and call it here.

droberts-sea · 2017-09-12T23:45:41Z

lib/hotel.rb

+        if reservation.id == reservation_id
+          return reservation.total_cost
+        else
+          return "Reservation Not Found"


This would be a good place to raise an error. Maybe a custom NoSuchReservationError?

droberts-sea · 2017-09-12T23:49:31Z

lib/hotel.rb

+      elsif room != 0 && available.include?(room)
+        first_open = room
+      else
+        raise ArgumentError, "Requested Room is Not Available during Date Range"


I'm not sure an ArgumentError is the right call here. That usually indicates the arguments are invalid (i.e. a bogus room number or date range) or not of the right type, but that's not what this error is describing. Maybe this would be a good place to use a custom exception.

droberts-sea · 2017-09-12T23:57:28Z

specs/date_range_spec.rb

+describe "DateRange Class" do
+  before do
+    @date_test = Hotel::DateRange.new
+  end #before


You don't have any tests around check_dates!

droberts-sea · 2017-09-12T23:59:23Z

specs/hotel_spec.rb

+      @hotel.make_reservation(2224, "2012/12/13", "2012/12/17")
+      @hotel.open_rooms("2012/12/12", "2012/12/13").must_be_kind_of Array
+      @hotel.open_rooms("2012/12/12", "2012/12/13")[0].must_equal 3
+    end #open rooms


Other cases I'd be interested in:

What if there are no reservations? (it should return an empty array)

Do reservations that don't overlap the date range affect the list? (they shouldn't)

Check the boundaries of what counts as an overlap. What about a reservation that ends on the checkin date, or starts on the checkout date?

droberts-sea · 2017-09-13T00:00:38Z

specs/hotel_spec.rb

+      proc {
+        @hotel.make_reservation(1000, "2001/1/30", "2001/2/4", room: 1)
+      }.must_raise ArgumentError
+    end


This is a good start, but I'd like to see more coverage around what date ranges will conflict with each other. Cases I'd be interested in:

Not available:

Same dates

Overlaps in the front

Overlaps in the back

Completely contained

Completely containing

Available:

Completely before

Completely after

Ends on the checkin date

Starts on the checkout date

droberts-sea · 2017-09-13T00:45:55Z

specs/hotel_spec.rb

+
+  describe "Blocks" do
+
+    it "Can call Block to create an array to store block information" do


Test coverage looks pretty good around blocks. There are just a couple of places where it seems like things are missing:

What happens if you try to create a block when there are not enough rooms available?

What if the dates on the reservation don't match those on the block?

cashmonger added 30 commits September 5, 2017 16:22

Set up Spec_Helper and Reservation Class

9f72ccb

Set up hotel and date files and specs

4f14110

Set up Classes; Wrote PseudoCode for Methods

56d1956

Update initialization tests for all classes

9cf882f

Update Initalization tests

d426ac5

Create Check_in and Check_out Variables and Date Range Array

e43b6a8

Add dates to Reservation Object

1119537

Update reservation method and test

fbe84f4

Add make_reservation method to Hotel Class

26a07a0

Link DateRange to Reservations

7615f62

More work on make_reservation method -- adding in DateRange

8f200fc

Add View Reservation Method to Reservation

4f0683c

Add Find Reservation by Date

d88b112

Add Total Cost

f0131f8

Update Some Tests after adding Total Cost in

a8a7872

Add Find Available Rooms Method

761e924

add ReservationList class to replace class variable in Hotel

a36755e

Improve interaction between hotel and reservation list

4f78684

Update Find Reservation Method

bb033c9

Finish Wave One

f7280d9

Work on finding open rooms

a0c397e

Update Open Room Method

cd61e9b

Add check available rooms

2c55c4b

Finish Is Available?

1a6dec9

simplify find reservation method

6153a8d

Add nights_reserved attribute and update tests

afe2930

Start Create Block

1e8092a

Check if enough rooms available for block

d983c4d

Add discount rate

260ce6d

Fix error in create block method

ead8db8

cashmonger added 14 commits September 10, 2017 14:16

Begin updating methods to request more than one room

c28fbd2

More updates to accomodate new room selection

a0f4148

Update Tests to reflect new room selection method

7a137d6

Update find reservations method

14ca04e

Fix a lot of test problems

a31c7ff

Fix Error Handing

0debf89

More Error Checking Tests

25e1a7f

Fix keyword arguments

eee3121

Trying to set up blocks again

3ecc2a6

Reserving Blocks

387264d

Check for Block Conflicts

830c402

More on Block Reservations

b53c7a1

Update Tests for Blocks

4033ec6

Update Tests

740d317

droberts-sea reviewed Sep 13, 2017

View reviewed changes

cashmonger added 4 commits October 1, 2017 15:45

Add design-activity

1466ac7

Work on Questions

780118b

Finally changed something without everything blowing up.

fbe32d5

Finish design-activity and hotel changes

9357171

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Amy Cash - Pipes - Hotel #33

Amy Cash - Pipes - Hotel #33
cashmonger wants to merge 48 commits intoAda-C8:masterfrom
cashmonger:master

cashmonger commented Sep 11, 2017

Uh oh!

droberts-sea commented Sep 13, 2017

Uh oh!

droberts-sea Sep 12, 2017

Uh oh!

droberts-sea Sep 12, 2017

Uh oh!

droberts-sea Sep 12, 2017

Uh oh!

droberts-sea Sep 12, 2017

Uh oh!

droberts-sea Sep 12, 2017

Uh oh!

droberts-sea Sep 12, 2017

Uh oh!

droberts-sea Sep 12, 2017

Uh oh!

droberts-sea Sep 12, 2017

Uh oh!

droberts-sea Sep 13, 2017

Uh oh!

droberts-sea Sep 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		class DateRange

		attr_reader :check_in, :check_out, :date_range


		describe "Blocks" do

		it "Can call Block to create an array to store block information" do

Conversation

cashmonger commented Sep 11, 2017

Hotel

Comprehension Questions

Uh oh!

droberts-sea commented Sep 13, 2017

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants