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
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:
Even though those hash accesses are super-cheap, it still makes sense to extract them, for readability reasons.