-
Notifications
You must be signed in to change notification settings - Fork 40
Space - Cathy #21
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
base: master
Are you sure you want to change the base?
Space - Cathy #21
Conversation
… reservation_test.
…ions. Added corresponding tests.
…en making a reservation, it will be added to room's tracking collection.
…nd #get_total_price method.
…rooms method to also consider blocks.
HotelSection 1: Major Learning Goals
Section 2: Code Review and Testing Requirements
Section 3: Feature Requirements
Overall Feedback
Additional FeedbackGreat work overall! You've built your first project with minimal starting code. This represents an incredible milestone in your journey, and you should be proud of yourself! I am particularly impressed by the way that you went about writing your tests. You had really strong set up and assertion checks throughout your project. It might make sense to re-think how you implemented blocks. The way you have it you just make a series of unrelated reservations. Once you've reserved a block of rooms you should be able to reserve those rooms through that block, but not outside of it. (They aren't just a bulk purchase of reservations with a discount.) This project was extremely difficult though and just completing the basic functionality of blocks is a victory! Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
kaidamasaki
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! Here are some ways you can tidy up and clarify your code. 😄
| nights = (@end_date - @start_date).to_i | ||
| return nights |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to assign to a variable before returning:
| nights = (@end_date - @start_date).to_i | |
| return nights | |
| return (@end_date - @start_date).to_i |
| return true if date >= @start_date && date < @end_date | ||
| return false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can directly return the boolean condition:
| return true if date >= @start_date && date < @end_date | |
| return false | |
| return date >= @start_date && date < @end_date |
| # your library code should assume that it's receiving Date objects to start | ||
| def initialize(start_date, end_date) | ||
| raise ArgumentError.new("Please pass in Date class instances.") if !(start_date.is_a? Date) || !(end_date.is_a? Date) | ||
| raise ArgumentError.new("Live in the present! Dates should not be prior to today.") if start_date < Date.today |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's helpful to include the bad arguments in message of the ArgumentError:
| raise ArgumentError.new("Live in the present! Dates should not be prior to today.") if start_date < Date.today | |
| raise ArgumentError.new("Live in the present! Dates should not be prior to today. Bad start_date: #{start_date}") if start_date < Date.today |
| @@ -0,0 +1,3 @@ | |||
|
|
|||
| class NoAvailabilityError < StandardError | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Custom exception class! 🎉
| attr_reader :id, :start_date, :end_date, :date_range, :room_id, :block | ||
| attr_accessor :cost | ||
|
|
||
| @@next_id = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally we advise against using a class variable. In this case it's not too bad but it would be preferable to have this stored in the SystemCoordinator.
| def list_rooms | ||
| return rooms | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need both a list_rooms method and an attr_reader on rooms, you should pick one.
|
|
||
| def find_reservations_room_date(room_id, date_range) | ||
| selected_room = find_room(room_id) | ||
| reservations_room_date = selected_room.bookings.reject{|reservation|false == reservation.date_range.overlapping(date_range)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified using select:
| reservations_room_date = selected_room.bookings.reject{|reservation|false == reservation.date_range.overlapping(date_range)} | |
| reservations_room_date = selected_room.bookings.select {|reservation| reservation.date_range.overlapping(date_range)} |
| status = false if item.date_range.overlapping(given_range) | ||
| end | ||
| end | ||
| available_rooms << room if true == status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to compare to true here since status is a boolean value:
| available_rooms << room if true == status | |
| available_rooms << room if status |
| class SystemCoordinator | ||
| attr_reader :rooms | ||
|
|
||
| @@next_block = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely doesn't need to be a class variable since there will only be one SystemCoordinator instantiated.
| rooms_in_block = [] | ||
|
|
||
| room_array.each do |room| | ||
| if room.is_available(date_range) == false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be clarified using an unless:
| if room.is_available(date_range) == false | |
| unless room.is_available(date_range) |
Assignment Submission: Hotel
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection