Simple: Need help refactoring this awkward looking method

Question

I need help refactoring this method. It is way too long.

  def report_total(feed_event, advisor)
    count = 0
    advisor.activity_feed_events.each do |lead|
      if lead == SignupFeedEvent
        count += 1
      else
        if lead.is_a?(feed_event) 
          if lead.event_date > (Time.now - 7.days)
            count += 1
          end
        end
      end
    end
    count
  end

Answer 1

You could use scopes to do this at the database level, which would (potentially) be much faster than returning then iterating over all ActivityFeedEvents.

# ActivityFeedEvent
scope :by_type, ->(type){ where(type: type.to_s) }
scope :recent, ->{ where("event_date > ?", Time.now - 7.days) }

# wherever report_total is
def report_total(feed_event_type, advisor)
  advisor.activity_feed_events.by_type(SignupFeedEvent).count +
    advisor.activity_feed_events.recent.by_type(feed_event_type).count
end

Note that you need a lambda for the recent scope despite it not taking args, because otherwise the Time.now will only be evaluated once (when the server starts).

Or, perhaps even more neatly, you could do it the other way round:

# ActivityFeedEvent
scope :for, ->(advisor){ where(advisor_id: advisor.id) }
scope :recent, ->{ where("event_date > ?", Time.now - 7.days) }

# wherever report_total is
def report_total(feed_event_type, advisor)
  SignupFeedEvent.for(advisor).count + feed_event_type.for(advisor).count
end

Depending on your models you may need a more descriptive name than for for this one, belonging_to or for_advisor are candidates.

Also note that some people prefer to write scopes as class methods instead of using lambdas

# ActivityFeedEvent
def self.for(advisor)
  where(advisor_id: advisor.id)
end

def self.recent
  where("event_date > ?", Time.now - 7.days)
end

Answer 2

Andy's solution with scopes is the one, so just to complement it, a refactor of your method using sum (although what you really need is count, see other answers):

def report_total(feed_event, advisor)
  advisor.activity_feed_events.sum do |lead|
    if lead == SignupFeedEvent ||
       (lead.is_a?(feed_event) && lead.event_date > (Time.now - 7.days))
      1
    else
      0
    end
  end
end