If statements in a function what is the best way

Question

If I have a function with a ton of conditionals what is the best way to organize it?

What I am worried about is someone else coming into the code and understanding what is going on. Even though the example is simple imagine that the conditional is very complex.

For an example:

public void function(string value, string value2)
{
    if (value == null)
        return;

    if (value2 == value)
        DoSomething();
}

or

public void function(string value, string value2)
{
    if (value != null)
    {
        if (value2 == value)
            DoSomething();
    }
}

or

public void function(string value, string value2)
{
    if (value != null && value2 == value)
        DoSomething();
}

Answer 1

I prefer the first option - fail fast is cleaner, clearer, and easier to read and understand.

I understand that this isn't a failure but the concept still applies. I really don't like nested if statements at all.

Answer 2

You can look at defensively programming to ensure the contract for the methods functionality can be fulfilled.

public void function(string value, string value2)
{
    if (string.IsNullOrEmpty(value1)) throw new ArgumentNullException("value1", "value 1 was not set");
    if (string.IsNullOrEmpty(value2)) throw new ArgumentNullException("value2", "value 2 was not set");

    DoSomething();
}

Answer 3

I like the third option, but this really depends on the language. You are assuming that in the third one that the first part of the statement will fail and not execute the second. This is language dependent. I know that most C based languages to do this, but since you didn't specify one that is a potential problem. There could be one out there that I am not aware of that doesn't have the short circuit concept.

Answer 4

Hi

3rd one is more compact. I guess 2nd one would involve too many nested conditionals and would obscure readability.

Since your main concern is code maintenance i.e. people joining down the line should be able to gain speed, then 1st is the best as it promotes better understanding and if I were to go by the example then not much commenting is also needed in the 1st.

cheers

Answer 5

If you have a function with a lot of conditionals, I'd use a switch statement -- not ifs. I might also break the details up in to several functions (or even classes) if that is possible.

Related Stack Overflow Articles:

• is “else if” faster than “switch() case” ?
• When to use If-else if-else over switch statments and vice versa

Answer 6

The fact that you are thinking in advance about code readability is half the battle.

As to which of your examples is most readable, that is pretty subjective.

My thoughts on the examples given:

• Personally, I think the first example is easiest to follow.
• Minimizing the nesting levels and/or number of conditionals usually enhances readability.
• Some would argue against multiple exit points from a method (like example 1), but I think t only becomes a problem when the method gets really long. It isn't really such a big deal if you are just checking inputs and failing quickly.

Answer 7

My preference is the second option. Personally when I read code like that I keep in mind the conditions under which I would enter each nested level. In the first example I would probably forget that the first condition (value == null is false) continues to hold. The third one isn't bad either but I prefer the 2nd.

Answer 8

Refactor that stuff out into it's own function. It's way better to read a descriptive function name than a bunch of boolean expressions.

// leave complex conditional code out, so that we can focus on the larger problem in the function
public void function(string value, string value2)
{
    if (MyDescriptiveTestName)
    {
        DoSomething();
    }
}

// encapsulate complex conditional code so that we can focus solely on it.
private bool MyDescriptiveTestName(string value, string value2)
{
    if (value != null && value2 == value)
    {
        return true;
    }
    return false;
}

Answer 9

Organize the conditions and put them into a method.

for instance replace this:

 if( a& & n || c  && ( ! d || e ) && f > 1 && ! e < xyz ) { 
      // good! planets are aligned.
      buyLotteryTicket();
 } else if( ..... oh my ... ) { 
 }

Into this:

if( arePlanetsAligned() ) { 
    buyLotteryTicket(); 
} else if( otherMethodHere() ) { 
   somethingElse();
}

That way it doesn't really matter what style you use ( 1, 2 or 3 ) because the if statement will clearly describe what's the condition being tested. No need for additional constructs.

The point is to make the code clearer and self documenting. If you are using a OO programming language you can use an object to store the state ( variables ) and avoid creating methods that take 5 - 10 parameters.

These are similar questions:

Best way to get rid of nested ifs

Is there an alternative to this hyperidented code

The second link one shows a more complete and complex way to transform an horrible's everyone maintainer nightmare into a self documenting code.

It shows how to transform this:

public String myFunc(SomeClass input)
{
    Object output = null;

    if(input != null)
    {
        SomeClass2 obj2 = input.getSomeClass2();
        if(obj2 != null)
        {
            SomeClass3 obj3 = obj2.getSomeClass3();
            if(obj3 != null && !BAD_OBJECT.equals(obj3.getSomeProperty()))
            {
                SomeClass4 = obj3.getSomeClass4();
                if(obj4 != null)
                {
                    int myVal = obj4.getSomeValue();
                    if(BAD_VALUE != myVal)
                    {
                        String message = this.getMessage(myVal);
                        if(MIN_VALUE <= message.length() &&
                           message.length() <= MAX_VALUE)
                        {
                            //now actually do stuff!
                            message = result_of_stuff_actually_done;
                        }
                    }
                }
            }
        }
    }
    return output;
}

into this:

if ( isValidInput() && 
    isRuleTwoReady() &&
    isRuleTreeDifferentOf( BAD_OBJECT ) &&
    isRuleFourDifferentOf( BAD_VALUE ) && 
    isMessageLengthInRenge( MIN_VALUE , MAX_VALUE ) ) { 
            message = resultOfStuffActuallyDone();
}

Answer 10

May I recommend the book Clean Code by Robert C. Martin which provides a great set of heuristics for writing readable and maintainable code.

Now another option would be to extract the conditional into another private function and name it so that it describes your intent. It doesn't work too well with the supplied code as it's generic but it would look something like:

public void function(string value, string value2)
{
    if (valuesAreValidAndEqual(value, value2))
    {
        DoSomething();
    }
}

private void valuesAreValidAndEqual(string value, string value2)
{
    return value != null && value2 == value;
}

Clearly this is more useful if the variable names and function names are related to your domain.

Answer 11

The fact that you have that many statements in a function is a sign that the function should be divided into smaller ones.

There is no BEST way of placing if statements, I think the answers are subjective.

Answer 12

Clarity is often a hard quality to judge. What's clear to you when you're writing a piece of code may be completely obtuse to someone else - or even to yourself after enough time goes by.

Here are my personal rules of thumb when building a function with many conditional checks:

1. Group conditions into logical units, when possible
2. Use clearly named intermediate values to simplify readability and debugging.
3. Avoid repeating the same logical constructs more than once (see above).
4. When possible, refactor complex or expensive conditions into separate functions.
5. When possible, use early exit conditions at the top of the method to exit, but...
6. Avoid peppering return statements throughout the body of the method.
7. Do not rely on operator precedence, use parentheses to group statements.
8. Avoid relying on indenting, use blocks ({...}) even with only one statement, this help with maintainability.