I have been reviewing the code of a new co-worker and, although in general I think it's OK, there's a thing that causes me mixed feelings:
private void Method(...)
{
Thread t = new Thread(() =>
{
// Some ~50 lines of code here
});
t.Start();
}
This is no big deal, but I'm curious about what you guys think of it. Is this exactly what lambdas are for? Or just the opposite? I try to avoid any lambda which doesn't fit on a couple lines at most...
In my opinion no preferable answer can be given without seeing the exact code. I don't like splitting code into smaller methods just for the sake of making methods smaller, as opposed to Brian Reichle's answer (and many other people that follow this approach).
If the code of the thread is and can only be used locally, I would leave it in there regardless of the LOC. Using a lambda has the advantage that you don't 'pollute' your class with an extra private method which is only called from one location. This provides for better encapsulation, which is a core principle of OO which makes it shine.
If the code of the thread were to be reuseable, I would rather think about splitting the behavior in a separate class than in a separate method, unless the code only makes sense in the original class.
Also as a sidenote; I realize you probably just named your code as to get the idea across, but I'd make sure the name of Method would indicate it only starts a certain action, but doesn't finish it. E.g. StartSomeMethod.
I think it should be a separate method rather than a lambda expression. If the method is as large as 50 lines then you should seriously think about breaking it up anyway, being an anonymous method only compounds the issue. It may be better to extract portions of the method instead, but that is a decision that you will need to make based on the actual code in the method.
My general rule-of-thumb is that if an anonymous method spans more than half the screens height then it's too big and should be a separate named method.
Update in response to Timwi's comments:
When a method is well named, the name helps to describe what the method is intended to do, both where it is used and where it is defined. Anonymous methods don't get this, and so to understand it you need to rely on the context more. This is fine when if the anonymous method is simple but if its large and/or there is deep nesting involved (within or containing various control structures) the behavior can become much less obvious.
The "exact" example used by the OP is not too bad because the containing method is very simple, but I don't see any real benefit (for either performance or readability) and so would still prefer to have it as a separate method.
Of-course, "readability" is a bit subjective. Most people will agree on the extreme cases but the edge cases are probably better discussed with, and a consensus drawn from those actually responsible for maintaining it.
Setting aside the debate of short vs. long lambdas for a moment, something I don't see mentioned in any of the other answers yet is testability.
If you put the thread body in a separate method, then you can test it's logic separately from the threading logic. This avoids all the difficulties and pitfalls of trying to unit test threaded code (starting, stopping, waiting, etc).
I find that this applies to most cases where lambdas are used. If it makes sense to test the code separately (and if its more than line or two, it probably does), then it is much easier to test properly using a separate method than a lambda inline.
It depends :-)
ignoring the splitting things into smaller methods debate... the code is already in a method. The part to do with threading is a tiny amount of the method. The lambda is just a construct to make the code multithreaded. If that was the intention of the method. Then thats ok.
It comes down to purity vs utility. Its easy to see this from the perspective of 'lambdas' and have a rule that lambdas should be short and to the point. But in this case its more using the utility of lambdas to wrap threading around a chunk of code.
(for the sake of this question, I'm ignoring if this is a good approach to threading... another example could be that this is a transaction context.... or some other wrapping context ).
So there is utility in using lambdas to wrap contexts around chunks of code without having to break them into a separate method. But as long as its not an excuse for badly factored code and it seems to make sense given the context of what you are doing.
I think that a method this long really deserve a name. Even if it is only used once, a name would be a good way to describe what is this lambda doing and it would be way more significant than a comment or something like this.
With a proper comment you create a "local function", so you don't create new global names unnecesarily. IMO it is a good thing somewhat similar to using local variables vs global ones.
Keeping it local is good. However for clarity you may want to assign the lambda expression to a Func or Action variable, and then do your threading code. This would help in making the threading code easier to understand on sight without mixing in the details of the actual code that is being threaded. In other languages it is not uncommon to declare the method to be threaded inside of the method that is calling it (javascript, ada, f#, etc.).
My rule of thumb for lambdas is: the should be no more than a single expression. The expression itself may be arbitrarily complex (within reasons) but as soon as I need more than one expression (or one statement, for a statement lambda) I use a separate method instead.
This isn’t an arbitrary rule either: in C# (and several other languages) there’s special syntax support for single-expression lambdas. As soon as you need more than one expression the conciseness of lambdas is lost, and you end up with a nameless function body.
I prefer proper functions then since the name of a function itself is very useful to identify the function’s task: this is very much in the interest of self-documenting code.
There’s one exception to the rule: when I need to access a comparatively large number of local variables (say, more than two) and would need to create private variables for those then the lambda may occasionally become longer than a single statement.
