When does a lambda in an extension method do too much?

Question

I realize this is partially subjective, but I'm generally curious as to community opinion and have not been able to successfully find an existing question that tackles this issue.

I am in a somewhat religious debate with a fellow coworker about a particular Select statement in a L2EF query.

.Select(r =>
{
    r.foo.Bar = r.bar;
    r.foo.Bar.BarType = r.Alpha;
    if (r.barAddress != null)
    {
        r.foo.Bar.Address = r.barAddress;
        r.foo.Bar.Address.State = r.BarState;
    }
    if (r.baz != null)
    {
        r.foo.Bar.Baz = r.baz;
        if (r.bazAddress != null)
        {
            r.foo.Bar.Baz.Address = r.bazAddress;
            r.foo.Bar.Baz.Address.State = r.BazState;
        }
    }
    return r.foo;
})

Caveats:

• This is Linq-to-Entities
• This is after the work in the DB as been performed and returned
• The input parameter r is anonymous

Personally, I'm of the opinion that (a) the select clause should not be altering values, it should merely project. His counter argument is that he's not altering anything, he's just making sure everything is properly initialized as a result of the DB query. Secondly, I think once he starts getting into full code blocks and return statements, it's time to define a method or even a Func<T, U> and not do all of this inline. The complicator here is, again, the input is anonymous, so a type would need to be defined. But nevertheless, we are still debating the general point if not the specific.

So, when does a lambda expression do too much? Where do you draw the fuzzy line in the sand?

Answer 1

My first instinct is to agree with you, primarily on the matter of size and complexity.

However, it is used in a context where it will be (or sometimes be) executed as something other than .NET code (particularly if it is turned into part of a SQL query), I'll become a lot more tolerant of it.

So, that's where I draw the fuzzy line, and also why I move it again :)

Answer 2

I also agree a Select() should used for projection. I'd rather see the use of the "let" keyword to do the checking in advance so that that projection in the Select() could be kept clean. This will enable the Select() to refer to the variable(s) set using the "let".

Answer 3

I don't think that's a long lambda expression, personally. I think you'll see a lot more complex, and nested lambdas in the future. Especially with something like Rx.

As for state changes ... well, here he is just initializing values. I'd only be concerned if it was assigning state to some variable outside the lambda, but this is all initialization of r, so it seems fine to me.

Answer 4

I had to stare at this for awhile before I saw what is bugging me.

1. This needs a refactor.

2. The fact it took me that long to read a lambda's intent.

I'm not sure if I can take a side on your definition of the Select's job, but I agree that the shorter you can keep a lambda the better. Break it out for re-use if the initialization after the dB fetch is needed so badly.

Answer 5

You have to factor in your other alternatives... is there a better place to put that logic. Either way, you're looping through each entity in LINQ, or in a foreach loop, to do that extra work... Unless you want to refactor it into your own extension method, I'd say leave it if it works.

Visually, it's not that messy, so that's a plus. Or, you can see if the let keyword (basically a subquery) buys you anything else.

Answer 6

Get your coworker and yourself a copy of Robert C. Martin's "Clean code" book, and compare "Uncle Bob's" coding style to your Linq statement. You will get plenty of ideas on how to refactor it.

Answer 7

The length of the lambda doesn't bother me at all. But I would agree with the feeling of dirt when you assign a value within your select statement. I would factor the initialization stuff into a statement just below the select statement.