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

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

end

-
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

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.

-
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

total_number = array_of_users.compact.uniq

end


You could remove the total_number variable and instead write:

def count_of_distribution

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