The best solution is the one that's the most readable and the easiest for other people to understand. That might not always be the same way.

By taking out all the complexity, you take away most of the information we'd need to judge a solution.

The way that you write these things tells other programmers a bit about what you were thinking. Code exists for one human to tell another human something. Although the humans might interpret your intent incorrectly (you can argue elsewhere who's dumber: the compiler or the person), here's what I see in your code:

The first style says that the first branch will probably execute more frequently, but either branch is equally expected (more or less). Compare that below with what I see in the last example.

sub sg {
  if (COND) {
    return 1;
  } else {
    return 2;
  }
}
[download]

Your second example says that you're very cautious because you don't trust what you see. You should never reach that die statement, but you think that you might so you put it there to catch bugs. You probably don't care out coverage testing too much when you purposedly add unreachable code.

sub sg {
  if (COND) {
    return 1;
  } else {
    return 2;
  }
  die "should never reach this";
}
[download]

The last one says that normally this subroutine returns a certain value (computed, from a variable, whatever), but in some special cases you want to short-circuit all the processing and return a different value. Most of the time, however, you expect the full subroutine to execute.

sub sg {
  if (COND) {
    return 1;
  }
  return 2;
}
[download]

This might be a case for a postfix if. To quote TheDamian:

Reserve postfix if for flow-of-control statements.
PBP p94

sub sg {
  return 1 if COND;
  return 2;
}
[download]

I'd probably go this path myself (or the third example).

It really all depends on the conditions and the context the sub is used in.

More often than not though, I don't have subroutines that can return several values where "undefined" isn't a possibility. Usually that being the case, a bare return at the end becomes the trap.

-Lee
"To be civilized is to deny one's nature."

Interesting question. I've been struggling with it myself, and wonder how people here feel about the issue.

I tend to try and make one of the branches very short, and use that as a guard (something that leaves the current iteration/loop/subroutine as soon as possible), while the long branch is then considered the main code. Most of the time, this agrees with programming logic. The rare case in which it does not, I always discover that a big part of the longer branch should be factored out anyway, because it has too little to do with the function of the subroutine it's in.

This is your third example, although I'd negate COND and swap the branches if the return 2 part was much shorter than the return 1 part.

I use one of the following three:

sub sga {
    if (COND) {
        1;
    } else {
        2;
    }
}
sub sgb {
    COND and 
        return 1;
    2;
}
sub sgc {
    COND ?
        1 :
        2;
}
[download]

Typically I use the sga version when both 1 and 2 are complicated comands, sgb when 1 is a simple and short expression and 2 is much more complicated. This is not an absolute rule: I sometimes use sgb even with 1 being a complicated command, like

sub sgb_prime {
    COND and do {
        somecommand();
        somemorecommand();
        return 1;
    };
    evenmorecommand();
    verylongsequenceofcommands();
    andsomemore();
    2;
}
[download]

I use sgc only in the simplest cases, when both 1 and 2 are single lines.

I strongly prefer the third option. Most of the subroutines I write have some sort of 'short-circuit' condition such that the majority of the code will not be executed (parameters invalid or incomplete, etc.). I find that when I use the first option, I usually end up duplicating code that could be factored out. I would never use the second example -- by definition, an if/else construct covers all possibilities, COND and !(COND).

Example 3. It reduces nesting.

I often invert the sense of a condition so that the shorter code block is the controlled block and early exit at the end of that block so that the following code needent be indented:

if (isTrue) {
   # long piece of convoluted code
} else {
   # short piece of simple code
}
[download]

becomes:

if (isFalse) {
   # short piece of simple code
   # early exit (return or last)
}

# long piece of convoluted code
[download]

if (COND) {
    return 1;
} elsif (OTHERCOND) {
    return 2;
} else {
    die "cannot happen";
};
[download]

sub sg {
my $result;

  die "COND isn't valid" if (COND eq undef);

  if (COND) {
    $result = 1;
  } else {
    $result = 2;
  };

  die "result isn't valid" if ($result eq undef);

  return $result;
}
[download]

I would do very much the same thing, with a little tweaking:

sub sg {
   my $res;
   
   # I won't assume COND being undefined is invalid

   if (COND) {
      $res = 1;
   }
   else {
      $res = 2;
   }

   die "Undefined result" unless defined $res;
   return $res;
}
[download]

Of course, this I'd only use this if the code were much more complex than simply assigning the result.

<-radiant.matrix->
A collection of thoughts and links from the minds of geeks
The Code that can be seen is not the true Code
I haven't found a problem yet that can't be solved by a well-placed trebuchet

The second form has a clause which you cannot ever reach, so it is utterly useless. It is not "should never reach this" but "cannot ever reach this", so it might lead the unwary to think that you can somehow --by error-- get to this statement which is simply impossible.

The first form makes it abundantly clear that there are two mutually exclusive possibilities which are chosen on the boolean value of the condition.

This is the same with the third form but only because the first branch ends with a return, so one would have to analyse the content of the branch to note that the last part of the sub can only be reached if the condition is false and not because you can fall through the if part of the sub, which is what one would normally expect. One could say there is some action at a distance which prevents you from normally reaching the last part of the sub.

It is not “should never reach this” but “cannot ever reach this”,

It depends. If someone modifies the code and does not put the requisite return in the right place, then you will get to that die statement.

That said, I’m not a fan either. Having that guard there is only useful If the branches are sp long that there’s a real chance that the screw-up it is supposed to protect against may happen. However, I’d prefer never to let the branches get that long to begin with.

Makeshifts last the longest.

Well you could also write

sub sg {
  (COND) && return 1;
  return 2;
}
[download]

But for clarity I would definitely prefer your first suggestion (especially if, as you say, things are a bit more complicated).

I usually use the third form. The else in your examples is superfluous, as is the die statement. However - if the conditionals become more complicated over time, it would be a good idea to have a default 'die' to catch any errors I made in nesting or in my logic.

It seems to me that perl would optimize away the 'else' in your first example so that this sub became equivalent to the third.

I'm a little unclear on what you're trying to achieve. Are you using a sub because this code will be called from multiple places or are you just using it because COND and the return values are too messy for a ternary?

If the latter then you might try

$value = do {
  if (COND) {
    1;
  } else {
    2;
  }
};
[download]

Either way, the second construction with the die is kinda pointless as the die can never be reached.

Which one of the below three examples would you prefer?

It depends :-)

sub sg {
  if (COND) {
    return 1;
  } else {
    return 2;
  }
}
[download]

I'd do this if both paths through the code were equally likely.

sub sg {
  if (COND) {
    return 1;
  } else {
    return 2;
  }
  die "should never reach this";
}
[download]

I'd never do this. I'd trust my test suite to catch any stupid code changes I might make rather than littering my code with die statements.

sub sg {
  if (COND) {
    return 1;
  }
  return 2;
}
[download]

I'd do this if 2 would be the "normal" result - although I'd probably write it as

sub sg {
  return 1 if COND;
  return 2;
}
[download]

As another option - if this was a method in some OO code I'd be looking to see whether that if statement was a sign that I was missing some polymorphic behaviour somewhere that I could factor out into separate methods.

Two alternatives that noone else has mentioned yet:

# both branches are very long

sub wibble {
    return 1;
}

sub wobble {
    return 2;
}

sub sg {
    return COND ? wibble( @_ ) : wobble( @_ );
}
[download]

# one branch (symbolised here by `return 2`) is much shorter than the 
+other

sub wibble {
    return 1;
}

sub sg {
    return wibble( @_ ) if COND;
    return 2;
}
[download]

If you avoid monolithic conditional code blocks, most of the time your question will trivially resolve itself.

Makeshifts last the longest.

My tendency is to the first construct when COND is true or false based on an option contained in the argument list and the third when COND is true or false based on improper inputs, e.g., such as log(0) or on external factors (the network is down).

I find the second case unesthetic, as the die statement is, at best, superfluous. Indeed, I would not be surprised if Perl optimized it away.

I assumed the second case was really supposed to be more like this:

if ( CASE1) {
    return 1;
} elsif ( CASE2 ) {
    return 2;
} else {
    die("Must be one of CASE1 or CASE2");
}
[download]

That is, a case where formal logic without knowledge of the inputs can't tell you what the results will be, but the business rules demand that the result be one of two choices.

It's always good to have an exception handler for when things that other people swear will never, ever happen suddenly do happen anyway. ;-)

--
Ytrew

Although there are a lot of answers already, an important point is missing: Gernerally speaking, your question is not a question of style. It is a question of logic and should be guided by the imperative:

Be explicit!

Let's assume that COND is (COND A or COND B). Then, what is the proper meaning of else? (ok, that was purely rhetorical.)

In other words, except for the most trivial CONDs I would always use the most explicit form, even dispatch tables.