2
\$\begingroup\$

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
\$\endgroup\$
5
  • \$\begingroup\$ Is this in a Rails app? Are the different types of FeedEvent in a single table (i.e. are you using STI)? A little context would be helpful. \$\endgroup\$ Commented Dec 12, 2012 at 17:14
  • \$\begingroup\$ Yes, it's a Rails app. \$\endgroup\$
    – Jon Smooth
    Commented Dec 12, 2012 at 17:22
  • \$\begingroup\$ ActivityFeedEvent is the parent class and all the others (ex: SignupFeedEvent) inherit from this parent. \$\endgroup\$
    – Jon Smooth
    Commented Dec 12, 2012 at 17:23
  • \$\begingroup\$ And are they all in the same table in your database (with a type column)? \$\endgroup\$ Commented Dec 12, 2012 at 17:27
  • \$\begingroup\$ @andy h Yup, thats correct! \$\endgroup\$
    – Jon Smooth
    Commented Dec 12, 2012 at 17:29

2 Answers 2

3
\$\begingroup\$

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
\$\endgroup\$
2
\$\begingroup\$

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
\$\endgroup\$
2
  • \$\begingroup\$ You could pull the boolean logic into a predicate method that takes lead as an argument. \$\endgroup\$ Commented Dec 15, 2012 at 21:59
  • \$\begingroup\$ yeah, something like lead.reportable?. But I wanted to focus on the refactoring of the same OP code. \$\endgroup\$
    – tokland
    Commented Dec 15, 2012 at 22:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.