0
\$\begingroup\$

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?

\$\endgroup\$
2
  • \$\begingroup\$ 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. \$\endgroup\$
    – radarbob
    Jan 3, 2017 at 13:43
  • 2
    \$\begingroup\$ 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? \$\endgroup\$ Jan 3, 2017 at 15:05

3 Answers 3

7
\$\begingroup\$

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 });
}
\$\endgroup\$
0
3
\$\begingroup\$

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.

\$\endgroup\$
3
  • 2
    \$\begingroup\$ This is less readable to me. \$\endgroup\$
    – 404
    Jan 3, 2017 at 16:15
  • 2
    \$\begingroup\$ Slower, uglier, non-idomatic, completely over-engineered. \$\endgroup\$ 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
    Jan 3, 2017 at 16:45
1
\$\begingroup\$

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.

\$\endgroup\$
2
  • 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\$ 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\$ Jan 3, 2017 at 16:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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