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

Hayden (Edges) - Hotel #42

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

Conversation

haydenwalls
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? I spent a couple of hours on day one using pen and paper to design the class hierarchy/interrelations of my program. I considered using a Room class originally but steered away from it after it seemed like more work than necessary to meet the specs of the project.
Describe a concept that you gained more clarity on as you worked on this assignment. I feel like I developed a better understanding of how to hide aspects of the program from itself, trying to reduce dependencies between classes/methods as much as possible.
Describe a nominal test that you wrote for this assignment. I tested that RoomBooker#new_reservation creates a new instance of the Reservation class with instance variables that match the input arguments.
Describe an edge case test that you wrote for this assignment. I tested that an error was thrown when trying to book a room while the entire hotel is already booked
How do you feel you did in writing pseudocode first, then writing the tests and then the code? I think I did a pretty good job, the only real issue being that I only wrote bare minimum tests before implementing my code (then going back and adding more tests later)

…ervations, and implement RoomBooker#reservation_cost plus helper method RoomBooker#find_reservation
…reserve_available_room, and refactor RoomBooker#list_available_rooms and RoomBooker#list_reservations, move RoomBooker#reservation_cost to Reservation class
…remove obsolete methods, refactor RoomBooker#list_available_rooms and RoomBooker#new_reservation, implement all RoomBlock related methods, RoomBooker#set_room_rate, and RoomBooker#room_unavailable? error handling
@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 You raise false instead! That's okay
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
Fundamentals
Names variables, classes and modules appropriately x
Understanding of variable scope - local vs instance x
Can create complex logical structures utilizing variables x
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
Appropriately writes and utilizes classes x
Appropriately utilizes modules as a mechanism to encapsulate functionality x, nicely done!
Wrap Up
There is a refactors.txt file
The file provides a roadmap to future changes
Additional Feedback

Hayden! Wonderful job on this.
In your BookingLogic module, you have your domain specific RoomBooker, Room, Reservation, and RoomBlock. While RoomBooker is responsible for keeping track of reservations, rooms, and blocks, and also managing them, the other three classes get to stay pretty slim by mostly having the responsibility of holding information consistent to themselves. You also had two other things: a custom error you defined, and a DateLogic module which held the logic for generically checking the overlapping date stuff. The result is some pretty clean and easy-to-read code! It worked out.

I also want to highlight that your tests, by the way they're structured, named, and written, are thorough and so clear, while at the same time being very very good/fun when you needed test data/names. I felt like it was really easy to get to know your code through its tests. Well done!

I'm adding a few comments directly into your code. The themes of these comments are mostly just suggestions for minor refactors.

Otherwise overall, your code is clean and hit the requirements in a very elegant way. Nice work!

@@ -0,0 +1,33 @@
module DateLogic
Copy link

Choose a reason for hiding this comment

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

This is one of the most appropriate ways to use a module and I'm very very very happy with this.

return true
else
return false
end
Copy link

Choose a reason for hiding this comment

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

Would it be as readable/work as well if we one-lined this to:

return reservation.check_in...reservation.check_out).cover?(date)

module BookingLogic
class Room
end
end
Copy link

Choose a reason for hiding this comment

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

Don't forget to remove unused files before you submit your project/as you refactor them out!

if DateLogic.date_range_include?(reservation, date)
list_of_reservations << reservation
end
end
Copy link

Choose a reason for hiding this comment

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

This might be a good opportunity to use select maybe?


unavailable_rooms.each do |unavailable_room|
available_rooms.delete_if { |room| room.id == unavailable_room.id }
end
Copy link

Choose a reason for hiding this comment

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

This block of code is wonderful ✨ I love it!

I kept looking at this block of code and thinking of a way to improve it. I thought of a couple ways that could potentially be less readable, like subtracting the two arrays (which I'm unsure works with structs), or using a destructive reject! or select!. Just a thought!

room_unavailable?(room, check_in, check_out)
new_reservation = BookingLogic::Reservation.new(room, check_in, check_out)
reservations << new_reservation
return new_reservation
Copy link

Choose a reason for hiding this comment

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

I just want to call out that we never formally teach this pattern, but this is a great and common pattern: when a method makes something new, it returns that new thing in the end. Nice!

return new_reservation
end

def set_room_rate(room_id, custom_rate)
Copy link

Choose a reason for hiding this comment

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

I think this method is unused? I think you probably wrote this and then realized you should move it to the RoomBlock class, which you did... but you left this code here! You can probably get rid of this if that's the case

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