Which way of writing a conditional test is the most readable?

Question

I have three conditions like the following:

if (IsAllTriggersRequired && meterFlag && workFlag && timeFlag)
{
    result.Add(new TodayTaskTest { Code = item1.Code, Id = item1.Id, Title = item1.Title });
}
else if (!IsAllTriggersRequired && (meterFlag || workFlag || timeFlag))
{
    result.Add(new TodayTaskTest { Code = item1.Code, Id = item1.Id, Title = item1.Title });
}

if ((IsAllTriggersRequired && meterFlag && workFlag && timeFlag) ||(!IsAllTriggersRequired && (meterFlag || workFlag || timeFlag)))
{
    result.Add(new TodayTaskTest { Code = item1.Code, Id = item1.Id, Title = item1.Title });
}

bool isAdd = false;

if (IsAllTriggersRequired && meterFlag && workFlag && timeFlag)
    isAdd = true;
else if (!IsAllTriggersRequired && (meterFlag || workFlag || timeFlag))
    isAdd = true;

if (isAdd)
    result.Add(new TodayTaskTest { Code = item1.Code, Id = item1.Id, Title = item1.Title });

In the second one, can I avoid repeating the same code?

Write the condition in terms of what it means in terms of your business rules, problem domain, user perspective. Next I'd have a final "else" if only to trap errors. Next, #2 is the worst of the 3 for readability. #3 is next worst - "isAdd"; add what? Again, use the "business" terminology. — radarbob
– radarbob, Commented Jan 3, 2017 at 13:43
Please state the purpose of this code in the title, as required by the How to Ask guidelines. Also provide adequate context (ideally an entire function or class) so that we may advise you properly. What are these flags in the code? — 200_success
– 200_success, Commented Jan 3, 2017 at 15:05

JanDotNet · Accepted Answer · 2017-01-03 14:41:12Z

7

I would suggest to add two more intermediate variables:

var allTriggersAvailable = meterFlag && workFlag && timeFlag;
var anyTriggerAvailable = meterFlag || workFlag || timeFlag;

if (allTriggersAvailable || (!IsAllTriggersRequired && anyTriggerAvailable))
{
    result.Add(new TodayTaskTest { Code = item1.Code, Id = item1.Id, Title = item1.Title });
}

edited Jan 3, 2017 at 14:41

answered Jan 3, 2017 at 13:29

JanDotNet

8,5982 gold badges22 silver badges48 bronze badges

Add a comment |

user378230 · Accepted Answer · 2017-01-03 16:10:43Z

3

You could use a little linq to make it slightly more documenting:

var triggers = new[] { meterFlag, workFlag, timeFalg };
var addToResults = AllTriggersRequired ? flags.All(x => x) : flags.Any(x => x);

if(addToResults) 
{
     result.Add(new TodayTaskTest() 
     { 
         Code = item1.Code,
         Id = item1.Id,
         Title = item1.Title 
     });
}

Might be slightly less performant than other suggestions but would be easier to extend if you added more flags in future.

answered Jan 3, 2017 at 16:10

user378230

411 bronze badge

2

\$\begingroup\$ This is less readable to me. \$\endgroup\$

404
– 404

2017-01-03 16:15:38 +00:00
Commented Jan 3, 2017 at 16:15
2

\$\begingroup\$ Slower, uglier, non-idomatic, completely over-engineered. \$\endgroup\$

Cody Gray
– Cody Gray

2017-01-03 16:18:54 +00:00
Commented Jan 3, 2017 at 16:18
1

\$\begingroup\$ I like it and you'll get +1 from me for a different point of view. \$\endgroup\$

t3chb0t
– t3chb0t

2017-01-03 16:45:47 +00:00
Commented Jan 3, 2017 at 16:45

Add a comment |

Philip Rollins · Accepted Answer · 2017-01-03 15:53:10Z

1

I mostly agree with JanDotNet, however in the Refactoring and 'Becoming a better programmer' books it's recommended NOT to use variables to hold conditional expressions but rather separate them out into their own functions/methods.

Do note that this code will not work unless the *Flag are member variables and not local ones.

if (AllFlagsTriggered() || (!IsAllTriggersRequired && AnyFlagTriggered()))
{
    result.Add(new TodayTaskTest { 
        Code = item1.Code,
        Id = item1.Id,
        Title = item1.Title
    });
}

bool AllFlagsTriggered() {
    return (meterFlag && workFlag && timeFlag);
}

bool AnyFlagTriggered() {
    return (meterFlag || workFlag || timeFlag);
}

Notice I removed the check for IsAllTriggersRequired, there's no reason to check the truth to that variable as either way the condition will evaluate to true.

answered Jan 3, 2017 at 15:53

Philip Rollins

2461 silver badge4 bronze badges

1

\$\begingroup\$ This only makes sense if those conditions need to be tested in multiple places (to reduce code duplication). If there's only one place where they are tested, this just makes the code less readable because you have to go find that function to see what it does. Choosing a descriptive name for the condition variable satisfies the motivation behind extracting it into a function. \$\endgroup\$

Cody Gray
– Cody Gray

2017-01-03 16:05:26 +00:00
Commented Jan 3, 2017 at 16:05
\$\begingroup\$ I kinda agree, but to me, it makes it more readable plus the OP said he has 3 conditionals and listed them (he never indicated if he was using all 3). Functions should tell you what they do I can quickly look at AllFlagsTriggered and know all flags must be triggered, now it's not a great description because it doesn't say what flags it's checking perhaps should use AreAllFlagsTriggered to indicate it's a boolean type. But I'd personally use an ENUM with the FLAGS attribute to improve code readability, but that isn't what the OP asked for, he asked to remove duplicate code. \$\endgroup\$

Philip Rollins
– Philip Rollins

2017-01-03 16:25:33 +00:00
Commented Jan 3, 2017 at 16:25

Add a comment |

Stack Exchange Network

Which way of writing a conditional test is the most readable?

3 Answers 3

You must log in to answer this question.

Hot Network Questions

Which way of writing a conditional test is the most readable?

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions