Best way to refactor this without making as many calls as I am?

Question

I am trying to cycle through a few of these blocks. They basically narrow down a number of people that fulfill a bunch of attributes.

I apologize if this seems really messy, but my database is really taking a toll processing this, and I know there's a better way. I'm just lost on strategy right now.

def count_of_distribution    
  #beginning with an array..
  array_of_users = []

  # any matching zip codes? ..
  # zip_codes
  @zip_codes = self.distributions.map(&:zip_code).compact
  unless @zip_codes.nil? || @zip_codes.empty? 
    @matched_zips = CardSignup.all.map(&:zip_code) & @zip_codes
    @matched_zips.each do |mz| 
      CardSignup.find(:all, :conditions => ["zip_code = ?", mz]).each do |cs|
        array_of_users << cs.id
      end
    end
  end

  # any matching interests?..
  # interest
  @topics = self.distributions.map(&:me_topic).compact
  unless  @topics.nil? || @topics.empty?
    @matched_topics = MeTopic.all.map(&:name) & @topics
    @matched_topics.each do |mt|
      MeTopic.find(:all, :conditions => ["name = ?", mt]).each do |mt2|
        mt2.users.each do |u|
          array_of_users << u.card_signup.id if u.card_signup
        end
      end
    end
  end

  # any matching sexes?..
  # sex
  @sexes = self.distributions.map(&:sex).compact
  unless @sexes.nil? || @sexes.empty?
    @matched_sexes = CardSignup.all.map(&:sex) & @sexes
    @matched_sexes.each do |ms|
      CardSignup.find(:all, :conditions => ["sex = ?", ms]).each do |cs|
        array_of_users << cs.id
      end
    end
  end

  total_number = array_of_users.compact.uniq

  return total_number
end

Answer 1

The first thing I notice is that you're calling map.{}.compact when you probably could be calling select{}, that is, you want to select from self.distribution. So that would replace 6 method calls with 3.

If you use select, it always returns an array and never a nil, so instead of unless X.nil or X.empty, you could use if X.any?, which would replace 6 method calls with three.

If you aren't using @matched_zips outside of this context, it makes no sense to assign a variable here. Instead of assigning, you could just use (CardSignup.all.map(&:zip_code) & @zip_codes).each do, which would spare you three variable assignments.

Answer 2

Another thing that can be fixed is your return call. So instead of

def count_of_distribution 
    # ... your code ...

    total_number = array_of_users.compact.uniq

    return total_number
end

You could remove the total_number variable and instead write:

def count_of_distribution 
    # ... your code ...

    array_of_users.compact.uniq
end

Answer 3

In addition to the things already mentioned I noticed was that you're setting a lot of instance variables. Obviously I don't know how and where you're going to use them, but my first instinct is that this is a sign of bad design.

Generally a method which is called primarily for its return value, should avoid side effects (like setting instance variables) when possible.

Another point: You might consider using a set instead of an array. This way you don't need to call uniq on it.