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

Dionisia - Edges - Hotel #37

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

Conversation

larachan15
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? One decision I made was to not use a module. I started to create one, but then it didn’t make sense to me because this is a small project and I only had 3 classes. I also wasn’t sure if I should make a room class or not. But in the end, because I didn’t have a module, I decided to create the room class to try and make my 3 classes have as much single responsibility as possible.
Describe a concept that you gained more clarity on as you worked on this assignment. I am still struggling with proper syntax. I had a LOT of errors in my code and so I’m becoming a lot better at reading through the stack trace. I also gained more clarity writing the tests. I definitely need to write more tests and this became a lot easier to do towards the end of the weekend.
Describe a nominal test that you wrote for this assignment. I wrote a test to check to see if reservation_tracker calculated the cost_of_a_given_reservation properly
Describe an edge case test that you wrote for this assignment. I wrote a test to check if a check_out_date was entered in before a check_in_date for reservation
How do you feel you did in writing pseudocode first, then writing the tests and then the code? I think I need more practice with this. I didn’t write much psuedocode. I tried to diagram the relationships between the methods and the classes that I thought I needed and then wrote tests. I did switch into writing code first and then creating some tests after because I was running out of time and that process for me is still faster at this time.

@tildeee
Copy link

tildeee commented Sep 21, 2018

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 ? maybe I'm not seeing if this requirement is met
Test coverage x... I expected more tests around date conflicts when creating reservations and for a lot more of your methods.
Wave 3
Create a block of rooms x
Check if a block has rooms x
Reserve a room from a block
Test coverage getting there...
Fundamentals
Names variables, classes and modules appropriately x
Understanding of variable scope - local vs instance x
Can create complex logical structures utilizing variables x yes!
Appropriately uses methods to break down tasks into smaller simpler tasks x
Understands the differences between class and instance methods x
Appropriately uses iterators and enumerables x, you took ownership of this!
Appropriately writes and utilizes classes x
Appropriately utilizes modules as a mechanism to encapsulate functionality Didn't use a module in this project, and that's okay!
Wrap Up
There is a refactors.txt file
The file provides a roadmap to future changes
Additional Feedback

Dionisia! You did a great job with this project. I think the code is really clean and logical -- I am impressed by how slim ReservationTracker turned out, and I'm pleased with that! I think that your ability to feel comfortable in Ruby (using map, iteration, pop/array manipulation, classes, constants) contributed to how nice the code feels.

I'm adding a bunch of comments that I think will amplify... elevate ... your code style.

Overall, I feel that your tests are lacking. I think you acknowledge that based on your comprehension question answers, but I just wanted to say it out loud! It's just a shame because I think the code style is great, but I can't be confident that it's working as required because of the lack of tests.

Otherwise, the code looks great... If the code actually works accurately, then the code looks really great. To be quite clear: I don't trust that much of this code works as you'd like it to/there are more bugs than you think! But code-wise and design-wise, I think this is a great start. Good job!

BLOCKRATE = 100

def initialize(check_in_date, check_out_date, block_of_rooms)
@id = SecureRandom.uuid
Copy link

Choose a reason for hiding this comment

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

I think your use of the SecureRandom gem here to generate an ID is rad!


def hotel
return "hotel"
end
Copy link

Choose a reason for hiding this comment

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

Whoops! You should probably delete this file and all references to it.

@check_in_date = check_in_date
@check_out_date = check_out_date
@room_number = room_number
raise StandardError, 'The end date cannot be before the start date.' if check_out_date <= check_in_date
Copy link

Choose a reason for hiding this comment

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

I love that the logic to check if reservation dates are valid are in the Reservation itself. Nice decision :)


# module Hotel
class Reservation
attr_reader :check_in_date, :check_out_date, :room_number, :nights_stayed
Copy link

Choose a reason for hiding this comment

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

attr_readers are reserved for names of instance variables. :nights_stayed isn't the name of an instance var, it's the name of a method -- it doesn't need to be in this list!

def initialize
@rooms = create_rooms
@reservations = []
@unreserved_rooms = []
Copy link

Choose a reason for hiding this comment

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

Does this instance variable @unreserved_rooms ever get used? If it doesn't get used, can we get rid of it?


# accesses the list of reservations for a specific date
def list_of_reservations(date)
return @reservations.find_all { |reservation| reservation.nights_stayed == date }
Copy link

Choose a reason for hiding this comment

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

Does this code actually work? It looks fishy to me, and I can't tell because there aren't any tests! :(

# set the rooms_not_reserved method and arguments to a local variable.
unreserved_rooms = rooms_not_reserved(check_in_date, check_out_date)
if unreserved_rooms.length == 0
raise ArgumentError, 'No Available Rooms for Given Dates'
Copy link

Choose a reason for hiding this comment

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

Instead of raising an ArgumentError, it might make more sense to make your own custom error or just raise StandardError!

def rooms_not_reserved(check_in_date, check_out_date)
# if the reservations array is empty, this returns an array of available room numbers. If there are no reservations, all rooms can be reserved.
return @rooms.map { |room| room.room_number } if @reservations.empty?
unreserved_rooms = @rooms.map { |room| room.room_number }
Copy link

Choose a reason for hiding this comment

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

Love how dense and juicy and exciting the above two lines are!

# checks to see if a room is available and reserves the first available room for a given date range. Uses the rooms_not reserved method.
def reserve_room(check_in_date, check_out_date)
# set the rooms_not_reserved method and arguments to a local variable.
unreserved_rooms = rooms_not_reserved(check_in_date, check_out_date)
Copy link

Choose a reason for hiding this comment

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

I love this line! Very clear what's going on here.

# if @reservation.empty? is not empty
@reservations.each { |reservation|
if (reservation.check_in_date >= check_in_date && reservation.check_in_date <= check_out_date) || (reservation.check_out_date >= check_in_date && reservation.check_out_date <= check_out_date)
unreserved_rooms.delete(reservation.room_number)
Copy link

Choose a reason for hiding this comment

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

This is a very funny assumption you're putting here:

For the call .delete to work, this assumes that the array unreserved_rooms is an array full room numbers (originally stemming from @rooms) that are unique. If they aren't unique, then this call to delete might not do what you expect... so if the requirement changes that the hotel does not have unique room numbers, this may introduce bugs here.


@blocks.each { |block|
if (block.check_in_date >= check_in_date && block.check_in_date <= check_out_date) || (block.check_out_date >= check_in_date && block.check_out_date <= check_out_date)
unreserved_rooms - block.rooms
Copy link

Choose a reason for hiding this comment

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

This is cool code here! I like it. However... the - isn't destructive. It just does the subtraction, and the result isn't stored anywhere. Observe in irb:

a = [1,2,3]
b = [1,2]
a - b
# => returns [3], as expected
# a is still [1,2,3]
# b is still [1,2]

So the subtraction doesn't do anything!

def reserve_room(check_in_date, check_out_date)
# set the rooms_not_reserved method and arguments to a local variable.
unreserved_rooms = rooms_not_reserved(check_in_date, check_out_date)
if unreserved_rooms.length == 0
Copy link

Choose a reason for hiding this comment

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

The indentation here is off ;)

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