How can I idiomatically write a doubly nested if/else in Ruby?-CodePudding

Suppose I have some code like:

def set_reminder(cond_one: false, cond_two: false)
  if cond_two
    if cond_one
      outcome_a
    else
      outcome_b
    end
  else
    if cond_one
      outcome_c
    else
      outcome_d
    end
  end
end

How can I more elegantly write a function like this, which has 4 potential results (one for each combination of possible cond_one and cond_two values)?

I'm not satisfied with this version, using an if/else statement with another if/else in both branches. In the actual code, the outcomes are already complex expressions, so writing something like return outcome_a if cond_one && cond_two (for all 4 outcomes) would be unwieldy.

CodePudding user response：

Ruby has a very powerful case expression that can be used for this sort of thing. Consider

def set_reminder(cond_one: false, cond_two: false)
  case [cond_one, cond_two]
  when [true, true] then outcome_a
  when [true, false] then outcome_b
  when [false, true] then outcome_c
  when [false, false] then outcome_d
  end
end

As pointed out in the comments, though, consider having your arguments convey more than just "pair of Booleans". See Boolean blindness for a good discussion on this.

CodePudding user response：

You could go with something like below, flattening your nested ifs into a series of guard statements.

def set_reminder(cond_one: false, cond_two: false)
  return outcome_a if cond_two && cond_one
  return outcome_b if cond_two
  return outcome_c if cond_one

  outcome_d
end

This is neater and allows for further refactoring.

I can't recommend enough Sandi Metz et al's book on refactoring, 99 Bottles of OOP. There's a Ruby version. The entire book walks you through this kind of refactoring. From multiple ifs to extracted classes.

CodePudding user response：

You could set it up in your constructor as a hash:

@choice = {[true, true] => :a, [true, false] => :c,
           [false, true] => :b, [false, false] => :d}

and then your method would be:

def set_reminder(cond_one: false, cond_two: false)
  @choice[[cond_one, cond_two]]
end

An alternative which avoids indexing by compound objects is to create a hash of hashes:

@h_of_h = {true => {true => :a, false => :c},
              false => {true => :b, false => :d}}

def set_reminder_h2(cond_one: false, cond_two: false)
  @h_of_h[cond_one][cond_two]
end

I did the following benchmark to compare the various proposed approaches:

require 'benchmark/ips'

combos = [[true, true], [true, false], [false, true], [false, false]]

def set_reminder_case(cond_one: false, cond_two: false)
  case [cond_one, cond_two]
  when [true, true] then :a
  when [true, false] then :b
  when [false, true] then :c
  when [false, false] then :d
  end
end

def set_reminder_guard(cond_one: false, cond_two: false)
  return :a if cond_two && cond_one
  return :b if cond_two
  return :c if cond_one
  :d
end

@choice = {[true, true] => :a, [true, false] => :c,
           [false, true] => :b, [false, false] => :d}

def set_reminder_hash(cond_one: false, cond_two: false)
  @choice[[cond_one, cond_two]]
end

@h_of_h = {true => {true => :a, false => :c},
              false => {true => :b, false => :d}}

def set_reminder_h2(cond_one: false, cond_two: false)
  @h_of_h[cond_one][cond_two]
end

N = 1_000
SEED_VALUE = 123_456_987

# The choicess of true/false combos are being randomized, but since the
# seed is reset they are identical for the two functions being tested.
Benchmark.ips do |b|
  srand(SEED_VALUE)
  b.report('case') do
    N.times do
      v1, v2 = combos[rand(4)]
      set_reminder_case(cond_one: v1, cond_two: v2)
    end
  end
  srand(SEED_VALUE)
  b.report('hash') do
    N.times do
      v1, v2 = combos[rand(4)]
      set_reminder_hash(cond_one: v1, cond_two: v2)
    end
  end
  srand(SEED_VALUE)
  b.report('guard') do
    N.times do
      v1, v2 = combos[rand(4)]
      set_reminder_guard(cond_one: v1, cond_two: v2)
    end
  end
  srand(SEED_VALUE)
  b.report('hsh_of_hsh') do
    N.times do
      v1, v2 = combos[rand(4)]
      set_reminder_h2(cond_one: v1, cond_two: v2)
    end
  end
  b.compare!
end

The results generated using Ruby 3.2.0 on an M1 MacBook Pro are:

% ruby case_v_hash.rb 
Warming up --------------------------------------
                case   197.000  i/100ms
                hash   239.000  i/100ms
               guard   596.000  i/100ms
          hsh_of_hsh   562.000  i/100ms
Calculating -------------------------------------
                case      1.977k (± 1.2%) i/s -     10.047k in   5.083713s
                hash      2.408k (± 0.5%) i/s -     12.189k in   5.062504s
               guard      5.952k (± 0.6%) i/s -     29.800k in   5.006765s
          hsh_of_hsh      5.637k (± 1.3%) i/s -     28.662k in   5.085419s

Comparison:
               guard:     5952.2 i/s
          hsh_of_hsh:     5637.2 i/s - 1.06x  (± 0.00) slower
                hash:     2407.8 i/s - 2.47x  (± 0.00) slower
                case:     1976.6 i/s - 3.01x  (± 0.00) slower

With --yjit:

% ruby --yjit case_v_hash.rb
Warming up --------------------------------------
                case   243.000  i/100ms
                hash   290.000  i/100ms
               guard     1.075k i/100ms
          hsh_of_hsh   952.000  i/100ms
Calculating -------------------------------------
                case      2.419k (± 0.7%) i/s -     12.150k in   5.022058s
                hash      2.921k (± 0.8%) i/s -     14.790k in   5.062949s
               guard     10.715k (± 1.4%) i/s -     53.750k in   5.017634s
          hsh_of_hsh      9.430k (± 0.7%) i/s -     47.600k in   5.048054s

Comparison:
               guard:    10714.6 i/s
          hsh_of_hsh:     9429.9 i/s - 1.14x  (± 0.00) slower
                hash:     2921.4 i/s - 3.67x  (± 0.00) slower
                case:     2419.4 i/s - 4.43x  (± 0.00) slower

It sure looks to me like user3574603's guard statement approach is the winner, with the hash of hashes a reasonably close second. Both dominate the hash lookup and case statement approaches.

CodePudding user response：

I would say that you cannot improve on what you have now. Your method has the following characteristics:

it is easy to comprehend by anyone reading your code (including yourself when you return to it in future);
testing is straightforward;
the variables cond_two and cond_one are each examined just once; and
it easily accommodates future changes in logic.

The number of lines of code in your method could be reduced but I advise against doing that if it would adversely affect readability or ease of testing.

The following are more or less equivalent ways of writing your method. I would say that choosing among these is purely a stylistic choice.

Use ternaries for the inner conditionals

def set_reminder(cond_one: false, cond_two: false)
  if cond_two
    cond_one ? outcome_a : outcome_b
  else
    cond_one ? outcome_c : outcome_d
  end
end

Use case statements

def set_reminder(cond_one: false, cond_two: false)
  case cond_two
    case cond_one
    when true then outcome_a
    else           outcome_b
    end
  else
    case cond_one
    when true then outcome_c
    else           outcome_d
    end
  end
end

Replace inner conditionals with method calls

def set_reminder(cond_one: false, cond_two: false)
  if cond_two
    set_reminder_cond_two_true(cond_one)
  else
    set_reminder_cond_two_false(cond_one)
  end
end

def set_reminder_cond_two_true(cond_one)
  cond_one ? outcome_a : outcome_b
end

def set_reminder_cond_two_false(cond_one)
  cond_one ? outcome_c : outcome_d
end