Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

Consider this bit of code:

if (x == 1)
{
  throw "no good; aborting" ;
}

[... more code ...]

Now consider this code:

if (x == 1)
{
  throw "no good; aborting" ;
}
else
{
  [... more code ...]
}

The two cases work exactly the same way. The first case has the advantage that you don't have to "encase" the rest of the code in an else. The second has the advantage of following the practice of explicitly having an else for every if.

Can anyone provide any solid arguments in favor of one over the other?

share|improve this question
5  
The practice you cite in favor of the explicit else seems bogus. Quite often, there simply is nothing to put into the else block unless you bend backwards. –  delnan Oct 30 '12 at 17:20
    
Consistency? Allow robustness in the face of code change? Readability? –  Thomas Eding Oct 30 '12 at 17:24
1  
Ehhhh, I'm not so much a fan of the idea that every if needs an else. The last programmer that worked on our codebase followed that rigidly (well, sometimes... it's kind of schizophrenic). As a result, we have a lot of entirely meaningless else { /* do nothing */ } blocks littering the code... –  KChaloux Oct 30 '12 at 18:39
4  
"An else for every if" seems like some bizarre proclamation issued by a cloud architect in the name of (a foolish) consistency. I see no advantage to following that practice and have never even heard of it anywhere. –  Erik Dietrich Oct 30 '12 at 19:29
    
It's a redundant else. If you are working with .NET stack, than install ReSharper and it'll remind you to remove all the redundant else statements. –  CodeART Oct 30 '12 at 23:13
add comment

2 Answers 2

up vote 15 down vote accepted

You should not add else after if branches that break control flow unconditionally, such as ones containing a throw or a return. This would improve readability of your program by removing the unnecessary level of nesting introduced by the else branch.

What looks more or less OK with a single throw becomes truly ugly when several throws in a row get involded:

void myMethod(int arg1, int arg2, int arg3) {
    // This is demonstrably ugly - do not code like that!
    if (!isValid(arg1)) {
        throw new ArgumentException("arg1 is invalid");
    } else {
        if (!isValid(arg2)) {
            throw new ArgumentException("arg2 is invalid");
        } else {
            if (!isValid(arg3)) {
                throw new ArgumentException("arg3 is invalid");
            } else {
                // The useful code starts here
            }
        }
    }
}

This snippet does the same thing, but it looks much better:

void myMethod(int arg1, int arg2, int arg3) {
    if (!isValid(arg1)) {
        throw new ArgumentException("arg1 is invalid");
    }
    if (!isValid(arg2)) {
        throw new ArgumentException("arg2 is invalid");
    }
    if (!isValid(arg3)) {
        throw new ArgumentException("arg3 is invalid");
    }
    // The useful code starts here
}
share|improve this answer
    
+1 True. Second OP case forces you to read carefully, then leaves you with a WTF. But... always try to make methods short. A return in the middle of a 200 lines method is also bad. –  user61852 Oct 30 '12 at 17:38
1  
To be fair, if you are just using repeated if's you can do else if. –  Guvante Oct 30 '12 at 17:44
    
+1 for simplicity. –  Cape Cod Gunny Oct 30 '12 at 17:50
2  
@Guvante: Each if tests for a single condition and deals with it if the condition is true, and unless there's some alternate thing that has to happen if the condition is false, else ifs are unnecessary. We have a term around my office for code like dasblinkenlight's first snippet: "pachinko machines." –  Blrfl Oct 30 '12 at 17:51
    
@Blrfl pachinko machines hah, perfect analogy +1 –  Jimmy Hoffa Oct 30 '12 at 18:12
show 1 more comment

I would call the "explicit else" practice you refer to as an anti-pattern, as it obscures the fact that there is no special-case code as an else to your if.

Readability/maintainability is generally improved when you mostly have nothing but necessary code-flow constructs, and you minimize them. This means redundant elses and if's which will add a scope to an entire function make following and maintaining it more difficult.

Say for example you have this function:

public void ConfigureOblogon(Oblogon oblogonToConfigure)
{
    if (_validColors.Contains(oblogonToConfigure.Color))
    {
        oblogonToConfigure.ColorIndex = _validColors.IndexOf(oblogonToConfigure.Color);
    }
    else
    {
        oblogonToConfigure.Color = _validColors[0];
        oblogonToConfigure.ColorIndex = 0;
    }
}

Now the requirement comes in that during configuration you should also specify the oblogon's type/type index, there are multiple scopes which someone could place that code and end up with invalid code i.e.

public void ConfigureOblogon(Oblogon oblogonToConfigure)
{
    if (!_validOblogons.Contains(oblogonToConfigure.Type))
    {
        oblogonToConfigure.Type = _validOblogons[0];
        oblogonToConfigure.TypeIndex = 0;
        if (_validColors.Contains(oblogonToConfigure.Color))
        {
            oblogonToConfigure.ColorIndex = _validColors.IndexOf(oblogonToConfigure.Color);
        }
        else
        {
            oblogonToConfigure.Color = _validColors[0];
            oblogonToConfigure.ColorIndex = 0;
        }
    }
    else
    {
        oblogonToConfigure.TypeIndex = _validOblogons.IndexOf(oblogonToConfigure.Type);
    }
}

Compare this to if the original code were written with minimal control flow constructs necessary and minimized ones at that.

public void ConfigureOblogon(Oblogon oblogonToConfigure)
{
    if (!_validColors.Contains(oblogonToConfigure.Color))
    {
        oblogonToConfigure.Color = _validColors[0];
    }

    oblogonToConfigure.ColorIndex = _validColors.IndexOf(oblogonToConfigure.Color);
}

It would now be far more difficult to accidentally put something in the wrong scope or end up bloating scopes causing duplication in the long term growth and maintenance of this function. Plus it's obvious what the possible flows through this function are so readability is enhanced.

I know, the example's a bit contrived, but I have many times seen

SomeFunction()
{
    if (isvalid)
    {
        /* ENTIRE FUNCTION */
    }
    /* Nothing should go here but something does on accident, and an invalid scenario is created. */
}

So formalizing those rules about control-flow constructs I think may help folks develop the intuition necessary to smell something when they start writing code like that. Then they will start to write..

SomeFunction()
{
    if (!isvalid)
    {
        /* Nothing should go here, and it's so small no one will likely accidentally put something here */
        return;
    }

    /* ENTIRE FUNCTION */
}
share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.