Skip to content

Ports - Sopheary#28

Open
sophearychiv wants to merge 54 commits intoAda-C11:masterfrom
sophearychiv:master
Open

Ports - Sopheary#28
sophearychiv wants to merge 54 commits intoAda-C11:masterfrom
sophearychiv:master

Conversation

@sophearychiv
Copy link
Copy Markdown

@sophearychiv sophearychiv commented Mar 18, 2019

Hotel

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What was a design challenge that you encountered on this project? It was hard at first to come up with composition relationships between classes. I was not sure which classes should contain the others and which ones should be contained. It got clearer as I started the actual coding.
What was a design decision you made that changed over time over the project? First, I had a superclass from which Reservation and Room classes would inherit from, but then after giving it a second thought, I decided that having a superclass was not necessary since the classes Room and Reservation do not have many similar behaviors or state.
What was a concept you gained clarity on, or a learning that you'd like to share? When and how to create an instance from a class. For example, I created an instance of the class Reserve to store a collection of reservations that I can later on query to check available rooms for a given date range.
What is an example of a nominal test that you wrote for this assignment? What makes it a nominal case? A nominal test I wrote was a test to check if the method .reserve can reserve an available room and adds the new reservation to the list of reservations. It's a nominal case because it behaves as expected.
What is an example of an edge case test that you wrote for this assignment? What makes it an edge case? An edge case I wrote was the one that tests that the .reserve method cannot reserve unavailable rooms. That it occurs at an extreme case (when the room is unavailable) makes it an edge case.
How do you feel you did in writing pseudocode first, then writing the tests and then the code? I didn't do well at first. It took me many attempts to start feeling more accustomed to it. I felt like it's kind of a waste of time to go through the process of writing the pseudocode before writing tests and then code, though I know that it simplified my thoughts and design and helped me break down the problems.

@CheezItMan
Copy link
Copy Markdown

Hotel

What We're Looking For

Feature Feedback
Baseline
Used git regularly Good number of commits and good commit messages
Answer comprehension questions Check, I'm sorry writing pseudocode was such a tough exercise for you.
Design
Each class is responsible for a single piece of the program For the most part, see my inline comments about your manager class.
Classes are loosely coupled Check
Wave 1
List rooms Check
Reserve a room for a given date range Check
List reservations for a given date Check
Calculate reservation price Check
Invalid date range produces an error Check
Wave 2
View available rooms for a given date range Check
Reserving a room that is not available produces an error Check
Wave 3
Create a block of rooms Check
Check if a block has rooms Check
Reserve a room from a block Check
Test coverage 100%
Fundamentals
Names variables, classes and modules appropriately For the most part, see my inline comments
Understanding of variable scope - local vs instance Check
Can create complex logical structures utilizing variables Check
Appropriately uses methods to break down tasks into smaller simpler tasks Check
Understands the differences between class and instance methods See my inline notes especially about your DateRange class.
Appropriately uses iterators and Enumerable methods Check
Appropriately writes and utilizes classes Check
Appropriately utilizes modules as a namespace Check
Wrap Up
There is a refactors.txt file Check
The file provides a roadmap to future changes Check
Additional Feedback Well done, this was a tough project and you got a design working. I do think your DateRange class is very underused and could either be folded into Reservation or made into a useful class where each Reservation and Block would have-a DateRange and could use it to detect overlaps between Reservations and Blocks. Let me know if you have questions.

Comment thread spec/date_range_spec.rb Outdated
@@ -0,0 +1,29 @@
require "spec_helper"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This should be require_relative

Comment thread lib/reservation.rb Outdated
end

def self.load_all
return @reservations || []
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You're trying to use in instance variable in a class method. That doesn't make sense.

This method looks like it should be reading from the CSV file and returning a list of Reservations.

Comment thread lib/date_range.rb
def initialize(check_in_date, check_out_date)
self.class.validate_date(check_in_date)
self.class.validate_date(check_out_date)
validate_date_range(check_in_date, check_out_date)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You're not saving check_in_date or check_out_date in instance variables here... so why have the class.

I think you're missing

@start_date = check_in_date
@end_date = check_out_date

On the other hand if you don't' want to make instances of DateRange I suggest making it a module and have no initialize method.

end

# I can access the list of reservations for a specific date, so that I can track reservations by date
def list_reservations(date:)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

end

# I can get the total cost for a given reservation
def total_cost(reservation_id:)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I suggest the cost of a reservation is best served in the Reservation class. It seems like your manger class is doing too many jobs.

check_in_date: "2019-03-10",
check_out_date: "2019-03-15",
discount_rate: 0.10)
expect {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You should also have edge-case tests, like reserving a room on the same day a previous reservation ends, and on the same day one begins.

You should also test reserving two different rooms on the same dates.

check_in_date: "2019-04-12",
check_out_date: "2019-04-15")
expect(reservation_manager.list_reservations(date: "2019-03-14").length).must_equal 2
expect(reservation_manager.list_reservations(date: "2019-04-14").length).must_equal 1
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You should also check the values inside those arrays.

reserve_04_15_04_20
expect(reservation_manager.find_available_rooms(check_in_date: "2019-03-20", check_out_date: "2019-03-22").length).must_equal 20
expect(reservation_manager.find_available_rooms(check_in_date: "2019-04-17", check_out_date: "2019-04-19").length).must_equal 19
expect(reservation_manager.find_available_rooms(check_in_date: "2019-03-10", check_out_date: "2019-04-19").length).must_equal 18
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would also check that at least one of the expected available rooms is available and that a room that is booked is not listed as available.

expect(reservation_manager.find_available_rooms(check_in_date: "2019-03-08", check_out_date: "2019-03-22").length).must_equal 19
end

it "returns the right number of rooms when check_in_date and check_out_date range is within reserved rate range " do
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The above are good tests.

end
end

describe "validate_id method" do
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

validate_id seems like it should be a private helper method and thus not tested directly.

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