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

hotel- Leanne Rivera #32

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

Conversation

leannerivera
Copy link

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? At first I was doing more classes with inheritance. It worked for some classes, but with others the classes would start breaking. I also at first wanted a hotel class. I decided that it wasn't necessary, I just needed the admin and perhaps the rooms, and a module to hold everything together.
Describe a concept that you gained more clarity on as you worked on this assignment. Modules. I kind of understood them before but after this project and reading more on them, I understand them much better, especially when it comes to module methods, etc.
Describe a nominal test that you wrote for this assignment. that if a room is reserved the dates are updated in the respective hash
Describe an edge case test that you wrote for this assignment. passing in a string instead of a date will error
How do you feel you did in writing pseudocode first, then writing the tests and then the code? it is good, helps with the thinking process and can also help with design.

@CheezItMan
Copy link

Hotel

What We're Looking For

Feature Feedback
Baseline
Used git regularly Good number of commits and awesome commit messages! Nice work!
Answer comprehension questions Check, I'm glad you understand modules better now.
Design
Each class is responsible for a single piece of the program No, Concierge and the Lodging module seem to be doing almost everything.
Classes are loosely coupled No very tightly coupled.
Wave 1
List rooms Check
Reserve a room for a given date range Check, but I don't think it works if you try to make more than 20 reservations, given different date ranges. That's because each room has one status value, not a status depending on the date.
List reservations for a given date Sort of, it lists rooms that are reserved by date.
Calculate reservation price with receipt method
Invalid date range produces an error Check
Test coverage 100%
Wave 2
View available rooms for a given date range Check, but see my inline notes
Reserving a room that is not available produces an error No check for when no room is available
Wave 3
Create a block of rooms Check
Check if a block has rooms Check
Reserve a room from a block Check
Fundamentals
Names variables, classes and modules appropriately Somewhat, see my notes on class names
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 Check
Appropriately uses iterators and enumerables Check
Appropriately writes and utilizes classes Check, but you really need some sort of class to encapsulate the concept of a Reservation and Block.
Appropriately utilizes modules as a mechanism to encapsulate functionality Check
Wrap Up
There is a refactors.txt file Check, good insight into the Concierge class.
The file provides a roadmap to future changes Somewhat, the details of moving around are unclear.
Additional Feedback Not bad, you have functionality in place, but waves 2-3 don't really fully work and aren't fully tested. If a room is reserved from 10/10/2018 to 10/15/2018 it is available on 10/15/2018 to 10/18/2018, but with your status attribute it won't be and you never tested that scenario. Take a look at my inline notes and let me know if you have questions.


module Lodging

class Concierge

Choose a reason for hiding this comment

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

Love the name, but maybe describe the functionality more clearly. BookingManager or something similar.

module Lodging

class Room
attr_reader :room_number, :status, :cost

Choose a reason for hiding this comment

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

Does a room need a status?

room = [@room_number, @cost, @status, Array.new]

CSV.open('data/all_hotel_rooms.csv', 'a+') do |row|
row << room

Choose a reason for hiding this comment

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

Your initialize method is reading in all the hotel rooms for each hotel room. This doesn't seem to make sense.

lib/room.rb Outdated
end

def self.available_room #to return one room with status available
CSV.open('data/all_hotel_rooms.csv', 'r', headers: true) do |row|

Choose a reason for hiding this comment

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

This just reads in the CSV file and returns the 1st row. That doesn't make sense to me given the method name.

lib/concierge.rb Outdated

raise ArgumentError if Date.parse(check_out) < Date.parse(check_in) #checkout cannot be earlier than check in

book_this = Lodging.room_status(@rooms_info, check_in) #returns one available room

Choose a reason for hiding this comment

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

What is the purpose of Lodging if your Concierge is keeping track of the rooms. Why not give it the job of finding an available room.

Also why name the method room_status instead of available_room.

room[:room_number] == book_this[:room_number]
end #finds room to be reserved

update_room[:status] = "unavailable" #changes status of said room in the array of hashes as 'unavailable'

Choose a reason for hiding this comment

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

A room isn't going to have 1 status. A room could be available in one date range and unavailable in another.

Copy link
Author

Choose a reason for hiding this comment

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

i thought room status checked for this with the conditional.

block.times do
hold_this = Lodging.room_status(@rooms_info, check_in) #returns one available room

update_room = @rooms_info.find do |room|

Choose a reason for hiding this comment

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

This looks like a good candidate for a helper method.


new_cost = (update_room[:cost].to_f) - (update_room[:cost].to_f * discount)

update_room[:cost] = new_cost.round(2).to_s

Choose a reason for hiding this comment

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

If you're giving a room a different cost per reservation, that value should apply to each reservation or booking, not to the room itself. A room could have a default cost, but now if a room is put into a block, the cost for past reservations go down.

require 'awesome_print'

module Lodging

Choose a reason for hiding this comment

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

These methods look like they should go into Concierge instead of a separate module.


expect(tiny_hotel.all_rooms[0][:reserved_dates].length).must_equal 7

end

Choose a reason for hiding this comment

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

What about testing if you can do multiple reservations.

What happens if you reserve rooms and no room is left available.

These things should be tested!

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