5

Since a ||= 1 is equivalent to a || a = 1, one can state that this is synctactic sugar for this:

if a.nil?
  a = 1
end

Likewise, given that session is a hash-like object, the following:

def increment_session_counter
  session[:counter] ||= 0
  session[:counter] += 1
end

is equivalent to:

def increment_session_counter
  if session[:counter].nil?
    session[:counter] = 0
  end
  session[:counter] += 1
end

Does that mean that the implicit if statement will be executed every time in the original definition of increment_session_counter? Since session[:counter] will mostly likely be nil only the first time (i.e. << 1% of the time), I have the feeling that the following code is better, since the implicit if won't be fired every time:

def increment_session_counter
  session[:counter] += 1
rescue NoMethodError
  session[:counter] = 1
end

Is this code better in such sense?

Having said that, I have no idea how Rescue is implemented in ruby and whether it's actually relevant regarding the tiny little optimization that can bring if so.

SanjiBukai
  • 563
  • 5
  • 19
  • 7
    Raising and rescuing an exception is a quite expensive operation whereas a check for `nil` is very simple and fast. Therefore I would say it is most of the time faster and a better practice to check for `nil`. – spickermann Jun 12 '17 at 05:32
  • Above has answered your qery, but I thought I would point out that your first 'if' example is incorrect as at no point does it assign a value to 'a' – grail Jun 12 '17 at 05:45
  • 1
    Yep.. Typo here.. Fixed.. *It was only `a` inside the if* (BTW, thank you for the edit Sawa) – SanjiBukai Jun 12 '17 at 05:47
  • About the expensiveness of exception handling, I had doubts about that but you confirmed me that the `||=` idiom for that assignment purpose is indeed the way to go. Thanks. – SanjiBukai Jun 12 '17 at 05:51
  • 1
    Hi, I just tried to fire 1 million times this method using both versions... The result is interesting.. With the `||=` operator I got an average of 2300 ms. While with the rescue version I got an average of 1300 ms. This seems to be not a big deal since I ran 1 million times but this count is not so excessive regarding the amount of data we've used to play nowadays.. So I have an idea performance. I still wait for thoughts about the idiom itself. E.g. I know that the error that being rescued should be as focused as possible in order to avoid other errors to be _ignored_. – SanjiBukai Jun 12 '17 at 06:35
  • 3
    I ran some benchmarks on my machine and found out that raising an exception is about 200 times slower than checking for `nil` upfront. That said it depends on the rate of misses what version will be faster. If you expected the number of misses vs the number of successful increments to be lower than ~ 1/200 then `rescue` might be faster. On higher rates, a check for `nil` might be better. On the other hand, given how little the difference is, I would still prefer the Ruby idiom `||=` over an exception, because of readability reasons. – spickermann Jun 12 '17 at 06:56
  • 1
    It's a simple piece of code, not a complicated algo. Don't think (too much) about what's can be faster. **Measure it** and you will **know**. – quetzalcoatl Jun 12 '17 at 07:13
  • @SanjiBukai If performance is that critical to you, and I think it probably isn't, then you should be prepared to put tests in place that will test the relative performance of *each* choice of algorithm that you make. This would let you verify that changes in platform or version have not modified the performance-optimal choice. – David Aldridge Jun 12 '17 at 07:28
  • Yep.. In the meantime I did some more advanced tests.. I confirm the expensiveness of exception handling _when it occurs_ but in fact as you said the conclusion depends upon the expected number of unassigned value vs the successful numbers of existing value. Regarding the readability, yes there is no direct link between the missing variable and the `NoMethodError`. But if the error could be something like this `rescue ValueNotDefined` I think that this will be in turn more readable than `||=`. – SanjiBukai Jun 12 '17 at 08:08
  • 1
    @SanjiBukai Why do you have a `nil` or missing `:counter` value in the first place? – Stefan Jun 12 '17 at 08:12
  • @Stefan I'm building something with trial and error.. So everything is not yet defined.. This counting is something that can be enabled or disabled.. But yes as also as Kathryn said, it could be easier to define everything upfront. – SanjiBukai Jun 12 '17 at 08:17
  • _"I'm building something"_ – don't waste your time on premature optimization, then. – Stefan Jun 12 '17 at 08:22
  • In fact, it's more for learning purpose at this stage.. But indeed I spend enough time for that particular usecase ;-) – SanjiBukai Jun 12 '17 at 08:24
  • `a ||= 1` isn't equivalent to `a || a = 1`, e.g. when `a` is undefined. It's also surely not equivalent to your `if a.nil?` example. `a` could be undefined or `a` could be `false`. [Here](https://stackoverflow.com/a/27628662/6419007)'s the best description I could find on SO. – Eric Duminil Jun 12 '17 at 12:17

3 Answers3

3

Catching the error is a pretty clever idea, but it's also harder to read than using ||=. But even easier would be to set the initial value when you create the hash in the first place:

@session = {:counter => 0}
def increment_session_counter
  @session[:counter] += 1
end

That doesn't work when the key isn't known beforehand:

def increment_user_counter(username)
  @session[username] ||= 0
  @session[username]  += 1
end

However, in that case your assumption that the value of the counter is only nil once is thrown into jeopardy. In fact, since many distributions follow the power law, it's likely 1 will be the most common count.

Therefore, if you know the possible values beforehand, it's best to set them to zero when the program or class is initialized and skip the need for the default value check. And if you don't know all possible values upfront, you will likely find the ||= statement is needed as often as not. There are probably few scenarios where using an exception is the best solution assuming checking for nil is substantially cheaper.

Kathryn
  • 1,557
  • 8
  • 12
  • Yep in the meantime I did some more tests and I found the same results as **spickermann** found. Regarding a forced initialization, I confirm that this is the best way to deal, but in a language like Ruby (or others) where metaprogramming is a very common usage some situation could not be defined in advance. I mean my usecase is a textbook case not so relevant.. – SanjiBukai Jun 12 '17 at 07:59
3

session[:counter] += 1 does three things:

  1. fetch the value (Hash#[])
  2. increment the value (Integer#+)
  3. store the incremented value (Hash#[]=)

That's pretty convenient, but its brevity also makes it inflexible.

Providing a default is much easier if you separate the steps:

def increment_session_counter
  session[:counter] = session.fetch(:counter, 0) + 1
end
Stefan
  • 109,145
  • 14
  • 143
  • 218
  • I had missed that answer.. Even if the intent could be less clear at first glance, i like this syntax. In other less trivial circumstances it could be more useful. Thank you. – SanjiBukai Dec 17 '17 at 19:57
1

I tried some benchmarks with the following code. This is a rails app. Rails v.5.1.1 / Ruby v2.4.1

Originally the purpose is just to count the number of visits in any show actions of some controllers like so:

include SessionCount
...
def show
  ...
  @counter = increment_session_counter
  ...

Then I could display the counter in the relevant views.

So I made a concern with the appropriate code with both version of the default assignment I wanted to test:

module SessionCount
  private

  #Counter version A
  def increment_session_counter_A
    session[:counter] ||= 0
    session[:counter] += 1
  end

  #Counter version B
  def increment_session_counter_B
    session[:counter] += 1
  rescue
    session[:counter] = 1
  end
end

In order to test both versions of default assignment I changed my controllers code as following:

include SessionCount
...
def show
  ...
  t0 = Time.now
  1000000.times do
    session[:counter] = 0; #initialization for normalization purpose
    increment_session_counter_A
  end
  t1 = Time.now
  puts "Elapsed time: #{((end_time - beginning_time)*1000).round} ms"
  @counter = increment_session_counter_A
  ...

Remark: In that code the initialization is here in order to enforce the "happy path" (where the value is not nil). In real scenario this will occurs only the first time for a given user.

Here are the results:
I get an average of 3100 ms with version A (||= operator).
I get an average of 2000 ms with version B (rescue).

But the interesting part begins now..

In the previous code the code is executed following the "happy path" where no exception occurs..
So I changed the initialization I made for normalization purpose as following in order to enforce the "exception path":

1000000.times do
  session[:counter] = nil; #initialization for normalization purpose
  increment_session_counter_A
end

And here is the results:
I get an average of 3500 ms with version A (||= operator).
I get an average of around 60 000 ms with version B (rescue). Yes I tried a couple of times only..

So here I can conclude as spickermann said that exception handling is indeed quite expensive.

But I think that there are many cases where the first time initialization happen very rarely (like a blog where there is no post at the very beginning)..
In such situations there is no reason to test for nil every time and it could be interesting to use the exception handling.

Am I wrong?

I don't want to nitpick about some ms.. Here I just want to know if this idiom makes sense as a design pattern. I see the difference of both of these versions like the difference between while... end and do ... while since the intent is not the same.

SanjiBukai
  • 563
  • 5
  • 19