Skip to content

Sockets-Bita#40

Open
Bitaman wants to merge 12 commits intoAda-C11:masterfrom
Bitaman:master
Open

Sockets-Bita#40
Bitaman wants to merge 12 commits intoAda-C11:masterfrom
Bitaman:master

Conversation

@Bitaman
Copy link
Copy Markdown

@Bitaman Bitaman commented Mar 18, 2019

Hotel

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What was a design challenge that you encountered on this project? deciding about how many classes do I need to keep classes single responsibility and at the same time not having too many classes that their only responsibility is to initialize themselves.
What was a design decision you made that changed over time over the project? I had thought about creating a booking system for the hotel but then I decided that it wouldn't be necessary and I can use hotel itself to do all the reservations. also I was tempted to make a date range class but I made my rooms keep track of their reservations and that way it was possible to check the date range inside methods.
What was a concept you gained clarity on, or a learning that you'd like to share? I learned a lot about writing tests. I am not sure how perfect are the test that I wrote for this project but I have a much clearer and better understanding on how to write test now.
What is an example of a nominal test that you wrote for this assignment? What makes it a nominal case? testing to see if the reservation class get initialize correctly is a nominal test.
What is an example of an edge case test that you wrote for this assignment? What makes it an edge case? I think testing for reservations that on the last day of a reservation or end at the start day of one is an edge case. edge cases are cases that are exactly on the boundary of what is acceptable and what is not.
How do you feel you did in writing pseudocode first, then writing the tests and then the code? I started doing that but I caught myself a lot skipping the pseudocode and write the code itself and testing that. but the parts that I tried I really like that it made the whole process of writing code much easier to handle

@dHelmgren
Copy link
Copy Markdown

Hotel

What We're Looking For

Feature Feedback
Baseline
Used git regularly For a project of this size, I would expect more commits.
Answer comprehension questions Yes
Design
Each class is responsible for a single piece of the program Yes, but see comments. things are a bit too broken down in this.
Classes are loosely coupled No, there are a lot of instances of calling instance methods in your Hotel class.
Wave 1
List rooms
Reserve a room for a given date range yes
List reservations for a given date Yes
Calculate reservation price Yes
Invalid date range produces an error No, you could pass in a backwards set of dates and it wouldn't know.
Test coverage The tests aren't testing things broadly enough at this point.
Wave 2
View available rooms for a given date range See comment, this method has a significant bug.
Reserving a room that is not available produces an error No, due to the error above.
Test coverage As above.
Wave 3
Create a block of rooms Yes
Check if a block has rooms Your block doesn't work to spec, it seems you misunderstood part of the prompt.
Reserve a room from a block as above.
Test coverage NA
Fundamentals
Names variables, classes and modules appropriately Yes
Understanding of variable scope - local vs instance yes
Can create complex logical structures utilizing variables Yes
Appropriately uses methods to break down tasks into smaller simpler tasks Yes
Understands the differences between class and instance methods Mostly, but you have a method that seems to have been put in by ritual rather than necessity. See Hotel#self.load_rooms.
Appropriately uses iterators and Enumerable methods yes
Appropriately writes and utilizes classes Utilize yes, but you broke down the components too far.
Appropriately utilizes modules as a namespace no, you didn't use a Module to encapsulate your work.
Wrap Up
There is a refactors.txt file Not Included
The file provides a roadmap to future changes NA
Additional Feedback I can tell you put a lot of work into this submission. You scaffolded out a lot of good elements, but I see that your code has some pretty fundamental errors as well. A lot of this can be salvaged with some better testing, which you admit is something that you're still getting the hang of. I'm going to suggest that you sit down with Dan or Kaida when you get back from break and talk with one of them about how you can catch the things that went wrong here, and how you could improve your testing so that these kinds of mistakes get caught early.

Comment thread lib/room.rb Outdated

def initialize(id)
@id = id
@price = 200
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This could be a constant somewhere else instead!

Comment thread lib/hotel.rb Outdated
@rooms = []
id = 1
20.times do
room = Room.new(id)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Given your implementation of Room below, what is the difference between a Hash of Int:Array pairs and an array of Room objects?

Besides a guaranteed order, I don't think there is one, because the Room class is basically a stub.

Comment thread lib/hotel.rb Outdated
return all_available
end

def self.load_rooms
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why did you need this method?

Comment thread lib/hotel_block.rb Outdated
i = 0
number_of_dates.times do
date = @start_date + i
dates.push(date)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Check out the Range class in Ruby: https://ruby-doc.org/core-2.2.0/Range.html

You can replace this method with the following call: (start_date .. end_date)

Comment thread lib/hotel_block.rb Outdated
# end
@room_rate = room_rate
@cost = ((@dates.length) - 1) * (@room_rate.to_i)
@dates = date_range
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why do you make this call again? super has already called it!

Comment thread lib/hotel.rb
@reservations.each do |reservation|
reserved << reservation.room
end
return @rooms - reserved.uniq
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice work on this! very clear and readable.

Comment thread spec/hotel_block_spec.rb
it "makes an array of all the dates in the blocks date range" do
expect(@block.dates).must_be_kind_of Array
expect(@block.dates.length).must_equal 5
end
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Some tests you should be doing:

  • Can you make a reservation into the block?
  • What if someone tries to book a room in the block and they aren't from the block?
  • What if someone tries to book a room in the block that extends past the block dates?

Comment thread lib/hotel_block.rb
i += 1
end
return dates
end
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Your implementation of block seems to miss one of the points of a block; that is, one needs to be able to book a room that is in the block, on top of creating the block itself.

Comment thread lib/hotel.rb Outdated
available_rooms = []
all_available = []
@reservations.each do |reservation|
if start_date >= reservation.dates.last || end_date <= reservation.dates.first
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This will return true for a room on a single reservation, but that same room could be booked by another reservation. The list of available rooms needs to be built subtractively, not additively.

Example:
load_availables(Jan 3rd 2019, Jan 15th 2019)

if someone has booked room 1 for those exact dates, but there was another booking that happened from Jan 1st to Jan 3rd in room 1, room 1 would be added to the list of available rooms.

Comment thread spec/hotel_spec.rb Outdated
@reservation2 = @hotel.make_reservation(start_date2, end_date2)
start_date3 = Date.new(2018, 3, 5)
end_date3 = start_date3 + 3
@reservation3 = @hotel.make_reservation(start_date3, end_date3)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is a significant flaw in this test: you used the same date range for all 3 bookings. This won't catch the case mentioned in the Hotel#load_availables comment above, where the same room is booked back to back.

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