About the code samples for "Avoid use of nested conditionals for flow of control"

[gnapse]

Regarding the rules about using nested conditionals for flow of control found here, I wanted to raise attention to these two particular fragments of the bad vs good code samples:

# (supposedly) bad
if thing[:foo][:bar]
  partial_compute(thing)
else
  re_compute(thing)
end

# (supposedly) good
return re_compute(thing) unless thing[:foo][:bar]
partial_compute(thing)

I agree with the overall rule, but I would argue that looking at these two fragments as variants on their own, the first one above is more acceptable than the second one. At least I have to think more with the second one to understand what's going on, than with the first one.

Plus, making the change from the first one to the second, when you see the complete code samples in the aforementioned rule, has nothing to do with what the rule states, which is:

A guard clause is a conditional statement at the top of a function that bails out as soon as it can.

This part of the original function in the "bad" sample is not at the top of the function, so it shouldn't require to be changed.

Additionally, in the full code sample, after converting the top-level conditional into a guard statement, this other if/else conditional is no longer nested, so the rule does not apply to it anyway, given the title of the rule is

Avoid use of nested conditionals for flow of control

[jdickey]

Jumping in late as usual here, but here goes...

Guard clauses, in my experience and understanding, are explicit short-circuits at the beginning of a method-like block: "if we get here and a condition exists that negates what this block does, return something without going through the rest of the block".

The examples here and in #215 commingle guard clauses with ordinary branch-tree logic: "do X, and if this condition is true, do Y; else do Z", as shown by @gnapse at the start here.

What we argue for in our shop standard is that the conditional be extracted to a support method with a guard clause:

def whomp(thing)
  # ...
  partial_or_full_recompute(thing)
end

# Ideally, this would instead be named for *why* you choose whether to do a
# full or partial recompute. Is `thing` something that may involve one or more
# resources, and only one resource is of interest in the current use case? Are
# we performing computation differently based on whether `thing` spans a
# time-period boundary or other conceptually-similar delimiter? What, in terms
# of the project's ubiquitous language, does `thing[:foo][:bar]`, and thus
# `#partial_or_full_recompute`, *mean?*
def partial_or_full_recompute(thing)
  return partial_compute(thing) if thing[:foo][:bar]
  re_compute(thing)
end

Our argument for this assumes that #whomp does something (or, preferably, calls methods that do something) before the re-computation based on thing[:foo][:bar]. and therefore the single method with conditionals is an unnecessary SRP violation. (CQRS also pipes in here, if you roll that way.) With the extracted #partial_or_full_recompute method, #whomp should become a pure utility method, having no logic of its own beyond calling other methods in sequence, any of which may themselves require guard clauses.

What do you folks think?

[gnapse]

I still think that in cases like this particular one, there's nothing wrong with the if/else clause with both code branches in it. I don't see why it should be extracted to a helper method just to be able to make the guard variant.

[jdickey]

We (try to) follow the convention that any given method should either "do stuff" or "call stuff", but not mix the two. It encourages more granular, easily-reasoned-about code, and helps us isolate and easily identify code that "knows about" any data attribute/property. As we shuffle our way towards eliminating externally visible state in our classes (which we have found helps reasoning/testing/maintaining), we consider this a significant win. YMMV, of course, but for anything more substantive than personal druthers, I'd welcome that conversation.

[gnapse]

I agree with you, but how does that reasoning makes this:

def some_method(thing)
  if thing[:foo][:bar]
    partial_compute(thing)
  else
    re_compute(thing)
  end
end

less acceptable than this:

def some_method(thing)
  return partial_compute(thing) if thing[:foo][:bar]
  re_compute(thing)
end

[jdickey]

We prefer guard clauses calling support methods rather than if/else branches, even for such utterly simple code, on the argument that what's in the if-match block can then change independently of the else-match block (or vice versa) to the extent that there is zero likelihood of changing one and inadvertently typo-breaking the other.

[gnapse]

Ok, but the if/else example above calls support methods too, both in the if-match block and the else-match block. So you'd still modify the code in the support methods if you want to modify the behavior of either branch of code. You still get the advantage of less likelihood of changing one and innadvertently typo-breaking the other. Don't you?

[jdickey]

@gnapse Now that you put it that way… IMO either should be perfectly acceptable. We're debating the difference between pronouncing to_may_to and to_mah_to, and the rubble is bouncing nicely.