Should multiple array/hash lookups be stored in a variable

Question

I've been using Reek lately to refactor my code and one of the smells, DuplicateMethodCall, is being called on array and hash lookups, such as array[1] or hash[:key] when called multiple times.

So I was wondering if multiple array or hash lookups are so expensive, that we should be storing them in a variable rather than calling them directly, which is what everyone does from my experience.

I would not hesitate to store multiple object method calls (especially if it's a DB call) in a variable, but doing that for array and hash lookups feel like an overkill.

For example, I'll get a warning for this piece of code:

  def sort_params
    return [] if params[:reference_letter_section].nil?

    params[:reference_letter_section].map.with_index(1) do |id, index|
      { id: id, position: index }
    end
  end

but I feel like storing params[:reference_letter_section] in its own variable is too much

Code quality tools should always be taken with a grain of salt. When performance is not the issue, readability is important. But use your judgement to decide which way it's better. The tools just show you where to look at. — ndnenkov, Jul 05 '17 at 12:54
Yeah totally agree with it @ndn, but I didn't know how much it'll improve performance, that's why I am asking this here — Maxim Fedotov, Jul 05 '17 at 12:58
"I didn't know how much it'll improve performance" - you can always _measure_ that. — Sergio Tulentsev, Jul 05 '17 at 13:02

score 2 · Accepted Answer · answered Jul 05 '17 at 12:03

2

So I was wondering if multiple array or hash lookups are so expensive

Expensive calls are not the only reason for not doing the call multiple times. It also clutters the code without real need. Consider this not-so-contrived example:

Order.new(
  name:       params[:order][:name],
  total:      params[:order][:total],
  line_items: [
    {
      product_name: params[:order][:line_item][:product],
      price:        params[:order][:line_item][:price],
    }
  ]
)

Even though those hash accesses are super-cheap, it still makes sense to extract them, for readability reasons.

order_params     = params[:order]
line_item_params = order_params[:line_item]

Order.new(
  name:       order_params[:name],
  total:      order_params[:total],
  line_items: [
    {
      product_name: line_item_params[:product],
      price:        line_item_params[:price],
    }
  ]
)

answered Jul 05 '17 at 12:03

Sergio Tulentsev

226,338
43
373
367

Of course, I didn't imply it's the only reason. But when readability doesn't suffer, but rather just increases the line count, I was wondering if it's still worth it – Maxim Fedotov Jul 05 '17 at 13:00
@MaximFedotov: well, that's a judgement call. One shouldn't blindly trust the tools. They don't know what makes good code. :) – Sergio Tulentsev Jul 05 '17 at 13:00
Yup, I understand that, but that's exactly why I posted the question, to see what more experienced developers would consider acceptable. Now that it looks like it doesn't quite affect performance, I'll probably just ignore array/hashes unless it's very frequently called. Will have to think of a best way to update my config – Maxim Fedotov Jul 05 '17 at 13:06
1

@MaximFedotov: ah, I see that you posted an example. In your case, I'd probably still extract that. It'd make the code more DRY, if nothing else. – Sergio Tulentsev Jul 05 '17 at 13:09

score 0 · Answer 2 · answered Jul 06 '17 at 07:46

The duplicate hash lookup represents coupling between those two lines of code. This can increase the time taken to understand the code, and can be a source of friction when changing it. Of course, within a small method like this the cost is relatively low; but if the two lines of code were further apart -- in different classes, for example -- the effects of the coupling would be much more costly.

Here's a version of your method that doesn't have the duplication:

def sort_params
  reference_letters = params[:reference_letter_section] || []
  reference_letters.map.with_index(1) do |id, index|
    { id: id, position: index }
  end
end

Should multiple array/hash lookups be stored in a variable

2 Answers2