Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Panda & Tiger level complete #10

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Panda & Tiger level complete #10

wants to merge 11 commits into from

Conversation

ralphos
Copy link

@ralphos ralphos commented Dec 9, 2012

I've calculated the average year of all the movies the user has liked, but I"m not sure if that's what you meant in your instructions?

Would you mind please clarifying the instructions in the Eagle level? Thanks!

year: struct.year,
score: struct.ratings["critics_score"]
)
rescue
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather you be more explicit about catching that there were no movies returned. Here you are returning nil if there was any exception.

@jwo
Copy link
Member

jwo commented Dec 9, 2012

Nice job with the user class and tests -- it flowed well.

As far as explaining the Eagle -- if the slope of the line of your average ratings of movies is increasing, then you're getting happier. If it's decreasing, then you're getting more upset.

@ralphos
Copy link
Author

ralphos commented Dec 10, 2012

I completed the Eagle level, (or at least my interpretation of it!). It basically calculates the slope based off of the year you 'added the movie to your likes'.

So to actually calculate the slope you would have to wait till next year to begin adding movies to see whether you are getting better or worse at picking movies over time. (Notwithstanding, I tried writing some specs to prove this logic indeed works).

@jwo
Copy link
Member

jwo commented Dec 12, 2012

I think this looks great... one minor note: It's the year of the movie that I was thinking would matter on the slope --- if a movie in 1985 you liked was just OK, and in 2010 was GREAT, then the slope would be positive... Not necessarily recording the data in 1985, just that the movie was in 1985.

That said -- you've definitely accomplished this pull. go you!

@ralphos
Copy link
Author

ralphos commented Dec 13, 2012

ah ok, I had a feeling I wasn't following along with the instructions. It was good practice nonetheless!

Any suggestions as to how to better use a begin/rescue block when handling exceptions? I wasn't sure what else to put in the rescue block other than nil when I first tried to use it?

@jwo
Copy link
Member

jwo commented Dec 13, 2012

For begin/rescue I recommend two rules:

  1. only having 1 line of code between begin and rescue
  2. only rescue the specific exception you want

Good:

def safely_do_the_thing(thing)
  begin
    do_the_thing(thing)
  rescue Panda::ThingFailedError
    return NilPanda.new
  end
end

Bad:

def do_the_thing(thing)
  begin
    operation_1.each do |p|
      p.whatever
      p.save
    end
  rescue Exception
    return nil
  end
end

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