current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

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();
share|improve this question

3 Answers 3

up vote 3 down vote accepted

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

share|improve this answer
    
It's an interesting idea, but I think I prefer the explicit Func, only perhaps, with a better name. –  RubberDuck 21 hours ago
    
I really like this solution! Can make code much cleaner and easier to read! –  Vadims Samsinovs 13 hours ago
up vote 5 down vote

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.

share|improve this answer
up vote 1 down vote

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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