Is a big boolean expression more readable than the same expression broken down into predicate methods?

Question

What is easier to understand, a big boolean statement (quite complex), or the same statement broken down into predicate methods (lots of extra code to read)?

Option 1, the big boolean expression:

    private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
    {

        return propVal.PropertyId == context.Definition.Id
            && !repo.ParentId.HasValue || repo.ParentId == propVal.ParentId
            && ((propVal.SecondaryFilter.HasValue && context.SecondaryFilter.HasValue && propVal.SecondaryFilter.Value == context.SecondaryFilter) || (!context.SecondaryFilter.HasValue && !propVal.SecondaryFilter.HasValue));
    }

Option 2, The conditions broken down into predicate methods:

    private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
    {
        return MatchesDefinitionId(context, propVal)
            && MatchesParentId(propVal)
            && (MatchedSecondaryFilter(context, propVal) || HasNoSecondaryFilter(context, propVal));
    }

    private static bool HasNoSecondaryFilter(CurrentSearchContext context, TValToMatch propVal)
    {
        return (!context.No.HasValue && !propVal.SecondaryFilter.HasValue);
    }

    private static bool MatchedSecondaryFilter(CurrentSearchContext context, TValToMatch propVal)
    {
        return (propVal.SecondaryFilter.HasValue && context.No.HasValue && propVal.SecondaryFilter.Value == context.No);
    }

    private bool MatchesParentId(TValToMatch propVal)
    {
        return (!repo.ParentId.HasValue || repo.ParentId == propVal.ParentId);
    }

    private static bool MatchesDefinitionId(CurrentSearchContext context, TValToMatch propVal)
    {
        return propVal.PropertyId == context.Definition.Id;
    }

I prefer the second approach, because I see the method names as comments, but I understand that it's problematic because you have to read all the methods to understand what the code does, so it abstracts the code's intent.

Answer 1

What is easier to understand

The latter approach. It's not only easier to understand but it is easier to write, test, refactor and extend as well. Each required condition can be safely decoupled and handled in it's own way.

it's problematic because you have to read all the methods to understand the code

It's not problematic if the methods are named properly. In fact it would be easier to understand as the method name would describe the intent of condition.
For an onlooker if MatchesDefinitionId() is more explanatory than if (propVal.PropertyId == context.Definition.Id)

[Personally, the first approach sores my eyes.]

Answer 2

If this is the only place these predicate functions would be used, you can also use local bool variables instead:

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
{
    bool matchesDefinitionId = (propVal.PropertyId == context.Definition.Id);
    bool matchesParentId = (!repo.ParentId.HasValue || repo.ParentId == propVal.ParentId);
    bool matchesSecondaryFilter = (propVal.SecondaryFilter.HasValue && context.No.HasValue && propVal.SecondaryFilter.Value == context.No);
    bool hasNoSecondaryFilter = (!context.No.HasValue && !propVal.SecondaryFilter.HasValue);

    return matchesDefinitionId
        && matchesParentId
        && matchesSecondaryFilter || hasNoSecondaryFilter;
}

These could also be broken down further and reordered to make them more readable, e.g. with

bool hasSecondaryFilter = propVal.SecondaryFilter.HasValue;

and then replacing all instances of propVal.SecondaryFilter.HasValue. One thing that immediately sticks out then is that hasNoSecondaryFilter uses logical AND on the negated HasValue properties, while matchesSecondaryFilter uses a logical AND on un-negated HasValue -- so it's not the exact opposite.

Answer 3

In general, the latter is preferred.

It makes the call site more reusable. It supports DRY (meaning you have less places to change when the criteria change, and can do it more reliably). And very often those sub-criteria are things that will be reused independently elsewhere, allowing you to do that.

Oh, and it makes this stuff a lot easier to unit test, giving you confidence that you've done it correctly.

Answer 4

If it's between these two choices, then the latter is better. These are not the only choices, however! How about breaking up the single function into multiple ifs? Test for ways to exit the function to avoid additional tests, roughly emulating a "short circuit" in a single line test.

This is easier to read (you might need to double check the logic for your example, but the concept holds true):

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
{
    if( propVal.PropertyId != context.Definition.Id ) return false;

    if( repo.ParentId.HasValue || repo.ParentId != propVal.ParentId ) return false;

    if( propVal.SecondaryFilter.HasValue && 
        context.SecondaryFilter.HasValue && 
        propVal.SecondaryFilter.Value == context.SecondaryFilter ) return true;

    if( !context.SecondaryFilter.HasValue && 
        !propVal.SecondaryFilter.HasValue) return true;

    return false;   
}

Answer 5

I like option 2 better, but would suggest one structural change. Combine the two checks on the last line of the conditional into a single call.

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal)
{
    return MatchesDefinitionId(context, propVal)
        && MatchesParentId(propVal)
        && MatchesSecondaryFilterIfPresent(context, propVal);
}

private static bool MatchesSecondaryFilterIfPresent(CurrentSearchContext context, 
                                                    TValToMatch propVal)
{
    return MatchedSecondaryFilter(context, propVal) 
               || HasNoSecondaryFilter(context, propVal);
}

The reason I suggest this is that the two checks are a single functional unit, and nesting parenthesis in a conditional is error prone: Both from the standpoint of initially writing the code and from the standpoint of the person reading it. This is especially the case if the sub-elements of the expression don't follow the same pattern.

I'm not sure if MatchesSecondaryFilterIfPresent() is the best name for the combination; but nothing better is immediately coming to mind.

Answer 6

Though in C#, the code is not very object oriented. It is using static methods and what looks like static fields (e.g. repo). It is generally held that statics make your code hard to refactor and difficult to test, while hampering reusability, and, to your question: static usage like this is less readable & maintainable than object-oriented construction.

You should convert this code to a more object-oriented form. When you do, you'll find that there are sensible places to put code that does comparison of objects, of fields, etc.. It is likely that you could then ask objects to compare themselves, which would reduce your big if statement to a simple request to compare (e.g. if ( a.compareTo (b) ) { }, which could include all the field comparisons.)

C# has a rich set of interfaces and system utilities for doing comparisons on objects and their fields. Beyond the obvious .Equals method, for starters, look into IEqualityComparer, IEquatable, and utilities like System.Collections.Generic.EqualityComparer.Default.

Answer 7

The latter is definitely preferred, I have seen cases with the first way and it's almost always impossible to read. I have made the mistake of doing it the first way and was asked to change it to predicate methods.

Answer 8

I would say that the two are about the same, IF you add some whitespace for readability and some comments to help the reader over the more obscure parts.

Remember: good commentary tells the reader what you were thinking when you wrote the code.

With changes such as I've suggested, I would probably go with the former approach, as it is less cluttered and diffuse. Subroutine calls are like footnotes: they provide useful information but disrupt the flow of reading. If the predicates were more complex, then I would break them out into separate methods so that the concepts they embody can be built up in understandable chunks.

Answer 9

Well, if there are parts you might want to reuse, separating them out into separate properly named functions is obviously the best idea.
Even if you might never reuse them, doing so might allow you to better structure your conditions and give them a label describing what they mean.

Now, let's look at your first option, and concede that neither was your indentation and line-breaking all that useful, nor was the conditional structured all that well:

private static bool ContextMatchesProp(CurrentSearchContext context, TValToMatch propVal) {
    return propVal.PropertyId == context.Definition.Id && !repo.ParentId.HasValue
        || repo.ParentId == propVal.ParentId
        && propVal.SecondaryFilter.HasValue == context.SecondaryFilter.HasValue
        && (!propVal.SecondaryFilter.HasValue || propVal.SecondaryFilter.Value == context.SecondaryFilter.Value);
}

Answer 10

The first one is absolutely horrible. You have been using || for two things on the same line; that is either a bug in your code or an intent to obfuscate your code.

    return (   (   propVal.PropertyId == context.Definition.Id
                && !repo.ParentId.HasValue)
            || (   repo.ParentId == propVal.ParentId
                && (   (   propVal.SecondaryFilter.HasValue
                        && context.SecondaryFilter.HasValue 
                        && propVal.SecondaryFilter.Value == context.SecondaryFilter)
                    || (   !context.SecondaryFilter.HasValue
                        && !propVal.SecondaryFilter.HasValue))));

That's at least halfway decently formatted (if the formatting is complicated, that's because the if-condition is complicated), and you have at least a chance to figure out if anything in there is nonsense. Compared to your rubbish formatted if, anything else is better. But you seem to be able to do only extremes: Either a complete mess of an if statement, or four completely pointless methods.

Note that (cond1 && cond2) || (! cond1 && cond3) can be written as

cond1 ? cond2 : cond3

which would reduce the mess. I'd write

if (propVal.PropertyId == context.Definition.Id && !repo.ParentId.HasValue) {
    return true;
} else if (repo.ParentId != propVal.ParentId) {
    return false;
} else if (propVal.SecondaryFilter.HasValue) {
    return (   context.SecondaryFilter.HasValue
            && propVal.SecondaryFilter.Value == context.SecondaryFilter); 
} else {
    return !context.SecondaryFilter.HasValue;
}

Answer 11

I don't like either of those solutions, they are both hard to reason about, and difficult to read. Abstraction to smaller methods just for smaller methods sake doesn't always solve the problem.

Ideally, i think you would metaprogrmatically compare properties, so you don't have a define a new method or if branch every time you want to compare a new set of properties.

I am not sure about c#, but in javascript something like this would be MUCH better and could at least replace MatchesDefinitionId and MatchesParentId

function compareContextProp(obj, property, value){
  if(obj[property])
    return obj[property] == value
  return false
}