1
\$\begingroup\$

I have a predicate builder that initially evaluates to false. Because of this, the very next expression should be a logical Or. But if I need a logical And I need to keep track of a boolean value. And this boolean value hasPassedFirstCheck is the thing I want to get rid of.

var cars = PredicateBuilder.False<Cars>();
var hasPassedFirstCheck = false;

if (filterObject.CarTypes.Contains(CarTypes.Truck))
{
    if (sf.ServiceTypes.Count < 2 && !hasPassedFirstCheck)
        cars = cars.And(x => x.CarType== CarType.DJ);
    else
        cars = hasPassedFirstCheck
            ? cars.Or(x => x.CarType== CarType.DJ)
            : cars.And(x => x.CarType== CarType.DJ);

    hasPassedFirstCheck = true;
}

if (filterObject.CarTypes.Contains(CarTypes.SUV))
{
    if (condition && !hasPassedFirstCheck)
        cars = cars.And(x => x.CarType == CarType.SUV);
    else
        cars = hasPassedFirstCheck 
            ? cars.Or(x => x.CarType == CarType.SUV)
            : cars.And(x => x.CarType == CarType.SUV);

    hasPassedFirstCheck = true;
}

if (filterObject.CarTypes.Contains(CarTypes.Limo))
{
    if (condition && !hasPassedFirstCheck)
        cars = cars .And(x => x.CarType == CarType.Limo);
    else
        cars = hasPassedFirstCheck 
            ? cars.Or(x => x.CarType== CarType.Limo)
            : cars.And(x => x.CarType== CarType.Limo);

    hasPassedFirstCheck = true;
}
if (filterObject.CarTypes.Contains(CarTypes.Race))
{
    if (condition && !hasPassedFirstCheck)
        cars = cars.And(x => x.CarType == CarType.Race);
    else
        cars = hasPassedFirstCheck
            ? cars.Or(x => x.CarType == CarType.Race)
            : cars.And(x => x.CarType == CarType.Race);

    hasPassedFirstCheck = true;
}

This also isn't really OOP. Any suggestion to refactor this is really appreciated.

\$\endgroup\$
5
  • 1
    \$\begingroup\$ In your first condition, you use the same predicate twice. Is this intended? \$\endgroup\$ Mar 14, 2015 at 23:57
  • 2
    \$\begingroup\$ Also is condition the same everywhere or is this simply a place holder for a check? If it's a random place holder then your question is off-topic because it is effectively pseudo code. It would be the best if the code to be reviewed would be compileable. \$\endgroup\$
    – ChrisWue
    Mar 15, 2015 at 8:08
  • \$\begingroup\$ The condition was replaced with the real condition for the sake of simplicity and code that actually mattered. But I edited my post for those who think it's relevant. \$\endgroup\$
    – Quoter
    Mar 15, 2015 at 13:21
  • 2
    \$\begingroup\$ This isn't Stack Overflow. You're on Code Review - code always matters. \$\endgroup\$ Mar 15, 2015 at 13:58
  • 1
    \$\begingroup\$ Hm, sorry but I'm voting to close for now. I don't think the code you supplied works as intended and contains to much hypothetical stuff for us to do a sensible review. \$\endgroup\$
    – ChrisWue
    Mar 15, 2015 at 18:07

2 Answers 2

1
\$\begingroup\$

If your condition if false then none of your if's executes. If it's true then condition && !hasPassedFirstCheck is always false, so your code simplifies to this:

var cars = PredicateBuilder.False<Cars>();

if (condition)
{
    // I guess you have a typo here as both expressions are same
    cars = sf.ServiceTypes.Count < 2 ? cars.And(x => x.CarType == CarType.Truk) 
                                     : cars.And(x => x.CarType == CarType.Truk);

    cars = cars.Or(x => x.CarType == CarType.SUV ||
                        x.CarType == CarType.Limo ||
                        x.CarType == CarType.Race);
}

And further reduced, but less readable:

var cars = !condition ? PredicateBuilder.False<Cars>() :
                        (sf.ServiceTypes.Count < 2 ? 
                                cars.And(x => x.CarType == CarType.Truk) : 
                                cars.And(x => x.CarType == CarType.Truk))
                            .Or(x => x.CarType == CarType.SUV ||
                                     x.CarType == CarType.Limo ||
                                     x.CarType == CarType.Race);
\$\endgroup\$
1
  • \$\begingroup\$ The CarType selected is not known upfront. I've updated my question with the real code of condition. Didn't think it was relevant, thus removed for simplicity, but added again. \$\endgroup\$
    – Quoter
    Mar 15, 2015 at 13:49
1
\$\begingroup\$
  1. You seem to have a typo in your CarType enum, it should probably be Truck.
  2. cars is initially False hence you can get rid of the And conditions because they will never evaluate to true anyway. So for example this makes this whole block useless:

    if (condition)
    {
        cars = sf.ServiceTypes.Count < 2 
            ? cars .And(x => x.CarType== CarType.Truk) 
            : cars .And(x => x.CarType== CarType.Truk);
    
         hasPassedFirstCheck = true;
    }
    

Update: Let's walk through the code a bit.

This is how it starts:

var cars = PredicateBuilder.False<Cars>();
var hasPassedFirstCheck = false;

if (filterObject.CarTypes.Contains(CarTypes.Truck))
{
    if (sf.ServiceTypes.Count < 2 && !hasPassedFirstCheck)
        cars = cars.And(x => x.CarType== CarType.DJ);
    else
        cars = hasPassedFirstCheck
            ? cars.Or(x => x.CarType== CarType.DJ)
            : cars.And(x => x.CarType== CarType.DJ);

    hasPassedFirstCheck = true;
}

The initial predicate evaluates to false and hasPassedFirstCheck is false as well. Entering the first block this means !hasPassedFirstCheck is always true and can be omitted and hasPassedFirstCheck is always false hence the Or path is never taken. Given all of that the code boils down to:

if (filterObject.CarTypes.Contains(CarTypes.Truck))
{
    if (sf.ServiceTypes.Count < 2)
        cars = cars.And(x => x.CarType== CarType.DJ);
    else
        cars = cars.And(x => x.CarType== CarType.DJ);

    hasPassedFirstCheck = true;
}

Obviously the check if (sf.ServiceTypes.Count < 2) is redundant because you do the same thing in either case. So it can be further simplified to:

if (filterObject.CarTypes.Contains(CarTypes.Truck))
{
    cars = cars.And(x => x.CarType== CarType.DJ);

    hasPassedFirstCheck = true;
}

However the cars predicate is false at that point and false && Something evaluates to false. Hence the And condition has no effect on the result and the block can be simplified into:

if (filterObject.CarTypes.Contains(CarTypes.Truck))
{
    hasPassedFirstCheck = true;
}

Given all of this I'm not sure that

  1. The code you supplied is actually the code you use.
  2. That the code is actually doing what you expect it to do in the first place.
\$\endgroup\$
2
  • \$\begingroup\$ Yes the truk was a typo, but the 2 And was a result of resharper refactoring that piece of code. I have updated my question with the original code. \$\endgroup\$
    – Quoter
    Mar 15, 2015 at 13:50
  • \$\begingroup\$ @Quoter: Resharper did the refactoring the correct way. Given your code that's what you can simplify it to. \$\endgroup\$
    – ChrisWue
    Mar 15, 2015 at 17:32

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