Filtering units by a dynamic Lambda expression

Question

I have a simple if statement which, based on user input, determines which expression is going to be utilized. It looks like it can be done better.

Func<Unit, bool> exp;
if (onlyChildren)
    exp = u => u.PhaseId == id;
else
    exp = u => u.PhaseId != id;

var unitGroups = _db.Units.Where(exp).GroupBy(m => m.UnitTemplate, (m, group) => new UnitTemplateZoneVm()
{
    Id = m.Id,
    Units = group.ToList()
}).ToList();

Answer 1

First of all, I support @EBrown about whitespace and braces.

Some other ideas depending on what you are doing:

• Use the conditional if, so you can logically see the if-else.
• Put logic into a method when you're using the same filter multiple times

Example:

...
.Where(u => (onlyChildren ? ChildFilter(u,id) : !ChildFilter(u,id))   
...

public static bool ChildFilter(Unit unit, int id)
{
    return unit.PhaseId == id;
}

In case the boolean value is not due to user input, but from different classes. You could consider:

• passing the func as a parameter, [optional] using enum object pattern
• passing the caller as a parameter which implements an interface with a filtering method

Clean code VS. Overkill

Answer 2

Don't omit braces. Ever. It's such a bad idea (I learned this the hard way).

As far as your lambda, your method is not necessarily a bad thing, though there are always other ways to write it.

var unitGroups = _db.Units
                    .Where(u => (onlyChildren && u.PhaseId == id) || (!onlyChildren && u.PhaseId != id))
                    .GroupBy(m => .UnitTemplate, (m, group) => new UnitTemplateZoneVm()
                    {
                        Id = m.Id,
                        Units = group.ToList()
                    })
                    .ToList();

Whitespace is free, use it. It makes it clearer as to what steps belong together.

I would personally prefer this method, as you can extract this out into LINQ-SQL style, instead of using the methods. Personally, I prefer the LINQ-SQL style rather than the methods, but the choice is up to you.

Answer 3

This is perfectly acceptable functional-style code. It would look prettier if you were using a functional language (and everything would be a function, not an expression).

It may not, however, scale nicely in an OO world, since it is still rather imperative. If you were to have to add more conditions to this code, etc., it might become unwieldy. At that point it would then make sense to break the different execution strategies into separate objects (maybe objects that inherit from an abstract base with 90% of code shared in a template method). You'll know when you get there.

For now, you should be good.