Should a piece of code only ever called once be a separate method?

Question

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);
}

Answer 1

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

Answer 2

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.

Answer 3

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.

Answer 4

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);
}