Conversation
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 commented your code with your user stories and especially that you mentioned things like breaking a story into multiple methods. That was really thoughtful and clear. I do see some room for improvement around little things, mostly code style stuff. When you make your code more concise it can help make many bugs more obvious. (See inline comments.) Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
| @end_date = end_date | ||
| @dates = Array(@start_date .. @end_date) | ||
|
|
||
| # User: I want an exception raised when an invalid date range is provided, so that I can't make a reservation for an invalid date range |
There was a problem hiding this comment.
I like including the user stories as comments. 😄
|
|
||
| # User: I want an exception raised when an invalid date range is provided, so that I can't make a reservation for an invalid date range | ||
| if @end_date < @start_date || @start_date == @end_date | ||
| raise ArgumentError.new("Your check-out date must be after your check-in.") |
There was a problem hiding this comment.
It's helpful to include the bad arguments in your error messages:
| raise ArgumentError.new("Your check-out date must be after your check-in.") | |
| raise ArgumentError.new("Your check-out date (#{@start_date}) must be after your check-in (#{end_date}).") |
| overlap = !(daterange_instance.start_date >= end_date || daterange_instance.end_date <= start_date) | ||
| return overlap |
There was a problem hiding this comment.
You don't have to assign this to a variable before you return it:
| overlap = !(daterange_instance.start_date >= end_date || daterange_instance.end_date <= start_date) | |
| return overlap | |
| return !(daterange_instance.start_date >= end_date || daterange_instance.end_date <= start_date) |
Also, you can simplify/clarify this using DeMorgan's Law and reversing the comparisons:
| overlap = !(daterange_instance.start_date >= end_date || daterange_instance.end_date <= start_date) | |
| return overlap | |
| return daterange_instance.start_date < end_date && daterange_instance.end_date > start_date |
| if date >= @start_date && date < @end_date | ||
| return true | ||
| else | ||
| return false | ||
| end |
There was a problem hiding this comment.
You can directly return the condition here:
| if date >= @start_date && date < @end_date | |
| return true | |
| else | |
| return false | |
| end | |
| return date >= @start_date && date < @end_date |
| attr_accessor :reservations, :rooms, :calendar | ||
|
|
||
| def initialize | ||
| @rooms = (1..20).map { |i| i } |
There was a problem hiding this comment.
It looks like you're just using map to convert this to an array. You can use to_a for that:
| @rooms = (1..20).map { |i| i } | |
| @rooms = (1..20).to_a |
| def populate_calendar(reservation) | ||
| temp = reservation.date_range.dates | ||
| temp.pop | ||
| temp.each do |date| | ||
| if !@calendar.key?(date) | ||
| @calendar[date] =[] | ||
| end | ||
| @calendar[date] << reservation | ||
| end | ||
| return @calendar | ||
| end |
There was a problem hiding this comment.
I think you probably meant to copy the array here. This doesn't do that so you wind up changing dates.
Cleaner would be to ask for a range that excludes the end of dates (you can also use an unless to tidy up your condition):
| def populate_calendar(reservation) | |
| temp = reservation.date_range.dates | |
| temp.pop | |
| temp.each do |date| | |
| if !@calendar.key?(date) | |
| @calendar[date] =[] | |
| end | |
| @calendar[date] << reservation | |
| end | |
| return @calendar | |
| end | |
| def populate_calendar(reservation) | |
| reservation.date_range.dates[0...-1].each do |date| | |
| unless @calendar.key?(date) | |
| @calendar[date] = [] | |
| end | |
| @calendar[date] << reservation | |
| end | |
| return @calendar | |
| end |
There was a problem hiding this comment.
Also, since this method doesn't directly address a user story I'd recommend making it private.
| def list_rooms | ||
| return @rooms | ||
| end |
There was a problem hiding this comment.
You don't need this and the attr_accessor for @rooms. I'd recommend picking one or the other.
| def range_reservations(requested_dates) | ||
| range_resv = [] | ||
| requested_dates.dates.each do |date| | ||
| if @calendar.key?(date) | ||
| range_resv.push(*@calendar[date]) | ||
| end | ||
| end | ||
| range_resv = range_resv.uniq | ||
|
|
||
| return range_resv | ||
| end |
There was a problem hiding this comment.
You can clean this up using select and flat_map:
| def range_reservations(requested_dates) | |
| range_resv = [] | |
| requested_dates.dates.each do |date| | |
| if @calendar.key?(date) | |
| range_resv.push(*@calendar[date]) | |
| end | |
| end | |
| range_resv = range_resv.uniq | |
| return range_resv | |
| end | |
| def range_reservations(requested_dates) | |
| return requested_dates.dates.select do |date| | |
| @calendar.key?(date) | |
| end.flat_map do |date| | |
| @calendar[date] | |
| end.uniq | |
| end |
(Since it's kind of obscure: flat_map is equivalent to doing a map followed by a flatten. It basically says "go through the input and produce an array out of the little arrays this block outputs".)
2 participants
Assignment Submission: Hotel
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection