Time - Corinna

[corinna-fab]

Assignment Submission: Hotel

Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.

Reflection

Question	Answer
What was a design challenge that you encountered on this project?	What design challenge didn't I encounter on this project?! I think the biggest challenge for me was taking the time to try and think through a design aspect through to the end and to understand how it fits into the big picture before diving in and trying to code.
What was a design decision you made that changed over time over the project?	Originally I didn't have a DateRange class and then I decided to pull it out for conciseness/cleanness but I'm not sure it justifies pulling that code out (at least in that way) and might want to put it back and/or find another place for it to live, when refactoring.
What was a concept you gained clarity on, or a learning that you'd like to share?	I'm starting to feel like I have a better understanding of Classes/Objects and how they relate and are executed. I feel pretty good about the theory behind it, it's the transition from cookie cutter to cookie that I get caught up on sometimes.
What is an example of a nominal test that you wrote for this assignment? What makes it a nominal case?	Checking for the proper handling of changing the block status when more than one room is tested, by searching for them and measuring for length, is an example of a nominal test because it's looking at something that should always happen given it's conditions. It is nominal because it is common.
What is an example of an edge case test that you wrote for this assignment? What makes it an edge case?	I wrote an edge case for how to handle if the requested start and end dates are the same. Because the hotel, in our case, requires a stay overnight, this is a non-standard case.
How do you feel you did in writing pseudocode first, then writing the tests and then the code?	I think I got better about this towards the end of the project but definitely took some ramping up to get used to. I also found it helpful to revisit my tests after I was feeling settled in my code (or at least portions of it) and thinking through (a) the most glaringly obvious things I could test for (the kind that feel borderline silly but are important and (b) if I'd tested all sides of the edges I'd created.

[kaidamasaki]

Good job! Here are a few ways you can clean up your code.

I was kind of long-winded here. It's been a long week. If anything doesn't make sense please Slack me.

[kaidamasaki]

You shouldn't require_relative files that you don't directly reference in here.

Specifically this causes a circular dependency which can confuse Ruby. (frontdesk.rb requires date.rb which requires frontdesk.rb.)

Why would you decide to keep a system that's this fragile? No idea. I hope they change it for Ruby 3.0!

[kaidamasaki]

Please avoid naming files the same things as built in Ruby gems you're using. This can confuse Ruby and cause errors.

Also, we try to make sure that if there's a single class in a file that the name matches it (but snake_case instead of CamelCase).

[kaidamasaki]

Please use explicit an return; also, your condition is already a boolean so you can return it directly:

Suggested change

	(date >= @start_date && date < @end_date) ? true : false
	return date >= @start_date && date < @end_date

[kaidamasaki]

One thing to note about ifs that don't have an else. They have an implied else that produces nil.

So this winds up being this when you make that explicit:

      def no_conflict?(new_start, new_end)
        if @end_date == new_start || new_start > @end_date || new_end < @start_date || new_end == @start_date
          return true
        elsif new_start >= @start_date && new_start < @end_date || new_end > @start_date
          return false
        end
          return nil
        end
      end

Because nil is falsey this is roughly equivalent to:

      def no_conflict?(new_start, new_end)
        if @end_date == new_start || new_start > @end_date || new_end < @start_date || new_end == @start_date
          return true
        elsif new_start >= @start_date && new_start < @end_date || new_end > @start_date
          return false
        end
          return false
        end
      end

Since the elsif and the else blocks both produce false then you can omit them entirely and just return your condition:

Suggested change

	def no_conflict?(new_start, new_end)
	if @end_date == new_start || new_start > @end_date || new_end < @start_date || new_end == @start_date
	return true
	elsif new_start >= @start_date && new_start < @end_date || new_end > @start_date
	return false
	end
	end
	def no_conflict?(new_start, new_end)
	return @end_date == new_start || new_start > @end_date || new_end < @start_date || new_end == @start_date
	end

[kaidamasaki]

You can simplify this using reject:

Suggested change

	def assign_room(new_reservation)
	taken_rooms = []
	all_rooms = @rooms.dup
	@all_reservations.each do |reservation|
	reservation.no_conflict?(new_reservation.start_date, new_reservation.end_date) ? taken_rooms << reservation.assigned_room : next
	end
	room_to_assign = (all_rooms - taken_rooms.flatten)
	assigning = room_to_assign.sample(new_reservation.num_rooms)
	taken_rooms.include?(assigning)
	assigning = room_to_assign.sample(new_reservation.num_rooms)
	end
	def assign_room(new_reservation)
	return @all_reservations.reject do |reservation|
	reservation.no_conflict?(new_reservation.start_date, new_reservation.end_date)
	end.sample
	end

[kaidamasaki]

Also, at least reading this it looks like you're rejecting all of the rooms that don't conflict which confuses me.

[kaidamasaki]

🎉 Bad arguments in the ArgumentError message! 🎉

I probably would have just left num_rooms off as an argument though, since it has to be 1.

[kaidamasaki]

This isn't really an ArgumentError since nothing was wrong with the arguments themselves.

I'd recommend making a custom error class like this:

module Hotel
  class BlockFullError < StandardError
  end
end

And then using it like this:

Suggested change

	raise ArgumentError.new("Invalid room reservation request: all rooms in this block have been reserved.")
	raise BlockFullError.new("Invalid room reservation request: all rooms in this block have been reserved.")

[kaidamasaki]

You seem very fond of ternary operators but you got a little carried away here:

Suggested change

	def total_cost
	@num_rooms == 1 ? final_bill = (@end_date - @start_date)*@num_rooms*COST : final_bill = (@end_date - @start_date)*@num_rooms*COST*(1 - @discount)
	return final_bill
	end
	def total_cost
	if @num_rooms == 1
	return (@end_date - @start_date) * @num_rooms * COST
	else
	return (@end_date - @start_date) * @num_rooms * COST * (1 - @discount)
	end
	end

When code is complex breaking it up into multiple lines can help make it much easier to read.

If you're going to do something like this as a ternary I'd recommend factoring out the assignment like this (since it can be easy to overlook and changes state):

Suggested change

	def total_cost
	@num_rooms == 1 ? final_bill = (@end_date - @start_date)*@num_rooms*COST : final_bill = (@end_date - @start_date)*@num_rooms*COST*(1 - @discount)
	return final_bill
	end
	def total_cost
	final_bill = @num_rooms == 1 ? (@end_date - @start_date) * @num_rooms * COST : (@end_date - @start_date) * @num_rooms * COST * (1 - @discount)
	return final_bill
	end

In fact, looking at this code more makes it clear there's a lot of duplication between the two branches so it would be reasonable to use a ternary to calculate the multiplier though I'd still want to assign it to an intermediate variable:

Suggested change

	def total_cost
	@num_rooms == 1 ? final_bill = (@end_date - @start_date)*@num_rooms*COST : final_bill = (@end_date - @start_date)*@num_rooms*COST*(1 - @discount)
	return final_bill
	end
	def total_cost
	multiplier = @num_rooms == 1 ? (1 - @discount) : 1
	return (@end_date - @start_date) * @num_rooms * COST * multiplier
	end

[kaidamasaki]

Good use of empty?!

[kaidamasaki]

Hotel

Section 1: Major Learning Goals

Criteria	yes/no, and optionally any details/lines of code to reference
Practices SRP by having at least two separate classes with distinct responsibilities, and test files for these two classes	✔️
Overall, demonstrates understanding instance variables vs. local variables. (There aren't unnecessarily too many instance variables, when it should be a local variable)	✔️
For each test file, tests demonstrate an understanding of instantiating objects properly, and using Arrange-Act-Assert	✔️
Practices pseudocode and TDD, and reflected on it by filling out the reflection questions	✔️
Practices git with at least 15 small commits and meaningful commit messages	Please commit more frequently. This will be helpful when we move to group projects.

Section 2: Code Review and Testing Requirements

Criteria	yes/no, and optionally any details/lines of code to reference
There is a class that represents a reservation, and a second class that holds/manages a collection of reservations through composition (instance variable)	✔️
The logic for checking if a reservation's date overlaps with another reservation's date is complex logic that is separated into method(s) (and potentially class(es))	✔️
The logic for checking if a reservation's date overlaps with another reservation's date has unit tests	✔️
All of the given tests run and pass	One transient test failure. Also, your requires do some weird things that break the tests on my machine.
A test coverage tool is installed and used, and shows 95% test coverage	✔️

Section 3: Feature Requirements

Feature Requirement: There is a method that...	yes/no
gives back a list of rooms, and it's tested	✔️
creates a specific reservation for a room for a given date range, and it has nominal test cases	✔️
creates a specific reservation for a room for a given date range, and it tests an edge case, such as no available room, or invalid date range	✔️
gives back a list of reservations on a given date. Its tests include a test that makes several reservations for a given date	✔️
calculates the total price for a reservation	✔️
gives back a list of available rooms for a given date range, and it has nominal test cases	✔️
gives back a list of available rooms for a given date range, and it has edge test cases, such as no available room, or invalid date range	No edge case tests.
creates a block of rooms	✔️
reserves a room from a block	✔️

Overall Feedback

Overall Feedback	Criteria	yes/no
Green (Meets/Exceeds Standards)	14+ total in all sections	✔️
Yellow (Approaches Standards)	9-13 total in all sections
Red (Not at Standard)	0-8 total in all sections, or assignment is breaking/doesn’t run with less than 5 minutes of debugging

Additional Feedback

Great work overall! You've built your first project with minimal starting code. This represents an incredible milestone in your journey, and you should be proud of yourself!

I am particularly impressed by the way that you made good use of Enumerable methods and made defensive copies of your data using dup when you were going to modify an array.

I do see some room for improvement around simplifying your code. Complex logic can make it really difficult to tell if there are bugs hiding in your code.

Code Style Bonus Awards

Was the code particularly impressive in code style for any of these reasons (or more...?)

Quality	Yes?
Perfect Indentation	✅
Elegant/Clever
Descriptive/Readable	There were some bits of your code that confused me. (See comments.)
Concise
Logical/Organized	✅