Refactor to avoid cope repetition in loop with condition checking

Question

I have snippet that really look ugly but I don't know how to refactor it:

if owner
 samples.each do |sample| 
 sample_name = get_name(sample) # logical to get correct name
 serialized_sample = SampleSerializer::Level10.new(sample, 10).serializable_hash
 groups[sample_name] = groups[sample_name] == nil 
 ? [].push(serialized_sample)
 : groups[sample_name].push(serialized_sample)
 end
else
 samples.each do |sample| 
 sample_name = get_name(sample) # logical to get correct name
 serialized_sample = ElementPermissionProxy.new(current_user, sample, user_ids).serialized
 groups[sample_name] = groups[sample_name] == nil 
 ? [].push(serialized_sample)
 : groups[sample_name].push(serialized_sample)
 end
end

As you can see the code block inside if .. else is mostly the same except I need to use 2 difference class on each case (SampleSerializer and ElementPermissionProxy).

I can change into:

samples.each do |sample| 
 sample_name = get_name(sample) # logical to get correct name
 serialized_sample = owner 
 ? SampleSerializer::Level10.new(sample, 10).serializable_hash
 : ElementPermissionProxy.new(current_user, sample, user_ids).serialized
 groups[sample_name] = groups[sample_name] == nil 
 ? [].push(serialized_sample)
 : groups[sample_name].push(serialized_sample)
end

However, the samples contains a few thousands of element, so that I have check the condition with every sample element. And I don't like this way of optimization.

In addition, I would really appreciate if someone help me with this block also:

 groups[sample_name] = groups[sample_name] == nil 
 ? [].push(serialized_sample)
 : groups[sample_name].push(serialized_sample)

• Jamal: As we all want to make our code more efficient or improve it in one way or another, try to write a title that summarizes what your code does, not what you want to get out of a review.

Answer 1

Have you considered using a Proc or a Lambda?

You could do the check once, before the loop, to decide the function to call to serialize a sample.

In pseudo Ruby, you need to work out the number of the arguments, I think — I'm not a Ruby programmer.

serializer = owner
 ? lambda { return SampleSerializer::Level10.new(sample, 10).serializable_hash }
 : lambda { return ElementPermissionProxy.new(current_user, sample, user_ids).serialized }
samples.each do |sample|
 sample_name = get_name(sample) # logical to get correct name
 serialized_sample = serializer.call(sample, current_user, user_ids)
 groups[sample_name] = groups[sample_name] == nil 
 ? [].push(serialized_sample)
 : groups[sample_name].push(serialized_sample)
end

Answer 2

Thanks for SQB's answer. I've refactored my code using lambda (other variables within lambda such as: current_user and user_ids belong to current scope).

The nil checking of groups[sample_name] array is also be changed using || operator like below. The code is now much more better now:

sample_serializer_selector =
 if own_collection
 lambda { |s| SampleSerializer::Level10.new(s, 10).serializable_hash }
 else
 lambda { |s| ElementPermissionProxy.new(current_user, s, user_ids).serialized }
 end
samples.each do |sample|
 sample_name = get_name(sample)
 serialized_sample = sample_serializer_selector.call(sample)
 groups[sample_name] = (groups[sample_name] || []).push(serialized_sample)
end