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, and Eagle code. #20

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

Conversation

iovortex
Copy link

With the Tiger discussion, I couldn't tell whether you really wanted the "average year" or the "average in a year", so I did both. :-)

There's a couple of spots I have comments/questions about that I'll add inline.

dictionary
end

def separate_by_years
Copy link
Author

Choose a reason for hiding this comment

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

I feel like with all of the wiz-bang stuff in Ruby "decompose this array into multiple sub arrays based on a block's evaluation" should be easier, but I couldn't find out to do it without the recursion in separate_by_years and array_of_years. Any suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

I think one of the reasons you're having trouble is that array feels too difficult here. I think hash is the appropriate data type here.

def movies_by_year(movies)
  hash = {}
  movies.each do |movie|
    hash[movie.year] ||= []
    hash[movie.year] << movie
  end
  hash
end

ok, so now you have a hash by year, which is easier to deal with.

In ruby, the data-type most often reached for is the hash. arrays are generally pretty stupid and simple lists.

Copy link
Author

Choose a reason for hiding this comment

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

Duh. That makes a lot more sense. Let me refactor this stuff and let you take another look. Thanks.

@jwo
Copy link
Member

jwo commented Jul 29, 2013

Looks entirely technically correct, good job! Do you want to re-work the Collection class with the hash?

otherwise, here are some style notes:

  • use 2 space tabs (think you're at 4)
  • use :need_mode_data instead of :NeedMoreData

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