# Simple: Need help refactoring this awkward looking method

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

  def report_total(feed_event, advisor)
count = 0
count += 1
else
if lead.event_date > (Time.now - 7.days)
count += 1
end
end
end
end
count
end

-
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. –  Andy H Dec 12 '12 at 17:14
Yes, it's a Rails app. –  Jon Smooth Dec 12 '12 at 17:22
ActivityFeedEvent is the parent class and all the others (ex: SignupFeedEvent) inherit from this parent. –  Jon Smooth Dec 12 '12 at 17:23
And are they all in the same table in your database (with a type column)? –  Andy H Dec 12 '12 at 17:27
@andy h Yup, thats correct! –  Jon Smooth Dec 12 '12 at 17:29

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
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 :recent, ->{ where("event_date > ?", Time.now - 7.days) }

# wherever report_total is
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
end

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

-

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)

You could pull the boolean logic into a predicate method that takes lead as an argument. –  Eric Walker Dec 15 '12 at 21:59
yeah, something like lead.reportable?. But I wanted to focus on the refactoring of the same OP code. –  tokland Dec 15 '12 at 22:55