GreenBOX Project _STHOMAS by Sabineth17 · Pull Request #1 · Ada-C10/greenbox

Sabineth17 · 2018-09-10T19:39:53Z

Last updates on the GreenBox and tests to be added and updated this afternoon.

…method

…es rent_movie method writtent

…le method still failing

CheezItMan

Overall pretty good waves 1-2.

You do need more work on available_movies and rent_movie and the tests.

You have a good number of commits and good commit messages and the code is overall pretty well done. I'd like you to first look through the MovieReserver class and the two incomplete methods and then work on the tests with Andrew if you can.

CheezItMan · 2018-09-10T19:45:17Z

+      if (date >= @start_date) && (date < @end_date)
+        return true
+      else
+        date < @start_date || date >= @end_date


You don't need this 2nd conditional since if it's not inside you can return false.

if (date >= @start_date) && (date < @end_date) return true else return false end

CheezItMan · 2018-09-10T19:45:35Z

+
+
+# `overlaps(other_date_range)` - This method takes another date range as a parameter and returns `true` if the date ranges overlap.
+    def overlaps(other_date_range)


CheezItMan · 2018-09-10T19:45:58Z

+      @actors = actors
+    end
+
+    movie = GreenBox::Movie.new(4,'Happiness','Fox',actors:['Denzel Washington', 'LL Cool J'])


You can take this line out

CheezItMan · 2018-09-10T19:46:43Z

+    def initialize
+      @movies = MovieReserver.load_movies
+      @rentals = []
+      @date_range = GreenBox::DateRange.new(Time.parse('2018-08-08'),Time.parse('2018-08-09'))


This @date_range is unnecessary since there isn't one daterange needed by the system. Instead each Rental will have it's own DateRange

CheezItMan · 2018-09-10T19:47:33Z

+        publisher = line[2]
+        all_actors = line[3]
+        actors_names = all_actors.split(':')
+        actors = {actors: actors_names}


You could also make this:

actors = actors_names

And just use an array instead of a has containing one array.

CheezItMan · 2018-09-10T19:48:21Z

+      # them with the ones in the movie list
+      #
+
+      if @rentals.length > 0


Since you have the previous if statement, you don't need this one.

CheezItMan · 2018-09-10T19:50:42Z

+        @rentals.each do |movie|
+          movie.id
+          if @movies.include(movie.id)
+          available_movies = @movies.delete(movie.id)


Since delete will remove any matching movies from the@movies array permanently, you would do better to:

Make a copy of @movies

Use .reject to remove movies that match the given id

Return that resulting list of movies

CheezItMan · 2018-09-10T19:51:13Z

+    end

+# *   `cost`:  This method will return the cost of the rental.  A movie rental will cost $3.0 per night.  The customer is **not** charged for the day the movie is checked in.  So a movie checked out on August 8th and checked in August 10th would cost $3.00 * 2 days = $6.00
+    def cost


CheezItMan · 2018-09-10T19:52:50Z

+    it 'will not include rented movies' do
      date_range = GreenBox::DateRange.new(Time.parse('2018-08-08'), Time.parse('2018-08-09'))
-      reserver.rent_movie('Crazy Rich Asians', date_range, 'Ada Lovelace')
+      #reserver.rent_movie('Crazy Rich Asians', date_range, 'Ada Lovelace')


If you're going to comment this out, you'll need to manually add a rented movie to the system.

reserver.rentals << Rental.new('movie title', Time.parse('2018-08-08'), Time.parse('2018-08-09'))

CheezItMan · 2018-09-10T19:53:57Z

+      # assert
+          ## the rent_movie methods will return
+
+      @rental.rent_movie(id = 3)


It might be easier just to give the title of the movie, although you could do it by ID. However you'll need a MovieReserver instance.

You are right these tests below need work. You might try replicating some of the format of the tests written for you.

…nt_movies and available_movies

… 2 test failrures and 1 test error

CheezItMan

I left a few more suggestions here, your tests are not bad, and they are catching some bugs in your code.

Some strong improvement here.

CheezItMan · 2018-09-13T03:24:44Z

-    xit 'cannot rent a movie already rented' do
-      # TODO Your Code goes here

+    it 'cannot rent a movie already rented' do


This is a well written test! 👍

Nice work!

CheezItMan · 2018-09-13T03:26:55Z

+    # *   `rent_movie(movie_title, date_range, customer_name)`:  This method will attempt to reserve a movie with the given title for the given date range.  If the movie is not available in that range the method should raise a `StandardError`.
+
+    def rent_movie(movie_title, date_range, customer_name)
+      @movies.each do |movie|


As your test shows, this isn't working because it always rents a movie as long as the movie is in the inventory.

If I can make a suggestion:

See if the list of available_movies has this movie title, you can use .each or find_by to do so.

If so, you can create a rental,

add it to the list

return the rental

CheezItMan · 2018-09-13T03:29:35Z

+
+      #Assertions - something that has changed that tells
+      #us we have the expected result
+      expect(rental).wont_be_nil


This is good, you also might want to test that
expect(rental.movie.title).must_equal title (the title of the movie being rented is right.

You can also test that the number of rentals has increased by one (so the length of rentals increased by one).

CheezItMan · 2018-09-13T03:30:58Z

    it 'can rent multiple movies with the same title' do
-      # TODO Your Code goes here

+      movie_reserver  = GreenBox::MovieReserver.new


Nicely done!

CheezItMan · 2018-09-13T03:40:59Z

+          # @movies is a MovieReserver object that contains movies.
+          available_movies = []
+          @movies.each do |movie|
+            if @rentals.date_range == date_range


This part is not quite right. @rentals is an array and so doesn't have a date_range attribute.
Instead you should:

make a copy of the movies list

then loop through your rentals array and remove any movies who have a rental that conflicts.

You can look up the select method to find movies that don't have a given ID or object id.

Sabineth17 added 16 commits September 4, 2018 16:11

Created movie class with attributes and created starring(actor_name) …

c35757f

…method

Started contain date_range method

712ed68

Successfully passed all tests for contains method

aadb210

created and successfully passed method nights to tally number nights

519749f

Completed overlaps methods and tests

f062626

Trying to refactor an adjust my rental initialize methods

651442d

Trying to update my methods for Rental so the test can pass

bf943a9

Wave 2 Rental initialization and cost methods completed

06db929

Created a self.load_movies method, still need to instantiate

d6d83ef

Add coverage directory to .gitignore file

e4528e1

added Simplecov in spec files

8bb4291

finalized self.load_movies and passed all instantialization tests

2a143a8

Working through available movies method

f58479f

Test0001 of movies available passed. Test002 failed because it requir…

4908fae

…es rent_movie method writtent

Updated code and test code to 'hide' rent_movie method. Movie availab…

a665208

…le method still failing

Tests and Codes to be updated this afternoon

d2275d4

CheezItMan reviewed Sep 10, 2018

View reviewed changes

Sabineth17 added 3 commits September 10, 2018 18:36

rent_movie methods are almost completed

704b131

All tests for rent_movies are written, need to ccomplete codes for re…

7bbb903

…nt_movies and available_movies

Still working on completing available_movies and rent_movies methods.…

e3167e4

… 2 test failrures and 1 test error

CheezItMan reviewed Sep 13, 2018

View reviewed changes



		# `overlaps(other_date_range)` - This method takes another date range as a parameter and returns `true` if the date ranges overlap.
		def overlaps(other_date_range)

Conversation

Sabineth17 commented Sep 10, 2018

Uh oh!

CheezItMan left a comment

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!

CheezItMan left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants