1
\$\begingroup\$

I recently wrote the following code:

def freq_progress
    user_flashcards = input_flashcards.includes(:term)
    terms_unicos = user_flashcards.map(&:term).compact.uniq

    freq_1, freq_2, freq_3, freq_4, freq_5 = 0, 0, 0, 0, 0
    terms_unicos.each do |term|
      case term.ranking
      when nil
        next
      when 1..100
        freq_1 += 1
      when 101..300
        freq_2 += 1
      when 301..1000
        freq_3 += 1
      when 1001..2000
        freq_4 += 1
      when 2001..5000
        freq_5 += 1
      else
        next
      end
    end
    {
        f1: freq_1,
        f2: freq_2,
        f3: freq_3,
        f4: freq_4,
        f5: freq_5
    }
  end

In the code review, our technical manager told me that parallel assignment is considered bad practice.

In the Ruby style guide, it says:

Avoid the use of parallel assignment for defining variables. Parallel assignment is allowed when it is the return of a method call, used with the splat operator, or when used to swap variable assignment. Parallel assignment is less readable than separate assignment.

However, I figure that this would apply mostly to situations where each variable is set to a different value, and that my particular use case (setting all values to 0, the same "boring" value) would potentially be an exception to the readability rule.

In other words, it seems that this would emphasize the "equal" nature of the 5 variables.

This may be a matter of opinion, but since the Ruby style guide recommends against it, I’d love to hear what Rubyists more experienced than I have to say about this.

As well as any opportunities for refactoring!

Thanks, Michael

p.s. this question was originally posted on StackOverflow, but I was told to close the question and move it to Code Review.

\$\endgroup\$
  • \$\begingroup\$ (One of not too many cases where such a "migration" suggestion seems helpful: Questions bound to get opinions are explicitly deprecated on SO and on-topic at CR.) \$\endgroup\$ – greybeard Mar 21 at 4:28
1
\$\begingroup\$

Yes, it is generally considered a bad practice because it is considered confusing. Though as you point out it is fairly unlikely to confuse because everything is being set to zeros and the alternative is five lines of repeated code.

However I find myself asking why you don't use an array. i.e.

freqs = [0, 0, 0, 0, 0] or freqs = Array.new(5, 0)

or else just define define the final hash:

def freq_progress
    user_flashcards = input_flashcards.includes(:term)
    terms_unicos = user_flashcards.map(&:term).compact.uniq

    result = { f1: 0, f2: 0, f3: 0, f4: 0, f5: 0}
    terms_unicos.each do |term|
      case term.ranking
      when 1..100
        result[:f1] += 1
      when 101..300
        result[:f2]+= 1
      when 301..1000
        result[:f3] += 1
      when 1001..2000
        result[:f4] += 1
      when 2001..5000
        result[:f5] += 1
      end
    end

    result
  end
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Good point on the hash. On the normal SO, somebody commented about the array solution as well. How would this be returned to the frontend? Simply as a 5-positioned array? \$\endgroup\$ – michaelsking1993 Mar 21 at 20:07
  • \$\begingroup\$ Understood - the hash is "required" just because that's how the React client receives it in json. Otherwise, an array would be great. Thanks again. \$\endgroup\$ – michaelsking1993 Mar 25 at 0:02

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.