Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Carets -- Guillermina Muro #40

Open
wants to merge 50 commits into
base: master
Choose a base branch
from
Open

Carets -- Guillermina Muro #40

wants to merge 50 commits into from

Conversation

murog
Copy link

@murog murog commented Sep 11, 2017

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 was considering making a hotel a class, so that I could potentially make more hotels in the future. Instead, I kept Hotel and all of its business logic within the scope of a module. I realized that there was no requirement to make more hotels, so a module was fine.
Describe a concept that you gained more clarity on as you worked on this assignment. Inheritance! I had originally made Block its own class. However, I realized that it behaved very similarly to a reservation and took in many of the same inputs to initialize.
Describe a nominal test that you wrote for this assignment. Checked to make sure a list of all_reservations created an array that matched the amount listed in the CSV file.
Describe an edge case test that you wrote for this assignment. Many of my methods raise argument errors if they are given input ID's as anything else besides an integer. I tested to make sure strings, symbol, arrays and hashes would raise errors.
How do you feel you did in writing pseudocode first, then writing the tests and then the code? It was very helpful!! The project felt initially overwhelming, but the pseudocode -> failing test -> code -> passing test cycle made it much more manageable.

…ck in date is equal to a reservation's checkout date
@tildeee
Copy link

tildeee commented Sep 24, 2017

I love this approach! The hotel as a module works great. I'm really happy with your work on the optionals of reading from a CSV-- and it was very close to working with writing to the CSV too! ;)

Your hotel is certainly doing a lot... which makes sense, because every other class is structured to represent some data, while hotel holds it all together.
The things to keep in mind is that it does a lot of stuff

  • making reservations/blocks
  • finding reservations/blocks
  • reading from CSVs/parsing it
  • building reservations/blocks from that CSV information/parsing it

and I could see some areas of being able to pull out the CSV parsing action.

Also, there are just some danger areas where your hotel does a lot of work that can be delegated to the other classes. I've made a few comments in some areas.

Overall good work though

return all_blocks
end

def self.cost(input_reservation_id)
Copy link

Choose a reason for hiding this comment

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

you should reuse self.find_reservation here!

@tildeee
Copy link

tildeee commented Sep 24, 2017

Hotel

What We're Looking For

Feature Feedback
Baseline
Used git regularly x
Answer comprehension questions x
Design
Each class is responsible for a single piece of the program x
Classes are loosely coupled x
Wave 1
List rooms x
Reserve a room for a given date range x
List reservations for a given date x
Calculate reservation price x
Invalid date range produces an error x
Test coverage x
Wave 2
View available rooms for a given date range x
Reserving a room that is not available produces an error x
Test coverage x
Wave 3
Create a block of rooms x
Check if a block has rooms x
Reserve a room from a block x
Test coverage x
Additional Feedback

@tildeee
Copy link

tildeee commented Sep 24, 2017

by the way, I love the process of working through this project that you described in your comprehension questions ;)

lib/hotel.rb Outdated
unavailable_rooms = []
all_reservations = self.all_reservations
all_reservations.each do |reservation|
if (begin_search >= reservation.check_in) && (begin_search < reservation.check_out) && (end_search >= reservation.check_in) && (end_search <= reservation.check_out)
Copy link

Choose a reason for hiding this comment

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

maybe this logic can be returned from a reservation itself if you pass in the begin_search and end_search properties instead! That way, hotel doesn't need to know that a reservation has a .check_in and a .check_out

lib/hotel.rb Outdated
blocked_rooms = []
all_the_blocks = self.all_blocks
all_the_blocks.each do |block|
if (begin_search >= block.check_in) && (begin_search < block.check_out) && (end_search >= block.check_in) && (end_search <= block.check_out)
Copy link

Choose a reason for hiding this comment

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

same as the other comment

block = self.find_block(block_id)
raise ArgumentError.new "Block ID is not found" if !(block.is_a? Hotel::Block)
# block.add_rooms
availability = block_available(block_id)
Copy link

Choose a reason for hiding this comment

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

in this method, you've already found the block object and stored it in block, but then you redo that work in calling block_available and passing in the block_id instead of the block itself.

I would refactor this so that it relies on block more, and lets block return internal information about itself

array_check_out =[block.check_out.year, block.check_out.month, block.check_out.day]
new_reservation = Hotel::Reservation.new(5, block.rooms[0].id, array_check_in, array_check_out, block.block_id)
# block.add_reservations # => does nothing currently, would work if I was writing new reservations to CSV...)-':'
block.reservations << (new_reservation)
Copy link

Choose a reason for hiding this comment

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

ideally instead of shoveling into a block's internal property reservations, you would use a method

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