Ports - Faiza by Faiza1987 · Pull Request #26 · Ada-C11/hotel

Faiza1987 · 2019-03-18T06:52:03Z

Hotel

Congratulations! You're submitting your assignment!

Comprehension Questions

Question	Answer
What was a design challenge that you encountered on this project?	I was unsure at first how many classes I would need. I re-designed my project twice before getting to Wave 3, and then redesigned it again because it made some of the previous classes redundant.
What was a design decision you made that changed over time over the project?	I started out with having a Room class that I decided to scrap because it was not doing anything at all and was not communicating with any other classes.
What was a concept you gained clarity on, or a learning that you'd like to share?	I discovered that I should keep a bigger view of the project in mind while writing the basic functionality so that it is easier to refactor and fits better in the larger scheme of things.
What is an example of a nominal test that you wrote for this assignment? What makes it a nominal case?	I wrote a test to make sure that there were no more than 20 elements in the @all_rooms array
What is an example of an edge case test that you wrote for this assignment? What makes it an edge case?	I wrote a test that would raise an ArgumentError if the check_out date was earlier than the current date.
How do you feel you did in writing pseudocode first, then writing the tests and then the code?	I wrote pseudocode for the larger project and for individual methods which on paper seemed it would work very well but I had to deviate from the pseudocode while writing the actual code to get the desired results.

…d_reservation method

…e method in Main.rb. Got previous failing tests working and wrote a new test test that is bith failing and passing when guard is run multiple times. Need to fix.

…d to fix previous issue but could not.

…les. Made FrontDesk a Class again. Modiefied tests by instantiating FrontDesk in each describe block

…sponding tests as well. Added .round to total_cost

…eservation method to check for overlapping dates for room reservations and to raise error if dates overlap.

…lass. Created a class called BlockRoom for Wave 3. Class has been tested.

… in FrontDesk class

tildeee · 2019-03-23T23:12:24Z

Hotel

What We're Looking For

Feature	Feedback
Baseline
Used git regularly	x
Answer comprehension questions	x
Design
Each class is responsible for a single piece of the program	you made reservations and hotel blocks the same thing, when their state and behaviors are actually quite different
Classes are loosely coupled	in some ways, you set this up well by making helper methods like `reservation_template`. However, in your tests, you didn't use keyword arguments
Wave 1
List rooms	x
Reserve a room for a given date range	x
List reservations for a given date	x
Calculate reservation price	x
Invalid date range produces an error	x
Test coverage	largely there
Wave 2
View available rooms for a given date range	x
Reserving a room that is not available produces an error	x
Test coverage	largely there, missing some test cases
Wave 3
Create a block of rooms	x, with misinterpretation (made block of rooms a kind of reservation)
Check if a block has rooms	x, with misinterpretation
Reserve a room from a block	x, with misinterpretation
Test coverage	largely there
Fundamentals
Names variables, classes and modules appropriately	x
Understanding of variable scope - local vs instance	there were many instances of instance variables being used incorrectly: in contexts in which they had `nil` value
Can create complex logical structures utilizing variables	x
Appropriately uses methods to break down tasks into smaller simpler tasks	x, but there were many more opportunities to make more helper methods
Understands the differences between class and instance methods	x
Appropriately uses iterators and `Enumerable` methods	there was a huge red flag on this: 100% of the time that you could have used an each loop you didn't, and your code became less readable and more confusing because of it. Please reach out if you are unsure how to use an each loop. Similarly, you can even push this skill and use `Enumerable` methods to make it even cleaner, but you didn't get to do that either
Appropriately writes and utilizes classes	x
Appropriately utilizes modules as a namespace	x
Wrap Up
There is a refactors.txt file	x
The file provides a roadmap to future changes	x
Additional Feedback

Good work on this, Faiza-- your design overall ended up to be pretty good, and your tests were full

However, I have a huge concern about some of your code style.

You relied on nesting a lot of ifs/elsifs/elses inside of nested loops, and more loops! With a little thought about how to refactor, you can get rid of a lot of complexity. Similarly, parts of your logic felt strange: what is a first_room_no? Etc.

Similarly, there were a lot of misses on some requirements because of how you implemented room blocks. Room Blocks are not a type of reservation-- even if it may seem like they are.

A lot of my comments are on the following things:

Simplify your logic-- watch your nesting
Use each loops instead of for loops
Misunderstanding about how instance variables work
Confirm that what you're doing follows the requirements!
You have pretty good tests, but there are areas it could improve

I would love to see improvements on all of these areas, especially the each loops, in the near future

Good work on pushing through this project-- your project could definitely grow and improve, but overall you have a submission that got through all the waves.

tildeee · 2019-03-23T23:12:48Z

+    attr_accessor :room_booking_ref, :block_room_number, :check_in, :check_out
+
+    def initialize(room_booking_ref:, block_room_number:, check_in:, check_out:)
+      @room_booking_ref = room_booking_ref


What is this variable? Was there a requirement for it?

tildeee · 2019-03-23T23:13:04Z

+module Hotel
+  class FrontDesk
+    # attr_reader :all_rooms, :reservations_record, :block_reservations_only, :reserved_rooms_in_blocks
+    attr_accessor :all_rooms, :reservations_record, :block_reservations_only, :reserved_rooms_in_blocks


Why did you choose to make this accessor instead of reader? Your tests still run the same way if you make it reader, so I would choose this over accessor

tildeee · 2019-03-23T23:13:14Z

+
+    def all_rooms_array
+      return @all_rooms
+    end


I don't think this method is wrong-- in fact, it is the perfect example of a "getter method"! However, since what you are returning is an instance variable (@all_rooms), there are ways to refactor this to be shorter and smaller. Think on it and let me know what questions you have :)

tildeee · 2019-03-23T23:13:37Z

+        check_out: new_check_out,
+        total_cost: @total_cost,
+        room_blocks: @room_blocks,
+      )


Here, when you're calling Reservation.new, you are saying that the value that should go into the parameter total_cost should be whatever @total_cost is... but @total_cost doesn't have any value stored on it, ever! You never call @total_cost = ... ever in your code, so it will always be nil.

ame with @room_blocks. @room_blocks is the name of the variable that this instance of FrontDesk has stored, but this instance of FrontDesk will never change that value, so it's always nil. What value were you expecting to be on @room_blocks?

tildeee · 2019-03-23T23:15:01Z

+      if (@reservations_record.length == 0)
+        booking = reservation_template(new_booking_ref, new_number_of_rooms, new_first_room_number, new_check_in, new_check_out)
+      else
+        for i in (0...@reservations_record.length)


Why did you choose this for loop instead of an each loop? If you used an each loop, then to refer to each reservation, you can say

@reservations_record.each do |reservation| old_first_room_no = reservation.first_room_number end

note the difference: reservation, as opposed to @reservations_record[i], so it's a lot shorter and cleaner

tildeee · 2019-03-23T23:15:12Z

+        booking = reservation_template(new_booking_ref, new_number_of_rooms, new_first_room_number, new_check_in, new_check_out)
+      else
+        for i in (0...@reservations_record.length)
+          old_first_room_no = @reservations_record[i].first_room_number


What does old_first_room_no represent?

tildeee · 2019-03-23T23:15:24Z

+
+          if ((old_first_room_no >= new_first_room_number && old_first_room_no <= new_last_room_no) ||
+              (old_last_room_no >= new_first_room_number && old_last_room_no <= new_last_room_no) ||
+              ((old_first_room_no <= new_first_room_number) && (old_first_room_no <= new_last_room_no) && (old_last_room_no >= new_first_room_number) && (old_last_room_no >= new_last_room_no)))


What is this logic for?

tildeee · 2019-03-23T23:15:37Z

+            booking = reservation_template(new_booking_ref, new_number_of_rooms, new_first_room_number, new_check_in, new_check_out)
+          end
+        end
+      end


Your variable names and your complex logic here are overwhelming and confusing, and difficult to read. This would be great pulled into helper methods.

tildeee · 2019-03-23T23:15:47Z

+    def get_reservations_by_date(date)
+      reservations_by_date_list = Array.new
+
+      for index in (0...@reservations_record.length)


For every time you do for i in (0...array.length), it is probably better and more clear to use an each loop

tildeee · 2019-03-23T23:15:56Z

+      reservations_at_date = get_reservations_by_date(date)
+
+      available_rooms = Array.new
+      for room_idx in (0...@all_rooms.length)


each loop!

tildeee · 2019-03-23T23:16:12Z

+      available_rooms = Array.new
+      for room_idx in (0...@all_rooms.length)
+        room_booked = false
+        for res_idx in (0...reservations_at_date.length)


each loop!

tildeee · 2019-03-23T23:16:26Z

+      for room_idx in (0...@all_rooms.length)
+        room_booked = false
+        for res_idx in (0...reservations_at_date.length)
+          for i in (0..@reservations_record[res_idx].room_blocks.length - 1)


each loop!

tildeee · 2019-03-23T23:16:40Z

+            end
+          end
+        end
+        if (room_booked == false)


For something this short, no need to put parens: if room_booked == false or even if !room_booked

tildeee · 2019-03-23T23:16:47Z

+        if (room_booked == false)
+          available_rooms << @all_rooms[room_idx]
+        end
+      end


This logic isn't clear-- it might be clearer if you iterated through your reservations first, instead of all rooms and then all reservations. However, ultimately, this code isn't readable because of the long variable names and the nested logic. Having a deep "V" nesting (there are 4 levels of nesting) is an indicator that things should be reworked, refactored, and pulled into helper methods.

tildeee · 2019-03-23T23:17:02Z

+
+    #### BLOCK METHODS ####
+    def add_block_reservations
+      for i in (0...@reservations_record.length)


tildeee · 2019-03-23T23:17:10Z

+          @block_reservations_only << @reservations_record[i]
+        end
+      end
+    end


What is this method accomplishing? Why do we add to @block_reservations_only every reservation with multiple rooms?

tildeee · 2019-03-23T23:17:21Z

+    end
+
+    def make_reservation_in_block(block_room_booking_ref, block_room_number, block_check_in, block_check_out)
+      for i in (0...@block_reservations_only.length)


tildeee · 2019-03-23T23:17:38Z

+
+      rooms_in_blocks = Array.new
+      available_rooms_in_block = Array.new
+      for i in (0...block_reservations_at_date.length)


tildeee · 2019-03-23T23:17:43Z

+      rooms_in_blocks = Array.new
+      available_rooms_in_block = Array.new
+      for i in (0...block_reservations_at_date.length)
+        for j in (0...block_reservations_at_date[i].room_blocks.length)


tildeee · 2019-03-23T23:17:54Z

+    def block_room_availability(date)
+      rooms_in_blocks = all_rooms_in_blocks(date)
+      rooms_available_in_blocks = Array.new
+      for i in (0...rooms_in_blocks.length)


tildeee · 2019-03-23T23:18:01Z

+      rooms_in_blocks = all_rooms_in_blocks(date)
+      rooms_available_in_blocks = Array.new
+      for i in (0...rooms_in_blocks.length)
+        for j in (0...@reserved_rooms_in_blocks.length)


tildeee · 2019-03-23T23:18:20Z

+module Hotel
+  class Reservation
+    attr_reader :booking_ref
+    attr_accessor :booking_ref, :number_of_rooms, :first_room_number, :check_in, :check_out, :total_cost, :room_blocks


You probably should make all of these reader. Also, you don't need to double up-- booking_ref is in both reader and accessor and you should probably pick one

tildeee · 2019-03-23T23:18:29Z

+    DISCOUNTED_RATE = 160.00
+
+    def initialize(booking_ref:, number_of_rooms:, first_room_number:, check_in:, check_out:, total_cost:, room_blocks:)
+      @booking_ref = booking_ref


What is a booking_ref? Is there a requirement for this?

tildeee · 2019-03-23T23:18:38Z

+      @booking_ref = booking_ref
+
+      if (number_of_rooms <= 5)
+        @number_of_rooms = number_of_rooms


There is no requirement that a reservation should have more than one room. Blocks should have different behavior: they are not a type of reservation

tildeee · 2019-03-23T23:18:47Z

+        raise ArgumentError, "A block can only have 5 rooms or less."
+      end # if
+
+      @first_room_number = first_room_number


What is a first_room_number? What does it represent?

tildeee · 2019-03-23T23:18:57Z

+        @total_cost = ((check_out - check_in) * number_of_rooms) * DISCOUNTED_RATE
+      end # if
+
+      @room_blocks = Array.new


Hm, I think that this isn't what the requirements were asking for...

tildeee · 2019-03-23T23:19:12Z

+
+    it "creates an instance of BlockRoom" do
+      expect(@block_room).must_be_kind_of Hotel::BlockRoom
+    end


Tests could be filled out a little more

tildeee · 2019-03-23T23:19:32Z

+      return Hotel::FrontDesk.new
+    end
+
+    describe "all arrays" do


Not the best name for this test! What if these values grow to not be arrays in the future? This is testing that the initial values are correct

tildeee · 2019-03-23T23:19:46Z

+          1,
+          Date.new(2019, 3, 20),
+          Date.new(2019, 3, 24),
+        )


Probably would make sense to use keyword arguments here, since you defined them as such in Reservation

tildeee · 2019-03-23T23:20:01Z

+        expect(by_date).must_be_kind_of Array
+        expect(by_date.length).must_equal 1
+        expect(reservation_2.check_in).must_be :<, Date.new(2019, 4, 10)
+        expect(reservation_2.check_in).must_be :>, Date.new(2019, 4, 7)


You probably don't need to make assertions on reservation2 in this test, since you didn't alter it

tildeee · 2019-03-23T23:20:19Z

+      before do
+        @concierge = build_front_desk
+      end
+      it "will raise an ArgumentError if check_out date is before current date" do


The logic for this test ends up living in the Reservation class, and you even test it there -- no need to double test it here in this file, unless this code was doing something different

tildeee · 2019-03-23T23:20:40Z

+            Date.new(2019, 10, 8),
+            Date.new(2019, 10, 12)
+          )
+        }.must_raise ArgumentError


It isn't clear from this test why this is relevant to the test (it looks like a regular, valid reservation)-- maybe there are ways to use variable names and error detail testing to make this clear

tildeee · 2019-03-23T23:20:54Z

+            Date.new(2019, 7, 5)
+          )
+        }.must_raise ArgumentError
+      end


I would expect more edge cases than this!

tildeee · 2019-03-23T23:21:09Z

+        @concierge.add_block_reservations
+
+        expect(@concierge.block_reservations_only.length).must_equal 1
+      end


What happens if you make two hotel blocks for the same date range?

tildeee · 2019-03-23T23:21:30Z

+            Date.new(2019, 7, 5),
+            Date.new(2019, 7, 6),
+          )
+        }.must_raise ArgumentError


Why should this raise an error? If this is a different case, then you should write a different test for it

tildeee · 2019-03-23T23:22:31Z

+          check_out: Date.new(2019, 5, 24),
+          total_cost: @total_cost,
+          room_blocks: @room_blocks,
+        )


Same as the comments that I had in your FrontDesk file... @total_cost will always be nil, no matter what, so you probably wouldn't want to say total_cost: @total_cost, but just total_cost: nil if that's what you're expecting

…ted two new helper methods to check for room and date overlaps

…ulate room cost, check block size, and populate the room_blocks array

Faiza1987 added 16 commits March 11, 2019 13:16

Initial of Hotel.

f0566ac

Added reservation class with Initialize method and Main class with ad…

82fc650

…d_reservation method

Modified some code in Main.rb and added a self.get_reservation_by_dat…

341d5f5

…e method in Main.rb. Got previous failing tests working and wrote a new test test that is bith failing and passing when guard is run multiple times. Need to fix.

Moved the before do block into the test it was most relevant to. Trie…

d601641

…d to fix previous issue but could not.

Re-named Main to Front_Desk, and changed name in all corresponding fi…

fe587e5

…les. Made FrontDesk a Class again. Modiefied tests by instantiating FrontDesk in each describe block

Added a 'is_room_available?' method in FrontDesk

ffedb14

Changed parameters of 'get_reservation_by_date' and changed the corre…

6eeefec

…sponding tests as well. Added .round to total_cost

Fixed is_room_available method and wrote a test to verify test

3e2ef62

Added a new reservation_template method to FrontDesk. Modified make_r…

6c89668

…eservation method to check for overlapping dates for room reservations and to raise error if dates overlap.

Modified Reservation class for Wave 3. Finished testing Reservation c…

1c387b8

…lass. Created a class called BlockRoom for Wave 3. Class has been tested.

Refactored old tests in front_desk_spec.rb to work with modified code…

7a2d41c

… in FrontDesk class

Added more tests in front_desk_specs.rb

16566e5

Added more tests in front_desk_specs.rb

debfb84

Removed un-needed comments. Added a few more tests.

56fc71b

Added refactors.txt

9ea6f64

Added refactors.txt

e3bc241

tildeee reviewed Mar 23, 2019

View reviewed changes

Faiza1987 added 6 commits March 26, 2019 20:48

Added design-activity.md

aa5c766

Anwered Hotel Revisited question in design-activity.md

4ed6a4f

Modified FrontDesk.rb to use each loops instead of for loops and crea…

169d236

…ted two new helper methods to check for room and date overlaps

Cleaned up initialize method and created 3 additional methods to calc…

b5ea618

…ulate room cost, check block size, and populate the room_blocks array

Modified tests to work with refactored code

7c0b882

Modified tests to work with refactored code

b0cc70e

Conversation

Faiza1987 commented Mar 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Hotel

Comprehension Questions

Uh oh!

tildeee commented Mar 23, 2019

Hotel

What We're Looking For

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Faiza1987 commented Mar 18, 2019 •

edited

Loading