Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm curious when a piece of code should be its own method or just left alone. This came up when I was creating a "work item" from the thread pool. When passing the argument to the "WaitCallback" object, I would argue that if the code that will run will ONLY run when this thread is being run, then it's OK to wrap the code inside of an Action delegate. If other places need to run this same code, then I understand separating it into a new method.

So, for example, what's better practice?

This --

ThreadPool.QueueUserWorkItem(new WaitCallback((state) =>
{
    Contract.SaveContract(Customer);
     pushResultNotification = DoPushContact();
      MyDispatcher.BeginInvoke((Action)(() => actionNotification.CompleteAction()), null);
}));

Or this --

ThreadPool.QueueUserWorkItem(new WaitCallback(DoStuff));

private void DoStuff(object state)
{
    Contract.SaveContract(Customer);
    pushResultNotification = DoPushContactToGp();
    MyDispatcher.BeginInvoke((Action)(() => actionNotification.CompleteAction()), null);
}
share|improve this question
1  
My experience has been that things that are only used once are almost always only used once for now. –  corsiKa Oct 25 '12 at 17:33

4 Answers 4

I think the answer here is no, it shouldn't be a separate method, as long as the piece of code in question is short. That way, when you read the code, you immediately see what the code does, you don't have to go elsewhere for that. But if the piece of code was too long, it would make the whole method less readable, so in that case, I think you should put it into a separate method.

Few more notes:

You don't need the new WaitCallback() in either case, the compiler can create the delegate automatically.

Also, it might make sense to use Task instead of ThreadPool, assuming you're on .Net 4+.

share|improve this answer

A "process" as defined for this answer is a distinct logical unit of functionality. Generally, a method/function should do one thing. Separating processes into their own methods makes your code more readable, especially for complex code bases. Separating processes into their own methods makes your code more maintainable. Since "simple" methods often evolve into complex ones, and software developers tend to do less refactoring than they should, it is often a better strategy to think long term early and put those "one off" processes into their own methods to begin with. They might not be "one off" for long.

There may come a time when multiple methods use the same process. By having a process contained within its own method, you can more easily maintain your code and simply call that method. If something about that process changes, you only need to change it in one place - the method where you so smartly put it when you wrote it.

So, to make a long story short, it may seem like overkill to move your process to its own method now, but in the long run, it is probably a better strategy for the readability and maintainability of your code. Yes, it is as much an art as it is a science. You have to decide whether or not a segment of code is its own process (as I defined earlier), or merely part of a different process. That decision should tell you whether or not it goes in its own method.

share|improve this answer

I believe that option one is the most correct option and I say that because limiting the number of threads that have availability to a specific piece of code built to run on a background thread will keep you from seeing locking and race conditions in production that you wouldn't see in development.

Furthermore, this code is clearly calling out to some other methods as well so containing the point at which it runs would be of interest.

share|improve this answer
2  
Thread safety is always an issue with multithreaded code, but I don't think using lambda will help you with that. If the code is not thread safe, you can get into problems just by calling the method containing QueueUserWorkItem twice from the same thread. –  svick Oct 24 '12 at 19:13
    
@svick, +1 as that is quiet correct. What I maybe should have stated is it's more like "out of sight out of mind" and so the chances of developers abusing it is smaller. It seems that sometimes if you just make it hard for developers to be bad they won't as frequently. –  Michael Perrenoud Oct 25 '12 at 11:24

Either way works and is good. Keep in mind though, that reducing duplication is not the only reason to create a method. I often extract a method that is only used once in order to give it a descriptive name that increases the readability and intentionality of the code.

I probably would not add a method to call it "DoStuff", but maybe something like this would be helpful to future maintainers of the code:

ThreadPool.QueueUserWorkItem(new WaitCallback(SaveCustomerAndFireNotifications));

private void SaveCustomerAndFireNotifications(object state)
{
    Contract.SaveContract(Customer);
    pushResultNotification = DoPushContactToGp();
    MyDispatcher.BeginInvoke((Action)(() => actionNotification.CompleteAction()), null);
}
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.