Skip to content

Ports - Jillianne Ramirez#41

Open
jillirami wants to merge 28 commits intoAda-C11:masterfrom
jillirami:master
Open

Ports - Jillianne Ramirez#41
jillirami wants to merge 28 commits intoAda-C11:masterfrom
jillirami:master

Conversation

@jillirami
Copy link
Copy Markdown

Hotel

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What was a design challenge that you encountered on this project? Determining the relationship between the hotel manager, the rooms and the reservations and what they knew about each other.
What was a design decision you made that changed over time over the project? I initially did not have a date range class, and as I created multiple ArgumentErrors regarding date, I created this class to dry my code.
What was a concept you gained clarity on, or a learning that you'd like to share? I gained clarity on dependence of classes and how change in one class can affect another.
What is an example of a nominal test that you wrote for this assignment? What makes it a nominal case? it "makes a reservation", testing that the code works normally.
What is an example of an edge case test that you wrote for this assignment? What makes it an edge case? it "raises an exception if a block is the wrong size" , testing the border of possible inputs
How do you feel you did in writing pseudocode first, then writing the tests and then the code? I feel that I jumped around in the process a lot, especially in the beginning. I would focus on pseudocode more at first next time rather than almost attempting BUFD.

@kaidamasaki
Copy link
Copy Markdown

Hotel

What We're Looking For

Feature Feedback
Baseline
Used git regularly Yes.
Answer comprehension questions Yes.
Design
Each class is responsible for a single piece of the program Mostly. HotelManager has a little more functionality than I'd like.
Classes are loosely coupled Yes.
Wave 1
List rooms Yes.
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 Yes.
Test coverage Yes.
Wave 2
View available rooms for a given date range Yes.
Reserving a room that is not available produces an error Yes. (Rooms are selected automatically, an error is raised if no rooms are free.)
Test coverage Yes.
Wave 3
Create a block of rooms Yes.
Check if a block has rooms Yes.
Reserve a room from a block No.
Test coverage Yes.
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 Yes.
Appropriately uses iterators and Enumerable methods Yes.
Appropriately writes and utilizes classes Yes.
Appropriately utilizes modules as a namespace Yes.
Wrap Up
There is a refactors.txt file Yes.
The file provides a roadmap to future changes Yes.
Additional Feedback Great job! There were a few small things but over all this is a really solid solution!

Comment thread lib/block.rb
class Block
attr_reader :start_date, :end_date, :room_collection, :discount_rate

def initialize(start_date:, end_date:, room_collection: [], discount_rate: 0.8)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would have liked to see Block use a DateRange object internally instead of just having a start and end date.

Comment thread lib/date_range.rb
@@ -0,0 +1,24 @@
module HotelManagementSystem
class DateRange
def self.is_valid?(start_date, end_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.

I'd like to see this turned into a constructor that raises since a date range is a thing you can perform meaningful operations on.

Comment thread lib/hotel_manager.rb
@blocks = []

1..20.times do |i|
room = HotelManagementSystem::Room.new(room: i + 1)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ruby will be looking for names locally first, prioritizing local names, since they're all in the same module, there's no need to namespace Hotel::Room. Room should just work! Same with the rest of the namespacing in the code.

Comment thread lib/hotel_manager.rb
end

def reserve(start_date, end_date)
if !HotelManagementSystem::DateRange.is_valid?(start_date, end_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.

You can use unless to make this a little clearer.

Suggested change
if !HotelManagementSystem::DateRange.is_valid?(start_date, end_date)
unless HotelManagementSystem::DateRange.is_valid?(start_date, end_date)

Comment thread lib/hotel_manager.rb

# assigns first available room to reservation
room_assignment = list_available_rooms(start_date, end_date).first
raise ArgumentError, "There is no room available for these dates." if room_assignment == nil
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Very slightly cleaner.

Suggested change
raise ArgumentError, "There is no room available for these dates." if room_assignment == nil
raise ArgumentError, "There is no room available for these dates." unless room_assignment

Comment thread lib/hotel_manager.rb
end

def cost_of_block(block)
block = @blocks.find{|i| i == block}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think all this line does is give you back nil if block isn't in @blocks.

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