Class methods discussion

gistfile1.rb

  def user_with_highest_kilometers
    highest = User.new
    users.each do |user|
      highest = user if user_distance_sum(user) > user_distance_sum(highest)
    end
    highest
  end

  def user_with_highest_ascent
    highest = User.new
    users.each do |user|
      highest = user if user_ascent_sum(user) > user_ascent_sum(highest)
    end
    highest
  end

  def user_with_highest_achievements
    highest = User.new
    users.each do |user|
      highest = user if user_achievement_sum(user) > user_achievement_sum(highest)
    end
    highest
  end

  private

  def user_distance_sum(user)
    sum_stat(user, :distance)
  end

  def user_ascent_sum(user)
    sum_stat(user, :ascent)
  end

  def user_achievement_sum(user)
    sum_stat(user, :achievements)
  end

  def sum_stat(user, stat_to_sum)
    user.activities.where("date >= '" + start_date.to_s + "' and date <= '"\
      + end_date.to_s + "'").sum(stat_to_sum)
  end

As a rule of thumb, you know you are using the wrong iterator when you have to set a local variable. In each of the 3 public methods you are using the each iterator - each in ruby means "for each of these items do this". In other languages what you're doing is considered the norm because you don't have access to a rich set of iterators. You want to take a look at the documentation for Enumerable. In there you'll see there is a method called max - using max the first method can be rewritten much more concisely:

def user_with_highest_kilometers
  users.max { |a, b| user_distance_sum(a) > user_distance_sum(b) } || User.new
end

I haven't tested this but I think it should work.

In our skype chat I mentioned that method prefixes are typically a sign that the method is on the wrong class. If we take user_with_highest_kilometers for example it could be rewritten to move the methods to user.

class Challenge
  def user_with_highest_kilometers
    users.highest_kilometers_for(self)
  end

  def period
    OpenStruct.new(start: start_date, finish: end_date)
  end
end

class User
  def self.highest_kilometers_for(challenge)
    activities.total_stat_between(challenge.period)
  end
end

class Activity
  def self.total_stat_between(period)
    between(period).sum(state_to_sum)
  end

  def self.between(period)
    where("date >= ? and date <= ?", period.start, period.finish)
  end
end

Extra stuff to show Justin:

ActiveRecord has date range support out of the box:

class Challenge
  def period
    start_date..end_date
  end
end

class Activity
  def self.between period
    where(date: period)
  end
end

Also, User#highest_kilometers_for(challenge) couples User to Challenge, it should take the period as the argument instead.

class Challenge
  def user_with_highest_kilometers
    User.highest_kilometers_for(users, period)
  end

  def period
    OpenStruct.new(start: start_date, finish: end_date)
  end
end

class User
  def self.highest_kilometers_for(users, period)
    users.max { |a, b| a.activities.total_stat_between(period) > b.activities.total_stat_between(period) }
  end
end

class Activity
  def self.total_stat_between(period)
    between(period).sum(:distance)
  end

  def self.between(period)
    where("date >= ? and date <= ?", period.start, period.finish)
  end
end

class Challenge
  def user_with_highest_kilometers
    users.highest_kilometers_within_period period
  end

  def period
    OpenStruct.new(start: start_date, finish: end_date)
  end
end

class User
  def self.highest_kilometers_within_period period
    all.max { |a, b| a.activities.total_distance_between(period)\
      <=> b.activities.total_distance_between(period) }
  end
end

class Activity
  def self.total_distance_between period
    between(period).sum(:distance)
  end

  def self.between period
    where(date: period.start..period.finish)
  end
end

NB - all. in a class method has replaced scope, this does the scoping of the data, tricky ha. :)