HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyrailsMinor

Counting subscribers with matching ZIP code, interests, and sex

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
countingwithandsexzipinterestscodesubscribersmatching

Problem

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  ["name = ?", mt]).each do |mt2|
        mt2.users.each do |u|
          array_of_users  ["sex = ?", ms]).each do |cs|
        array_of_users << cs.id
      end
    end
  end

  total_number = array_of_users.compact.uniq

  return total_number
end

Solution

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.

Context

StackExchange Code Review Q#1611, answer score: 6

Revisions (0)

No revisions yet.