Where is the right balance of predicates in a single if statement?

Question

I personally have no problem with the following code

if (Object foo != null && !String.IsNullOrEmpty(foo["bar"]))
{
    // do something
}

Because I think the following is too verbose

if (Object foo != null)
{
    if (!String.IsNullOrEmpty(foo["bar"]))
    {
        // do something
    }
}

But I wouldn't go so far with this standpoint if say there were 5 predicates and I had to wrap the text in the editor to see them all at once, is there a logical "line" that you draw as to how many predicates you include in a single if statement in a similar sense to saying that methods should never require more than 7 parameters

Answer 1

Not the number of predicates but the complexity counts. As long as you can understand what the code does with minimal effort, it looks ok to me.

To add to that, I would not change to multiple if's but I add a function for the predicates. Specially if the same number of predicates are used at several places.

Answer 2

I don't think there are any rules, however:

1. is it sufficiently complex that when you show it to someone else, they struggle to understand it ?
2. does it need to cover multiple lines ?
3. do you have repeated condition checking in multiple 'if' clauses ? This would point to a required refactoring into some method

If any of the above apply, I would rework it.

Answer 3

I think it's a balance of the different types of operators and correct formatting. If all of the operators are the same (all "and" or all "or"), then you can probably concatenate together an indefinite number of expressions without losing comprehension:

if (something() &&
    something_else() &&
    so_on() &&
    so_forth() &&
    some_more_stuff() &&
    yada() &&
    yada() &&
    yada() &&
    newman() &&
    soup_nazi() &&
    etc())
  ...

Answer 4

It depends on what your are doing. The second statement produces a different result when you reverse it. ( add a "not" in front of it ) and is a very common source of bugs.

I have seen code with around 20 predicates which works (or at least, works well enough!) The rule of thumb I use, if it looks like a dogs dinner, I consider refactoring.

Answer 5

I don't believe that there's a magic number. If all the predicates make sense together, then I will put them together. This might involve splitting the if statement over two lines, but I usually never introduce extra if statements that are superfluous. But if it is particularly long, you should ask yourself if all the statements are really necessary. Perhaps you could filter out some of the values earlier or something like that. The biggest concern is readability. If it is hard for someone else to understand, you need to refactor your code. But splitting up the code into two different if statements rarely makes the code more readable, it just takes up more lines.

Answer 6

Short term memory has a capacity of seven items, give or take two. This implies that an expression involving more than five dissimilar objects might require one to pause and think about it.

Answer 7

I don't think rewriting if-statements in two is the way, especially if you consider that adding an else clause to your examples gives different results. If I wanted to reduce the number of atomic propositions in the condition, I would factor some that make sense together to a function of their own, like:

if(won_jackpot(obj)) ...

Answer 8

This issue with long lists of conditions is not so much the loss of readability, but the loss of testability. Especially when dealing with bad method arguments sometimes it really is easier to ask for forgiveness rather than permission (eg see answer to this question). That way you keep your code clean and testable, and either fix the caller or let them handle the resulting exception.

Answer 9

It all depends on what needs to be done. But one thing you may want to look into that will make certain algorithms less verbose is a switch statement:

switch (myVal)
{
    case "isThis":
    case "isThat":
    case "something else":
        doThis();
        break;
    case "another":
    case "yet another":
    case "even another":
        doThat();
        break;
    case "another one":
    case "more more":
        doThisAgain();
        break;
}

Doing that would have been quite verbose in if else statements. Some code will need tons of if and else statements, some will be able to be condensed, etc. Just never sacrifice the quality of the execution of the code for the prettiness of the source of the code.

Answer 10

Which way is better? Neither. The semantics of both are different.

I agree though splitting makes debugging easier, but so does conditional breakpoints :)

If it's a simple combination of 'AND's or 'OR's that's longer than 3 tests, refactor it.