Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
share|improve this question
    
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

3 Answers 3

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



UPD: Who asked for description:

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

http://en.wikipedia.org/wiki/Boolean_algebra

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) && lead.event_date > (Time.now - 7.days)
                count += 1
            end
        end
    end
    count
end

http://en.wikipedia.org/wiki/Short-circuit_evaluation

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

http://www.ruby-doc.org/core-1.9.3/Array.html#method-i-count

def report_total(feed_event, advisor)
    advisor.activity_feed_events.count do |lead|
        lead == SignupFeedEvent || lead.is_a?(feed_event) && lead.event_date > (Time.now - 7.days)
    end
end
share|improve this answer
1  
It would be better if you gave a brief description of what you changed and why. –  Jeff Vanzella Dec 14 '12 at 1:44
    
+1 of course, count, no need to sum ones. –  tokland Dec 15 '12 at 11:40

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
share|improve this answer

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
share|improve this answer
    
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

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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