Ruby if statement optimization refactor best practice

Question

I am having a very common refactor situation here with me, and after going through a few blogs I still didn't get any satisfactory comment on the same; so asking a question here.

h = {
  a: 'a',
  b: 'b'
}
new_hash = {}
new_hash[:a] = h[:a].upcase if h[:a].present?

According to my friend, this code can be refactored in following way to improve performance.

a = h[:a]
new_hash[:a] = a.upcase if a.present?

At first glance it looks a little optimized. But is it something that'll make a lot of difference or its an over-optimization? And which style should be preferred?

Looking for an expert advice :)

UPDATE with Benchmark n = 1000

              user     system      total        real
hash lookup  0.000000   0.000000   0.000000 (  0.000014)
new var      0.000000   0.000000   0.000000 (  0.000005)
AND op       0.000000   0.000000   0.000000 (  0.000018)
try          0.000000   0.000000   0.000000 (  0.000046)

UPDATE with Memory Benchmark using gem benchmark-memory

Calculating -------------------------------------
         hash lookup    40.000  memsize (    40.000  retained)
                         1.000  objects (     1.000  retained)
                         1.000  strings (     1.000  retained)
             new var     0.000  memsize (     0.000  retained)
                         0.000  objects (     0.000  retained)
                         0.000  strings (     0.000  retained)
              AND op    40.000  memsize (    40.000  retained)
                         1.000  objects (     1.000  retained)
                         1.000  strings (     1.000  retained)
                 try   200.000  memsize (    40.000  retained)
                         5.000  objects (     1.000  retained)
                         1.000  strings (     1.000  retained)

depends on what you intend to do. some more context will definitely help. if you are ok with _mutating_ `h`, you can just do `h[:a] && h[:a].upcase!` or `h[:a].try(:upcase!)` — kiddorails, Jul 03 '18 at 13:22
to answer your question on perf and optimization with second approach, it's better you do some benchmarking around it. You are compensating for hash-lookup with storing in a variable. — kiddorails, Jul 03 '18 at 13:26
@kiddorails `upcase!` OR `upcase` both will do the same as I am anyways using another hash to store its value. Also trying to benchmark with the `try` method. — Swaps, Jul 03 '18 at 13:29
They will not do the same thing as `upcase!` may alter the value in `h` as well as it may return `nil`. e.g. `'A'.upcase! #=> nil` @kiddorails' point was `new_hash` is not needed if altering `h[:a]`, and by association `h`, directly is an acceptable solution. This way would technically be more performant albeit infinitesimally so . — engineersmnky, Jul 03 '18 at 13:49
I have updated the question with benchmark for 1000 count. It seems assigning new variable is way too faster. — Swaps, Jul 03 '18 at 14:01
@engineersmnky oh, indeed. What language is it in the first place? — Aleksei Matiushkin, Jul 03 '18 at 14:23
share the benchmark code please, the size of your sample hash will matter greatly — Anthony, Jul 03 '18 at 14:27
@mudasobwa since he is using `present?`, I presumed he was running this in RoR which also has `try`. but I get your point. — kiddorails, Jul 03 '18 at 14:30
to run benchmarks I included `active_support/core_ext/object/try` & replaced `present?` with `!nil?`. — Swaps, Jul 04 '18 at 05:38

score 1 · Answer 1 · answered Jul 03 '18 at 13:27

Optimization wears different shoes, there's memory optimization, performance optimization, and there's the readability and how the code is structured.

Performance: There's nearly no effect whatsoever on the speed and performance because the hash is being accessed in O(1). Try using benchmark to see yourself that there's nearly no difference

You can check this article about hash lookup and why it's so fast

Memory: Your friend's Code is less optimized than yours because he initialized another object a while yours doesn't.

Readability and Style: At the first glance your friend's code looks like fewer lines and more descriptive. But keep in mind that you may need to do this for every key/value in the hash, so you may need to have a, b, and it goes on as your hash goes on (When it goes like that it's better to iterate over the hash of course). Not too much to look on here tbh

I have updated the question with benchmark for 1000 count. It seems assigning new variable is way too faster. In memory benchmarking as well assigning new variable seems to be consuming less memory. — Swaps, Jul 03 '18 at 14:06
Thanks for providing benchmarks! not sure if the difference between `0.000014` and `0.000005` can be put as "way too faster". time is "nearly" the same in both cases, so the effect on speed can easily be neglected — amrdruid, Jul 03 '18 at 14:13

engineersmnky · Accepted Answer · 2018-07-03T15:15:56.027

Depending on your circumstances rails methods like present? can be dirty and definitely impact performance. If you are only concerned about a nil check and not things like empty Array or blank String then using pure ruby methods will be "much" faster (the quotes are to emphasize the fact that performance is completely inconsequential in this basic example)

Since we are benchmarking things.

Setup

h = {
  a: 'a',
  b: 'b'
}


class Object
  def present? 
    !blank?
  end
  def blank?
    respond_to?(:empty?) ? !!empty? : !self
  end
end

def hash_lookup(h)
  new_hash = {}
  new_hash[:a] = h[:a].upcase if h[:a].present?
  new_hash
end

def new_var(h)
  new_hash = {}
  a = h[:a]
  new_hash[:a] = a.upcase if a.present?
  new_hash
end

def hash_lookup_w_safe_nav(h)
  new_hash = {}
  new_hash[:a] = h[:a]&.upcase
  new_hash
end

def hash_lookup_wo_rails(h)
  new_hash = {}
  new_hash[:a] = h[:a].upcase if h[:a]
  new_hash
end

def new_var_wo_rails(h)
  new_hash = {}
  a = h[:a]
  new_hash[:a] = a.upcase if a
  new_hash
end

Benchmarks

N = [1_000,10_000,100_000]
require 'benchmark'
N.each do |n|
  puts "OVER #{n} ITERATIONS"
  Benchmark.bm do |x|
    x.report(:new_var) { n.times {new_var(h)}}
    x.report(:hash_lookup) { n.times {hash_lookup(h)}}
    x.report(:hash_lookup_w_safe_nav) { n.times {hash_lookup_w_safe_nav(h)}}
    x.report(:hash_lookup_wo_rails) { n.times {hash_lookup_wo_rails(h)}}
    x.report(:new_var_wo_rails) { n.times {new_var_wo_rails(h)}}
  end
end

Output

OVER 1000 ITERATIONS
                        user     system      total        real
new_var                 0.001075   0.000159   0.001234 (  0.001231)
hash_lookup             0.002441   0.000000   0.002441 (  0.002505)
hash_lookup_w_safe_nav  0.001077   0.000000   0.001077 (  0.001077)
hash_lookup_wo_rails    0.001100   0.000000   0.001100 (  0.001145)
new_var_wo_rails        0.001015   0.000000   0.001015 (  0.001016)
OVER 10000 ITERATIONS
                        user     system      total        real
new_var                 0.010321   0.000000   0.010321 (  0.010329)
hash_lookup             0.010104   0.000015   0.010119 (  0.010123)
hash_lookup_w_safe_nav  0.007211   0.000000   0.007211 (  0.007213)
hash_lookup_wo_rails    0.007508   0.000000   0.007508 (  0.017302)
new_var_wo_rails        0.008186   0.000026   0.008212 (  0.016679)
OVER 100000 ITERATIONS
                        user     system      total        real
new_var                 0.099400   0.000249   0.099649 (  0.192481)
hash_lookup             0.101419   0.000009   0.101428 (  0.199788)
hash_lookup_w_safe_nav  0.078156   0.000010   0.078166 (  0.140796)
hash_lookup_wo_rails    0.078743   0.000000   0.078743 (  0.166815)
new_var_wo_rails        0.073271   0.000000   0.073271 (  0.125869)

Thanks for more detailed benchmarks. It looks like `new_var_wo_rails` is better and looking more descriptive in code as well. I will just remove the `present` check. — Swaps, Jul 04 '18 at 05:45

Ruby if statement optimization refactor best practice

2 Answers2