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 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
share|improve this question
    
It would help if you explained a bit about self.distributions and CardSignup etc. contain. –  philosodad Apr 1 '11 at 19:38
    
Distributions are a selection of characteristics for an email to go to.. Examples include age, interests, sex, etc. And CardSignups are the subscription, so when you sign up you fill out your age, interests, sex. etc. So I'm basically trying to find all the CardSignup attributes that match the Distribution attributes. –  Trip Apr 1 '11 at 19:43

3 Answers 3

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.

share|improve this answer
    
Thanks philosodad ;) That's a tough call because I need to compare to arrays to see which one matches, but select will just pull the objects and not the array of attributes. –  Trip Apr 1 '11 at 19:48
    
So you want all the :zipcode from distribution such that :zipcode is not nil? Why? For the purposes of comparison with your other array, the nil values will be dropped anyway... unless you allow nil values in your CardSignup array as well. –  philosodad Apr 1 '11 at 21:35

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

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.

share|improve this answer

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.