-1
\$\begingroup\$

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)
\$\endgroup\$
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – Jamal Aug 19 '16 at 10:35
2
\$\begingroup\$

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
\$\endgroup\$
2
\$\begingroup\$

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
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.