Movie JSON submission #26

codeblahblah · 2014-02-19T21:44:43Z

@jwo Submission of Panda and Tiger levels.

codeblahblah · 2014-02-20T03:09:14Z

@jwo Eagle level has so much Math...
Question:
"1. Tiger Level, plus display the average year for the ratings for the movie selected." - Are you asking to calculate the average rating for all movies in the searched year?
i.e. expand on the API methods

…working.

codeblahblah · 2014-02-20T09:02:19Z

@jwo Some questions around RSpec before I continue with the Eagle level:

When I run the rspec command in order to run multiple specs, some of the assertions in movie_spec.rb fail due to api_spec.rb executing first. How do you change scope of RSpec within a file as well as a test suite.
How do you recreate this scenario using stub/mock objects?

describe ".average_rating" do
    it "should have no average rating initially" do

    end

Can I do the above using my own stub/mock Movie objects where I have only set the @score?

jwo · 2014-02-20T15:41:57Z

When I run the rspec command in order to run multiple specs, some of the assertions in movie_spec.rb fail due to api_spec.rb executing first. How do you change scope of RSpec within a file as well as a test suite.

Generally this means your tests have side-effects, and generally should not happen. Is this happening now on your pull-request?

How do you recreate this scenario using stub/mock objects?

Seems you wouldn't need to -- you could just check the average-rating before calling anything and make sure it's 0, or that it throws an exception.

Can I do the above using my own stub/mock Movie objects where I have only set the @score?

Maybe? But usually you don't want to think about internal instance variables. It's a sign that you should pass in a data-object --- to separate your data from your data-methods.

jwo · 2014-02-20T15:59:44Z

spec/movie_spec.rb

+    end
+
+    it 'should calculate the average rating while ignoring the Movie#build calls above' do
+      #how do you change scope for RSpec object. Or rather how do you reset the Movie object instance variables?


Movie.new will always give you a new object.

codeblahblah · 2014-02-20T18:08:26Z

@jwo Hmmm. I'm using Movie#build to keep track of the Movies searched.
You ask "Display the average movie Rating". Is this for both the searched Movies and for Movies relative to the current search?
Also, it seems RottenTomatoes protects its data by not revealing all the "blockbuster" movies via their API. As such, I'm getting Documentaries returned with a -1 critic rating....

jwo · 2014-02-20T18:13:35Z

I'm using Movie#build

I see you're storing the movies in a class variable. You may want to move this to another class, like "MovieLibrary"

it seems RottenTomatoes protects its data by not revealing all the "blockbuster" movies via their API. As such, I'm getting Documentaries returned with a -1 critic rating....

Maybe try older movies:

OH HAI. Search?
y
Found: 10 Things I Hate About You. Score: 61
Search Again (Y/N)
y
OH HAI. Search?
Groundhog Day
Found: Groundhog Day. Score: 97
Search Again (Y/N)
y
OH HAI. Search?
3 Days To Kill
Found: 3 Days To Kill. Score: -1
Search Again (Y/N)
y
OH HAI. Search?
Top Gun
Found: Top Gun. Score: 55
Search Again (Y/N)
y
OH HAI. Search?

codeblahblah · 2014-02-20T18:37:28Z

@jwo Gotcha. Am I keeping track of just the searched movies in MovieLibrary?
Should I exclude the -1 rated movies?
Also, the API does not provide for a way to search for movies by year or any other field. What I'm doing is looping through the JSON request and pulling out the the entries from the "movies" key which has a sub-array.
Is this correct?
I'm thinking that my final output should read:

OH HAI. Search?
Top Gun
Found: Top Gun. Score: 55
Average Score for the year: 99
Search Again (Y/N)

jwo · 2014-02-20T18:39:29Z

Am I keeping track of just the searched movies in MovieLibrary?
yes

Is this correct?

yep, you have an excellent collection of searching capabilities in Ruby. The API should just give you some data to slice and dice.

your final output looks like a fine start

codeblahblah · 2014-02-20T19:17:50Z

@jwo How do I create a mock Movie instance in order to check if it is cataloged in the libray?

describe "#register" do
    it 'should add a new movie to the library whenever a search happens' do
      movie = mock(:movie) # some API class call
      expect(MovieLibrary.all_movies).to include (movie)
    end
    xit 'should increase the movie count' do

    end
  end

Is there a rule of thumb regarding when to mock/stub?

jwo · 2014-02-20T20:30:46Z

you would stub Api.search_by_title to force it to return what you wanted

Is there a rule of thumb regarding when to mock/stub?

You should stub out methods when you want to force a certain state in order to test it.
You should send in a mock as a dependency to another colaborator.

codeblahblah · 2014-02-20T20:46:46Z

@jwo You've kinda lost me.
New code is up. Any comments?
MovieLibrary#average_rating is returning NaN?

jwo · 2014-02-20T20:48:00Z

lib/api.rb

@@ -16,6 +18,24 @@ def self.search_by_title(title)
             )
  end

+  def self.search_by_year(year)


I don't really know why this method exists.

jwo · 2014-02-20T20:50:47Z

You're getting NaN because you have two repositories. Movies' class variable, and the MovieLibrary. Simplify that to 1 and your problems should go away.

codeblahblah · 2014-02-20T20:59:30Z

@jwo Movie class looks a lot cleaner/readable.
NaN issue still persists when running movie_library_spec.rb

jwo · 2014-02-20T21:03:01Z

For the future, please include the full exception message -- it gives you a hell of a lot of information:

  1) MovieLibrary#average_rating should have no average rating when no movies are cataloged
     Failure/Error: movie_library.average_rating.should eq(0)

       expected: 0
            got: NaN

       (compared using ==)
     # ./spec/movie_library_spec.rb:33:in `block (3 levels) in <top (required)>'

jwo · 2014-02-20T21:03:40Z

What happens when you divide by 0 in real life?

codeblahblah · 2014-02-20T21:10:00Z

@jwo Hmmmmm.
How do I add the DivisionByZero check to

  def average_rating
    result = all_movies.inject(0.0) { |sum, movie| sum + movie.score } / all_movies.size
  end

jwo · 2014-02-20T21:12:02Z

If you have a specification that it should return 0, then you'll need to pre-check:

  def average_rating
    return 0 if movie_count == 0
    all_movies.inject(:)/ all_movies.size
  end

codeblahblah · 2014-02-20T21:17:46Z

@jwo Ditto on the full exception message.

codeblahblah · 2014-02-20T22:18:29Z

@jwo The yearly average is calculating correctly but I feel there some refactoring needed below...
I can see the duplication but I'm unsure about how to move things about.

 def average_rating_by_year(year)
    result = all_movies.select {|movie| movie.year == year}
    return 0 if result.length == 0
    result.inject(0.0) { |sum, movie| sum + movie.score } / result.size
  end

  def average_rating
    return 0 if count == 0
    all_movies.inject(0.0) { |sum, movie| sum + movie.score } / all_movies.size
  end

jwo · 2014-02-20T22:20:19Z

Give it a try -- I'll give what I like after you try.

codeblahblah · 2014-02-20T22:40:15Z

@jwo Refactored it to.

  def average_rating_by_year(year)
    result = all_movies.select {|movie| movie.year == year}
    return 0 if result.length == 0
    average_rating(result)
  end

  def average_rating(movies)
    return 0 if count == 0
    movies.inject(0.0) { |sum, movie| sum + movie.score } / movies.size
  end

Is the use result = bad style?

jwo · 2014-02-21T05:45:33Z

I think you could get away without it:

  def average_rating_by_year(year)
    average_rating all_movies.select {|movie| movie.year == year}
  end

  def average_rating(movies)
    return 0 if count == 0
    movies.inject(0.0) { |sum, movie| sum + movie.score } / movies.size
  end

codeblahblah · 2014-02-21T06:02:12Z

@jwo Wow.

codeblahblah · 2014-02-21T21:31:51Z

@jwo Slope calculation added as:

  def calculate_slope
    sorted_catalog = sort_by_year
    (sorted_catalog.first.score - sorted_catalog.last.score).to_f / (sorted_catalog.last.year - sorted_catalog.first.year).to_f
  end

  def sort_by_year
    all_movies.sort { |movie,another_movie| movie.year <=> another_movie.year }
  end

I feel sorted_catalog = sort_by_year is a code smell. Do you agree?

jwo · 2014-02-21T21:38:41Z

I think it could be an input to the method. Right now, calculate_slope has a dependency that another method exists named sort_by_year. It's not very usable by itself. So, as a general rule, try to pass in dependencies for methods like this.

  def calculate_slope(catalog)
    first = catalog.first
    last = catalog.last
    (first.score - last.score).to_f / (last.year - first.year).to_f
  end

  def sort_by_year (movies)
    movies.sort { |movie,another_movie| movie.year <=> another_movie.year }
  end

The only thing is that the slope should already be calculated when calling it.

codeblahblah · 2014-02-22T15:20:46Z

@jwo Got it!
Made the MovieLibrary#catalog central to setting up the MovieLibrary instance.

  def catalog(movie)
    @movies << movie
    @library = sort_by_year(@movies)
    @slope = calculate_slope(@library)
    return @library
  end

codeblahblah · 2014-02-22T17:15:33Z

@jwo Added a program summary.
Preventing NilClass errors when no movie is found.
Is this good Ruby?

unless movie.nil?
movie_library.catalog(movie)
puts "Found: #{movie.title}. Score: #{movie.score}"
end

And preventing NaN errors when calculating a slope for a single movie.

  def calculate_slope(catalog)
    first = catalog.first
    last = catalog.last
    return 0 if first == last
    (first.score - last.score).to_f / (last.year - first.year).to_f
  end

Is return 0 if first == last the best way to go about checking equaity in terms of working with the same object?

jwo · 2014-02-25T15:04:04Z

Is this good Ruby?

unless movie.nil?
  movie_library.catalog(movie)
  puts "Found: #{movie.title}. Score: #{movie.score}"
end

The intensions are good. However, I would invoke a rule where you only use "unless" as a 1 statement modifier:

# Good
bank_account.withdraw_funds! unless bankrupt?

# Bad
unless bankrupt?
   bank_account.withdraw_funds!
end

So, if you switch to this, it'll read easier.

if movie
  movie_library.catalog(movie)
  puts "Found: #{movie.title}. Score: #{movie.score}"
end

jwo · 2014-02-25T15:05:24Z

Is return 0 if first == last the best way to go about checking equaity in terms of working with the same object?

That works... though you could achieve the same if you did

return 0 if catalog.count <= 1

jwo · 2014-02-25T15:06:16Z

lib/movie_library.rb

+
+  def catalog(movie)
+    @movies << movie
+    @library = sort_by_year(@movies)


Why are you using instance variables here?

codeblahblah · 2014-02-25T16:10:52Z

@jwo I renamed the #catalog to #add and moved the instance variable setting.

  def add(movie)
    movies << movie
    catalog = sort_by_year(movies)
    calculate_slope(catalog)
  end

Set instances like so:

def calculate_slope(catalog)
    first = catalog.first
    last = catalog.last
    return 0 if catalog.count <= 1
    @slope = (first.score - last.score).to_f / (last.year - first.year).to_f
  end

def sort_by_year(movies)
    @catalog = movies.sort { |movie,another_movie| movie.year <=> another_movie.year }
  end

Better?

jwo · 2014-02-25T18:41:30Z

As I'm reading the code, I am wondering if there is significance in the catalog of catalog = sort_by_year(movies)

If there is no significance, then I try to reduce down to the essence

  def add(movie)
    movies << movie
    calculate_slope sort_by_year(movies)
  end

jwo · 2014-02-25T18:41:45Z

Better?

MUCH. yay!

codeblahblah · 2014-02-25T18:54:33Z

@jwo Awesome!
Hopefully I'll get better at sussing out these code smells.

codeblahblah · 2014-02-25T19:01:03Z

@jwo calculate_slope sort_by_year(movies) will return the slope value? Is it okay if a method returns an object not inline with the method name?

jwo · 2014-02-25T19:07:37Z

Sure, check this example out: http://rubyfiddle.com/riddles/8f294

jwo · 2014-02-25T19:08:12Z

Of course, this calculate_scope sort_by_year(movies) is the same thing as

calculate_scope(sort_by_year(movies))

Just more parens

codeblahblah · 2014-02-25T19:24:46Z

@jwo Hmmmmm. Your code looks great!
Seems I was caught up on the return values...

codeblahblah added 3 commits February 19, 2014 23:17

Panda Level complete.

745009b

Added unrefactored Tiger Level.

c7a7953

Added refactored Tiger Level.

0c115b4

codeblahblah added 2 commits February 20, 2014 00:47

Added RSpec file as #expect syntax was failing. Movie.average_rating …

8df31e7

…working.

Added Gemfiles.

ccfed8f

jwo reviewed Feb 20, 2014
View reviewed changes

Added partial yearly search.

910010d

codeblahblah added 2 commits February 20, 2014 21:11

Added MovieLibrary concept.

3b54798

Deleted MovieList class.

32fddc6

Added MovieLibrary#average_rating.

b770147

jwo reviewed Feb 20, 2014
View reviewed changes

Code clean-up.

527e834

Added yearly average method.

b977711

Refactored MovieLibrary#average_rating.

12f27b0

Further refactoring of MovieLibrary#average_rating.

7f81c59

Add slope calculation.

5b786f0

Fixed #calculate_slope. Using #catalog to handle all dependencies.

78d49fd

Added program summary and one movie slope test.

cb1e2c0

jwo reviewed Feb 25, 2014
View reviewed changes

lib/movie_library.rb

def catalog(movie)

@movies << movie

@library = sort_by_year(@movies)

Copy link

Member

jwo Feb 25, 2014

Choose a reason for hiding this comment

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

Why are you using instance variables here?

codeblahblah added 2 commits February 25, 2014 17:49

Removed the use of unless as a 1 statement modifier.

f9814fd

Refactored the use of instance variables and rename the catalog method.

a93af62

Removed verbose calls to catalog.

10a00ea

Added fixes inline with renamed methods

2e367b8

Movie JSON submission #26

Are you sure you want to change the base?

Movie JSON submission #26

Conversation

codeblahblah commented Feb 19, 2014

codeblahblah commented Feb 20, 2014

codeblahblah commented Feb 20, 2014

jwo commented Feb 20, 2014

jwo Feb 20, 2014

Choose a reason for hiding this comment

codeblahblah commented Feb 20, 2014

jwo commented Feb 20, 2014

codeblahblah commented Feb 20, 2014

jwo commented Feb 20, 2014

codeblahblah commented Feb 20, 2014

jwo commented Feb 20, 2014

codeblahblah commented Feb 20, 2014

jwo Feb 20, 2014

Choose a reason for hiding this comment

jwo commented Feb 20, 2014

codeblahblah commented Feb 20, 2014

jwo commented Feb 20, 2014

jwo commented Feb 20, 2014

codeblahblah commented Feb 20, 2014

jwo commented Feb 20, 2014

codeblahblah commented Feb 20, 2014

codeblahblah commented Feb 20, 2014

jwo commented Feb 20, 2014

codeblahblah commented Feb 20, 2014

jwo commented Feb 21, 2014

codeblahblah commented Feb 21, 2014

codeblahblah commented Feb 21, 2014

jwo commented Feb 21, 2014

codeblahblah commented Feb 22, 2014

codeblahblah commented Feb 22, 2014

jwo commented Feb 25, 2014

jwo commented Feb 25, 2014

jwo Feb 25, 2014

Choose a reason for hiding this comment

codeblahblah commented Feb 25, 2014

jwo commented Feb 25, 2014

jwo commented Feb 25, 2014

codeblahblah commented Feb 25, 2014

codeblahblah commented Feb 25, 2014

jwo commented Feb 25, 2014

jwo commented Feb 25, 2014

codeblahblah commented Feb 25, 2014