Coding style and undoing indentation hell

I've seen a lot of heavy nesting caused by the belief that functions should only have one return statement. Ex:

    // Original
    function render() {
      $return = null;
      if($condition1) {
        if($condition2) {
          $return = 2;
        } else {
          $return = 1;
        }
      } elseif($condition3) {
        $return = 3;
      } else {
        $return = 4;
      }
      return $return;
    }

But I'd rather read:

    // Refactored
    function render() {
      if($condition1 && $condition2) return 2;
      if($condition1) return 1;
      if($condition3) return 3;
      return = 4;
    }

It's probably a matter of style at some level.

I hate this style (from the article):

  if (...) {
        if (...) {
            ...
    } else {
        if (...) {
            ...
        } else if (...) {
            ...
        } else {
            ...
        }
    }

and prefer this:

  if (...) 
  {
      if (...) 
      {
        ...
      } 
      else 
      {
          if (...) 
          {
            ...
          } 
          else if (...) 
          {
            ..
          } 
          else 
          {
            ...
          }
      }
  }

The first example is confusing and hard to read. The second is clean and easy to read.

As to the question of optimization, that's a different matter. Lots of tricks can be used to compress code into fewer statements on a case by case basis:

  if(pushbutton == 0)
  {
    led = 12;
  }
  else
  {
    led = 24;
  }

can become

  led = (pushbutton == 0) ? 12 : 24;

or

  led = (pushbutton * 12) + 12;

or

  led = 12 * (pushbutton + 1);

etc.

Don't forget that in many cases, you can replace a conditional with polymorphism. http://c2.com/cgi/wiki?ReplaceConditionalWithPolymorphism

This -

  if (!test()) {
    handle_failure();
  } else {
    // do lots of work
  }

is just another incarnation of

  if (NULL == v)

i.e. it sacrifices a natural flow of code and makes it harder to follow for something of a superficial value.

Also this -

  if (!test1()) {
      handle_failure_1();
      return false;
  }

is fine unless the function needs to do a cleanup, which is pretty much always the case, and which in turn means that the { } block with grow with every if and it will include a lot of redundant code.

A far cleaner and easier to follow approach is to use if-goto, whereby all cleanup code sits at the bottom of the function (where it naturally belongs) and the flow jumps to the appropriate part of the cleanup on failure. The only exception is when there's no cleanup needed, in which case it can do if-return.

The broad suggestion that indentation is somehow wrong is ill-advised.

Sure, various of the refactorings given are sensible. But the basic problem is not indentation. If something can be simplified, simplify it. If something is too long, break it into pieces. But if something is tree-structured, that is what it is, and it probably ought not be bent out of shape.

(Note: Fowler's refactoring 'Replace Nested Conditional with Guard Clauses' is very specific -- it does not declare that all nesting is bad.)

A tree-like-indentation structure is a perfectly good element of programming -- why try/want to change it into something else?

In fact, a good case could be made that software in its most fundamental essential conception is tree-structured -- even that engineering generally is tree-structured.

At this point in my coding life (15 years in), I'm pretty strongly against if / else clauses. The readability of such code just plummets in my view.

I try to limit my conditionals to inline statements:

  $x = $y if $b;

Often joined with early returns:

  return $b if $a;

If I find myself coding an if/else branch, that's often a clue that I should refactor this chunk into a function that can use early returns to avoid nested conditionals:

so

  if ($a == FOO) {
    $c = $d;
    }
  else if ($a == BAR) {
    $c = $e; 
    }
  else {
    $c = $f;
  }

becomes

  $c = get_val($a);

	...

  sub get_val {
   my ($a) = @_;
   return $d if $a == FOO;
   return $e if $a == BAR;
   return $f;
   }

Thankfully much of my coding lately is in perl, which lets me use postfix "if/unless" as well as "and/or" to avoid having to use braced conditionals.

I also love myself the ternary operator. Basically my take comes down to "use conditionals for one-line assignments only, NOT for block execution".

I'm glad I am not the only that is particular about these things. Computers may not care which order your IF statement is in... but some people completely disregard that their code is going to be read by humans too.

Generally good advice, but w/r/t the first recommendation, I prefer to structure if/else clauses so that common case is handled first and the exceptional case is handled last.

meh. when i'm looking for a bug, i tend to start in the places with lots of branching. in general, high quality code doesn't have much branching, and if you find yourself neding complicated and nested branching, there is probably a better way to factor your code.

one easy trick is to structure your code such that 'null' is impossible, by initializing to an empty value, or re-ordering the way things happen, or using exceptions or error-monad.

I have to run so this is really a substandard comment for hacker news, but... Really? He advocates adding multiple exit points to a function as a way of REDUCING COMPLEXITY? The solution when you have too deep of nesting is to pull out inner parts into useful simpler stateless functions, or so I was led to believe!

He's preaching to the choir in my case. I'm a big fan of busting out of functions/methods early when you already know "it's over". (sometimes this means putting the cleanup in something like a try/finally wrapper routine, though)

But, kudos: I can send this to my team instead of having to write it myself. Win!

The goto cleanup strategy can be improved upon by using multiple goto labels, like here: http://vilimpoc.org/research/raii-in-c/

If you do your indentation "wrong" and nothing breaks, then you will do your indentation "wrong". That's why I find whitespace-sensitive languages easier to read, YMMV.

He's basically a step away from having several objects (let's call them handlers) which can do your handling for you. Further, they can define the set of circumstances in which they are valid.

I'm going to work with a couple of assumptions:

1) The test casts which he refactors to test*() methods are non-trivial 2) The handlers _may_ be large sets of code

Here's a way to handle one of this refactored methods:

if (!test1()) { handle_failure_1(); return false; }

if (!test2()) { handle_failure_2(); return false; }

if (!test3()) { handle_failure_3(); return false; }

handle_success(); return true;

That's a lot of potential code we're dealing with in this class.

What if:

class HandlesFailure1

  def handles_this_case(params)
     #code from test1()
  end

  def handle(params)
    #code from handle_failure_1()
    return false;
  end

end

class HandlesFailure2

  def handles_this_case(params)
     #code from test2()
  end

  def handle(params)
    #code from handle_failure_2()
    return false;
  end

end

class HandlesFailure3

  def handles_this_case(params)
     #code from test3()
  end

  def handle(params)
    #code from handle_failure_3()
    return false;
  end

end

class HandlesSuccess

  def handles_this_case(params)
    return true
  end

  def handles(params)
    //code from handle_success
    return true
  end

end

#And now the original class

class DoesSomething

  #There are ways to set this up fairly easily. One simple way it to just new it up here, but you could auto-wire it too
  handlers = [HandlesFailure1, HandlesFailure2, HandlesFailure3, HandlesSuccess]

  def do_something(params)

    handlers.each do |handler|
      return handler.handle(params) if handler.handles_this_case(params)
    end

  end

end

There. Now there's a loop and an if. Check each case and either handle or toss it out. This can also work for the case where you might want multiple handlers to handle something, just don't return on the first one.

I'm not saying it's the right way, but it's another way.

Pros: It really helps to group together the handling methods with the test methods. Very simple dispatch method

Cons: Separate classes, and harder to view all the handlers together.

I agree with most of what was said, especially the use of goto! I don't like success boolean, but that's probably just me.

I stopped reading when he recommended if-goto.

Someone would sure enjoy golang!