-
Notifications
You must be signed in to change notification settings - Fork 40
Space - Lak #23
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 - Lak #23
Conversation
Design Scaffolding for the Hotel project
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 grouped data into well thought out classes and thoroughly tested all parts of your program. Testing is a tough skill to master, but your tests cover your code very well, and also explain the reasons for each test clearly! Also, the way you broke up data between the different classes showed a lot of forethought on your part, letting each class represent a unique and tightly clustered set of values. I do see some room for improvement around grouping functionality under the appropriate class. Specifically, I left comments on how your Here are the warnings your code produces: Code Style Bonus AwardsWas the code particularly impressive in code style for any of these reasons (or more...?)
|
| class HotelBlock | ||
| attr_reader :date_range , :rooms , :discounted_rate | ||
| def initialize(date_range, rooms, discounted_rate) | ||
| @date_range = date_range | ||
| raise ArgumentError.new("A block can contain a maximum of 5 rooms") if rooms.length > 5 | ||
| raise ArgumentError.new("The hotel block cannot be made without having a room") if rooms.empty? | ||
| @rooms = rooms | ||
| @discounted_rate = 0.1 | ||
| 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.
A class that only has an initialize method is usually a sign that this didn't need to be a fully fledged class. With this method, it seems like you could have done the work of the HotelBlock class using a hash instead.
Sometimes what this means is that you are letting the wrong class take on responsibilities that belong to this data.
| class NoRoomAvailableError < StandardError | ||
| 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.
This doesn't have to be in it's own file! :)
| def ==(other) | ||
| self.id == other.id | ||
| 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.
Nice work overwriting this method!
| @room1 = Hotel::Room.new(1, 200) | ||
| @room2 = Hotel::Room.new(2, 200) | ||
| @room3 = Hotel::Room.new(3, 200) | ||
| @room4 = Hotel::Room.new(4, 200) | ||
| @room5 = Hotel::Room.new(5, 200) | ||
| @room6 = Hotel::Room.new(6, 200) | ||
| @room_array1 = [@room1, @room2] | ||
| @room_array2 = [@room1,@room2,@room3,@room4,@room5,@room6] | ||
| @room_array3 = [] | ||
| @start_date = Date.today | ||
| @end_date = @start_date + 3 | ||
| @date_range = Hotel::DateRange.new(@start_date,@end_date) | ||
| @discount_rate = 0.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.
Do you need all of this data before each test?
| it "returns true for a range that overlaps in the back" do | ||
| start_date = Date.new(2017, 02, 02) | ||
| end_date = start_date + 10 | ||
| test_range = Hotel::DateRange.new(start_date, end_date) | ||
| expect(@range.overlap?(test_range)).must_equal true | ||
| end | ||
|
|
||
| it "returns true for a containing range" do | ||
| start_date = Date.new(2017, 02, 02) | ||
| end_date = start_date + 1 | ||
| test_range = Hotel::DateRange.new(start_date, end_date) | ||
| expect(@range.overlap?(test_range)).must_equal true | ||
| end | ||
|
|
||
| it "returns false for a range starting on the end_date date" do | ||
| start_date = Date.new(2017, 02, 04) | ||
| end_date = start_date + 10 | ||
| test_range = Hotel::DateRange.new(start_date, end_date) | ||
| expect(@range.overlap?(test_range)).must_equal false | ||
| end | ||
|
|
||
| it "returns false for a range ending on the start_date date" do | ||
| start_date = Date.new(2017, 01, 28) | ||
| end_date = Date.new(2017, 02, 01) | ||
| test_range = Hotel::DateRange.new(start_date, end_date) | ||
| expect(@range.overlap?(test_range)).must_equal false | ||
| 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.
Love how thorough your tests are!
| end | ||
|
|
||
| describe "cost" do | ||
| it "returns a number" do |
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 isn't a great name for this test, because you are also checking the value of that number. I'd write
| it "returns a number" do | |
| it "returns the correct cost as a Number" do |
| def remove_room_from_hotel_block(room, start_date, end_date) | ||
| specific_hotel_block = specific_hotel_block(room, start_date, end_date) | ||
| specific_hotel_block.rooms.delete(room) | ||
| return specific_hotel_block | ||
| 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.
This seems like something the HotelBlock should be in charge of to me.
| def available_rooms_of_block(hotel_block) | ||
| if hotel_block.rooms.empty? | ||
| raise ArgumentError.new("The hotel block cannot be made without having a room") | ||
| else | ||
| return hotel_block.rooms | ||
| end | ||
| 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.
This method also seems like something the HotelBlock should be in charge of. if it were in the HotelBlock, it might look like this
| def available_rooms_of_block(hotel_block) | |
| if hotel_block.rooms.empty? | |
| raise ArgumentError.new("The hotel block cannot be made without having a room") | |
| else | |
| return hotel_block.rooms | |
| end | |
| end | |
| def available_rooms | |
| if self.rooms.empty? | |
| raise ArgumentError.new("The hotel block cannot be made without having a room") | |
| else | |
| return rooms | |
| end | |
| end |
Moving this code both makes HotelController easier to read, and makes for fewer chained method calls.
Assignment Submission: Hotel
Congratulations! You're submitting your assignment. Please reflect on the assignment with these questions.
Reflection