Branches - Monick

[mfunkemomo]

Hotel

Congratulations! You're submitting your assignment!

Comprehension Questions

Question	Answer
What was a design challenge that you encountered on this project?	The biggest challenge was being unsure if my design would even work in the first place and thinking too much about anticipating for the future use of the program.
What was a design decision you made that changed over time over the project?	I went from having 4 classes to 3 and I changed the relationship between my date class and my manager class (changed arrow directions).
What was a concept you gained clarity on, or a learning that you'd like to share?	I like to start writing the program first as it helps me gain a better understanding of my design relationships but I think if I had started with writing tests first, it would have helped build my program better.
What is an example of a nominal test that you wrote for this assignment? What makes it a nominal case?	I wrote tests that double checked if I was getting the correct output from my methods as I was anticipating, such as an array or hash.
What is an example of an edge case test that you wrote for this assignment? What makes it an edge case?	The only edge case that I think was able to do was raise an ArgumentError if the checkout date was before the checkin date (did not have time to think about other edge cases)
How do you feel you did in writing pseudocode first, then writing the tests and then the code?	I love writing pseudocode first as it really helps me have a clear idea and outline of what I'm trying to do. But I admit, I don't do it often enough

*** Forgot to add refactor.txt file. Here are my list of refactors.

• change the way current_reservations saves
• maybe load all Reservation instances
• change tests to pass saving newly made reservations
• add Booking class for wave 3
• check methods that check for unavailable/available rooms

[tildeee]

Hotel

What We're Looking For

Test Inspection

Workflow	yes / yes but no test / no
Wave 1
List rooms	yes
Reserve a room for a given date range	yes
Reserve a room (edge case)	yes but not all the tests
List reservations for a given date	yes
Calculate reservation price	yes
Invalid date range produces an error	yes, in reservation_manager
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 for if no rooms are available
Test coverage	yes
Wave 3
Create a block of rooms
Check if a block has rooms
Reserve a room from a block
Test coverage

Code Review

Baseline	Feedback
Used git regularly	yes
Answer comprehension questions	yes! you and your answers are valid!
Design
Each class is responsible for a single piece of the program	a little confusing-- what is the Reservation class for if reservations are being tracked in a different data structure in Reservation_Manager? Also, it seems like there's not a clear responsibility for what Reservation_Dates does...
Classes are loosely coupled	a lot of work is happening in Reservation_Manager-- it would be cool to see if some logic about validity, overlapping, etc for reservations or dates could move to other classes!
Fundamentals
Names variables, classes and modules appropriately	yes
Understanding of variable scope - local vs instance	there are some cases of using instance variables not quite correctly
Can create complex logical structures utilizing variables	yes
Appropriately uses methods to break down tasks into smaller simpler tasks	your code would benefit a lot from making additional smaller, simpler helper methods!
Understands the differences between class and instance methods	yes
Appropriately uses iterators and Enumerable methods	you rely a lot on .times loops -- there are a lot of cases where a refactor to use .each, .map, .select, and .find would have really helped organize your code!
Appropriately writes and utilizes classes	yes, though I have good notes about naming conventions
Appropriately utilizes modules as a namespace	yes
Wrap Up
There is a refactors.txt file	yes in your PR
The file provides a roadmap to future changes	yes

Overall 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!

Good work on this project! I really like how you took ownership of how to keep track of reservations by utilizing a hash and nested arrays. A lot of your tests are cleanly written, too! I think you have a really solid start on this project.

That being said, I want to highlight places that I'd want to see this project improve if you had more time. In general, I see two things that concern me:

• Some of your code is not quite clear or where I'd like them to be. This happens in some of your implementation code and some of your tests. Some tests miss the Arrange-Act-Assert steps. Some of your implementation code gets long, and loses clarity. I think that the lack of clarity in the code and in the tests feed into each other, and lead to syntax issues
• A lot of your code relies on going "the long way"-- your code relies on .times loops, and not utilizing instances of other classes. For example, your Reservation_Manager class does not rely a lot on the Reservation class, despite the name! It keeps track of data without using that class. I'm afraid that this comes from not being comfortable with using instances of classes. Also, I observe some lack of comfort with instance variables and such.

I would love to make sure that you come out of Unit 1 with more clarity on:

• how to setup tests
• how to think about composition and instances of other classes within one class
• how to utilize arrays of classes, like an array of reservations

I've added a few comments on some places that I think would be good considerations and questions to muse on. From those comments, I'd love to know what questions come up, too! Let's catch up soon!

[tildeee]

Nitpick: Naming convention says this is named ReservationDate for class names-- no underscore, and keep it singular!

[tildeee]

Nitpick: the Ruby naming convention would call this ReservationManager, without the _

[tildeee]

Is it possible to refactor this MAX_ROOMS.times to @current_reservations.each do |room, reservations|, so that room will always represent the value of the room? (aka below you would use @current_reservations[room])

[tildeee]

Using a .times loop feels awkward, and relying on a variable called key doesn't quite evoke the meaning of what key is. It might be interesting to refactor this to use a .select, since .select will create an array of values that are true in certain conditions (like if @current_reservations at that room excludes the date)

[tildeee]

Instead of using an instance variable @dates, it probably will be fine to keep it as a local variable dates. This is because currently, @dates isn't used outside of this method, and the ReservationManager doesn't need to keep and manage @dates.

[tildeee]

this method is long and a little difficult to read. What are you trying to accomplish here? What should this method return? How can we change this name so it's more evocative?

[tildeee]

Hm, your ReservationManager keeps a list of all reservations in the hash @current_reservations, whose keys are rooms, and whose values are hashes of room number and dates... And this instance of Reservation in new_reservation doesn't get tracked in ReservationManager. Do we need the Reservation class?

[tildeee]

Feel free to delete this class before submission if it doesn't get used!

[tildeee]

Your tests read a little strangely to me: here -- the value of @new_res2 is an instance of Reservation_Manager. Is there a better name for this variable?

[tildeee]

Using binding.pry before this line here shows me that the value of @current_reservations is nil (this makes sense-- @current_reservations hasn't been defined here yet), and the value of @new_res2 is an instance of Reservation_Manager without any reservations stored on it.

I think that this test will be better if you think about what the Arrange-Act-Assert steps are. We want to arrange an instance of Reservation_Manager and also all the data we will need to make a reservation... Our act will be to call the make_reservation method from that instance of Reservation_Manager, and our assert in this test would be to check the value of .current_reservations saved on the instance of Reservation_Manager, to see if it includes the reservation and reservation data we were expecting. To read the current_reservations instance variable from our instance of Reservation_Manager would need the attr_reader :current_reservations in Reservation_Manager, and to use the syntax reservation_manager.current_reservations in this test.

[tildeee]

This is a nitpick on syntax: Because this isn't the expect { ... }.must_raise ArgumentError syntax, we just use (), not {}, so expect( ... ).must_equal true

[tildeee]

Nice detail!