16
votes
\$\begingroup\$

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...

\$\endgroup\$
10
votes
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ @Steven Jeuris: Thanks, your answer really makes sense. \$\endgroup\$ – raven Apr 17 '11 at 19:32
  • 1
    \$\begingroup\$ @Steve Sorry, the second “disagree” should have been “agree”. \$\endgroup\$ – Konrad Rudolph Apr 18 '11 at 19:28
  • 2
    \$\begingroup\$ I completely agree with this answer. I don’t understand why it is not upvoted more. The other (more upvoted answer) is very blanket and doesn’t back itself up. This answer is much more thoughtful. \$\endgroup\$ – Timwi Apr 19 '11 at 20:13
  • 1
    \$\begingroup\$ How would you test this 50-line method? That, alone, is a reason to extract the functionality to another method/class. I really doubt this code would ever have been written test-first, and it is most likely difficult to test after-the-fact as well. \$\endgroup\$ – Jedidja Mar 2 '12 at 13:13
  • 1
    \$\begingroup\$ @StevenJeuris I think it would be interesting to see the both the code in the 50-line method, and the rest of the class that Method belongs too. I do have to agree we're all kind of making blanket statements without seeing the code. In general, though, when I've come across this type of code, it's often been refactorable. \$\endgroup\$ – Jedidja Mar 2 '12 at 13:21
13
votes
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ @Brian Reichle: Hmmm... thanks for your input. I don't want to discuss the optimal size of a function (be it 20, 50, 100 or 200 LOC). That's worth a question of its own. I was asking about the more general issue: is it good practice to use lambdas more than a few (maybe 4 or 5 at most) lines long? And your answer seems to be: "Yes, it is" :-) \$\endgroup\$ – raven Apr 16 '11 at 9:03
  • \$\begingroup\$ @Brian: How big is your screen? :P \$\endgroup\$ – Mehrdad Apr 18 '11 at 1:35
  • \$\begingroup\$ @Mehrdad: Arbitrarily small :). It's a "rule of thumb", it's not meant to be specific. the basic idea is that you want a "good" amount of context around the lambda expression visible at the same time as the expression itself. \$\endgroup\$ – Brian Reichle Apr 18 '11 at 3:02
  • \$\begingroup\$ “I think it should be a separate method rather than a lambda expression.” — Uhm, why? Do you have a reason for this viewpoint or are you just saying it because a lot of people say it? \$\endgroup\$ – Timwi Apr 19 '11 at 20:17
  • 2
    \$\begingroup\$ Testability is yet another reason... @Chris Phillips' answer below deserves upvotes too :) \$\endgroup\$ – Jedidja Mar 2 '12 at 13:14
6
votes
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ That's the best argument I've seen for not using long lambdas. Well done. \$\endgroup\$ – raven Aug 4 '11 at 8:24
  • 1
    \$\begingroup\$ Why hasn't this been upvoted more? :) If someone was writing this TDD style, I doubt a 50-line lambda inside a Thread would have ever happened. And, furthermore, even if you're writing tests after the fact, splitting out responsibility into another method (or class) is going to help testing immensely. \$\endgroup\$ – Jedidja Mar 2 '12 at 13:12
  • \$\begingroup\$ See my reply on Jedidja's comment. Nothing prevents your from TDD'ing the sh*t out of the methods used inside the 50-line lambda. "Anything else is specific to the context of the code running in the thread. So testing whether it works outside of a thread is a bit pointless." \$\endgroup\$ – Steven Jeuris May 17 '12 at 0:58
  • \$\begingroup\$ @StevenJeuris - it's not "testing whether it works outside of a thread", it's testing whether it works, period. If you have a 50-line method that means complexity, which means you need to test it, prove it correct, or live with the possibility of bugs. \$\endgroup\$ – Marcel Popescu Jan 10 '14 at 9:19
3
votes
\$\begingroup\$

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.

\$\endgroup\$
1
vote
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks for answering! The existing method (called 'Method' in my code) only creates a thread to do what the lambda does. So it could have the name you would assign to the lambda, making your point moot :-) \$\endgroup\$ – raven Apr 17 '11 at 11:09
  • \$\begingroup\$ -1 Sorry, but you make a poor argument. How is a method name more significant than a comment? I know the self-documenting code opinion, but "being more significant" is definitly not an advantage, it's rather the other way around. \$\endgroup\$ – Steven Jeuris Apr 22 '11 at 22:14
  • \$\begingroup\$ @StevenJeuris I am sorry but comments are often unupdated and unread. Function names are, on the contrary, always read, and must fit with what they do. Martin Fowler have wrote a wonderful book about those kind of things, you should read it. amazon.com/Refactoring-Improving-Design-Existing-Code/dp/… \$\endgroup\$ – Drahakar Apr 23 '11 at 2:54
  • \$\begingroup\$ That's what I meant when I said I know the self-documenting code opinion, and here is my opinion about the comments argument. Still have to read that book though. Perhaps it's just my poor perception of your word of choosing, but when I read "more significant" in that phrase I interpret it as "it tells more what it is doing". Which is not the case ... hence the downvote. \$\endgroup\$ – Steven Jeuris Apr 23 '11 at 6:34
1
vote
\$\begingroup\$

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.

\$\endgroup\$
1
vote
\$\begingroup\$

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.).

\$\endgroup\$
0
votes
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ I don’t get why programmers seem to be such fans of such blanket rules. “Lambdas should be no more than a single expression” — uhm, why? What sense does it make to say this outright without taking into account any specific circumstances, which are guaranteed to be widely different from case to case? \$\endgroup\$ – Timwi Apr 19 '11 at 20:16
  • \$\begingroup\$ @Timwi I grant that it’s a rough rule of thumb. But one that is greatly supported by C#’s syntax: for single-expression lambdas you don’t need parentheses. Once you need parentheses you lose all of the conciseness of lambdas. And consequently, also one of the advantages of lambdas over proper functions. Proper functions of course also have advantages over lambdas: for one, they have a proper name that gives some clue as to their function. So setting the break-even point of trade-offs at one expression seems very reasonable. \$\endgroup\$ – Konrad Rudolph Apr 20 '11 at 8:08
  • 1
    \$\begingroup\$ @Konrad: Interesting — so your brain has trouble with long methods but not with even longer classes with a plethora of single-use methods? Personally, my brain prefers a long method consisting of well-commented chunks (which you can just read from top to bottom) over the same code split into several disordered methods (where you have to keep jumping around). Of course, if the method has a lot of logic (ifs, loops, etc.) that’s a different matter — but the OP’s method only instantiates and starts a thread... \$\endgroup\$ – Timwi Apr 22 '11 at 14:40
  • 1
    \$\begingroup\$ Thank you for your answer, Konrad. Even if I don't fully agree with your view, I don't think you deserver those downvotes. Perhaps I don't fully understand this site. \$\endgroup\$ – raven Apr 22 '11 at 17:22
  • 1
    \$\begingroup\$ @Timwi “so your brain has trouble with long methods but not with even longer classes with a plethora of single-use methods?” Yes, absolutely. This is one of the very few established facts of the psychology of programming: methods that span more than a screen become incomprehensible (Code Complete, I believe). Note that I also agree with Steven’s answer. There’s always a trade-off involved. \$\endgroup\$ – Konrad Rudolph Apr 23 '11 at 13:18

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