Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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.

share|improve this question

closed as primarily opinion-based by gnat, Snowman, maple_shaft Mar 11 at 18:12

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise.If this question can be reworded to fit the rules in the help center, please edit the question.

13  
Option 2 is similar to what Martin Fowler recommends in his refactoring book. Plus your method names serve as the intent of all the random expressions, the content of the methods are just the implementation details that could change over time. – Jason Mar 9 at 18:00
7  
Thank you for the term "predicate methods". – HireThisMarine Mar 9 at 18:47
3  
What @thepacker says. The fact that doing it the first way has caused you to make a mistake is a pretty good clue that the first way is not easily understandable to a very important sector of your target audience. Yourself! – Steve Jessop Mar 10 at 11:08
3  
Option 3: I don't like either one. The second is ridiculously verbose, the first is not equivalent to the second. Parentheses help. – David Hammen Mar 10 at 15:58
3  
This may be pedantic, but you don't have any if statements in either block of code. Your question is about Boolean expressions. – Kyle Strand Mar 10 at 21:03

11 Answers 11

up vote 88 down vote accepted

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.]

share|improve this answer
12  
If the methods names are good, then it is also easier to understand. – BЈовић Mar 10 at 8:39
    
And please, make them (method names) significative and short. 20+ chars method names sore my eyes. MatchesDefinitionId() is borderline. – Mindwin Mar 10 at 18:12
2  
@Mindwin If it comes down to a choice between keeping method names "short" and keeping them meaningful, I say take the latter every time. Short is good, but not at the expense of readability. – Ajedi32 Mar 11 at 14:11
    
@Ajedi32 one doesn't have to write an essay on what the method does on the method name, or to have gramatically sound method names. If one keeps the abbreviation standards clear (across the work group or organization) won't be a problem with short names and readability. – Mindwin Mar 11 at 14:29
    
Use Zipf's law: make things more verbose to discourage their use. – hoosierEE Mar 11 at 15:51

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.

share|improve this answer
3  
This solution is pretty good and I've certainly written lots of similar code. It's very readable. The downside, compared to the solution I posted, is speed. With this method, you perform a pile of conditional tests no matter what. In my solution, the operations can be dramatically reduced based on the values processed. – BuvinJ Mar 10 at 16:52
5  
@BuvinJ Tests like the ones shown here should be fairly cheap, so unless I know some of the conditions are expensive or unless this is extremely performance sensitive code, I would go for the more readable version. – svick Mar 10 at 17:03
1  
@svick No doubt this is unlikely to introduce a performance issue most of the time. Still, if you can reduce operations without losing readability, then why not do so? I'm not convinced this is much more readable than my solution. It does give self documenting "names" to the tests - which is nice... I think it comes down to the specific use case and how understandable the tests are in their own right. – BuvinJ Mar 10 at 17:12
    
Adding comments can help readability too... – BuvinJ Mar 10 at 17:14
    
@BuvinJ What I really like about this solution is that by ignoring everything except the last line, I can quickly understand what it's doing. I do think this is more readable. – svick Mar 10 at 17:22

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.

share|improve this answer
1  
Yes, though your answer should also address fixing the use of repo, which appears a static field/property, ie a global variable. Statics methods should be deterministic and not use global variables. – David Arno Mar 9 at 21:48
3  
@DavidArno - while that is not great, it seems tangential to the question at hand. And without more code it's plausible that there's a semi-valid reason for the design to work like that. – Telastyn Mar 9 at 21:57
1  
Yes, nevermind repo. I had to obfuscate the code a little, don't want to share client code as-is on the interwebs :) – willem Mar 10 at 6:10

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;   
}
share|improve this answer
3  
Why did I get a downvote for this within seconds of posting it? Please add a comment when you downvote! This answer operates just as quickly and is easier to read. So what's the problem? – BuvinJ Mar 10 at 2:02
2  
@BuvinJ: Absolutely nothing wrong with it. The same as the original code, except you don't have to fight with a dozen parentheses and a single line that stretches past the end of the screen. I can read that code from top to bottom and understand it immediately. WTF count = 0. – gnasher729 Mar 10 at 15:20
1  
Returning other than at the end of the function makes code less readable, not more readable, IMO. I prefer single exit point. Some good arguments both ways at this link. stackoverflow.com/questions/36707/… – Brad Thomas Mar 10 at 15:26
5  
@Brad thomas I can't agree with the single exit point. It usually leads to deep nested parenthesis. The return ends the path so for me is much easier to read. – Borjab Mar 10 at 15:43
1  
@BradThomas I fully agree with Borjab. Avoiding deep nestings is actually why I use this style more often than to break up long conditional statements. I use to find myself writing code with tons of nestings. Then, I started looking for ways to hardly ever go more than one or two nestings deep, and my code has become MUCH easier to read and maintain as a result. If you can find a way to exit your function, do so as soon as possible! If you can find a way to avoid deep nestings and long conditionals, do so! – BuvinJ Mar 10 at 16:08

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.

share|improve this answer
    
Very nice, trying to explain what is being done inside the methods is actually better than just restructuring calls. – klaar Mar 11 at 9:08

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.

share|improve this answer
    
Nice suggestions, thanks Erik. – willem Mar 10 at 6:14

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.

share|improve this answer

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.

share|improve this answer
    
Deserves a +1. Good food for thought, even though not popular opinion based on the other answers. Thanks :) – willem Mar 10 at 6:13
1  
@willem No, it doesn't deserve +1. Two approaches are not the same. The additional comments are stupid and unneeded. – BЈовић Mar 10 at 8:45
2  
A good code NEVER depends on comments to be understandable. In fact the comments are the worst clutter a code could have. The code should speak for itself. Also, the two approaches that OP wants to evaluate can never be "about the same", no matter how many whitespaces one adds. – wonderbell Mar 10 at 9:29
    
It is better to have a meaningfull function name than having to read the comment. As stated in the "Clean Code" book a comment is a failure to express throw code. Why explain what you are doing when the function could have stated it much more clearly. – Borjab Mar 10 at 15:47

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);
}
share|improve this answer

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;
}
share|improve this answer

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
}
share|improve this answer
1  
Should not be a problem to implement something like this in C#. – StevieV Mar 9 at 18:05
    
I'm not seeing how a boolean combination of ~5 calls to compareContextProp(propVal, "PropertyId", context.Definition.Id) would be easier to read than the OP's boolean combination of ~5 comparisons of the form propVal.PropertyId == context.Definition.Id. It's significantly longer and adds an extra layer without really hiding any of the complexity from the call site. (if it matters, I did not downvote) – Ixrec Mar 22 at 9:21

protected by Community Mar 10 at 1:57

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

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