Refactoring large method in .NET

Question

Hi,

If I have a large .NET method, I want to know if it is good practice to split it up into multiple methods or not. My concerns are: 1 - Is there any point creating a method if it will only be called once?
2 - If I'm calling a private method from within my class, I try not to use instance variables. Is this good or bad practice? For example:

var a, b;
public void TestMe
{
   FirstTest(a,b);
}
private void FirstTest(paramA, paramB)
{
         //code here with passed in parameters
}

//instead of:
private void FirstTest()
{
      //code here with instance variables a,b
}  

edit: replace global with instance

Answer 1

1. If a piece of code is only going to be called once, there is most likely not a reason to create the method in the first place. HOWEVER, with that said, if it is a private method and it is being utilized as a separate piece of functionality (a separate concern), then it could be a good idea to split it out. You would want to do this to avoid having a single enormous method. An example would be, if I only do a search once in a piece of code, I still will want to split that search out because search is a separate piece of functionality. (This is a very poor example. If someone has a better one, please feel free to edit.

2. A variable isn't TRULY global unless it is accessible across the entire system. With that said, I would highly recommend avoiding global variables because it makes it very difficult do debug and to work through code because the variables have an extremely large scope. Instead, I would focus on having more localized variables and passing the variables that a function/method may need and returning data that the caller function expects. This way, you keep your data more localized and, ultimately, the code is easier to read and manage. Doing this is not particular to private or public methods.

Answer 2

With respect to your first question, reducing complexity in a method is sufficient reason to break a large method into small ones. Smaller methods are much easier to comprehend than a single large method. If done properly, by breaking the method up into logically consistent units of work, many small methods are preferrable to a single large method in almost all cases. The only conditions where it might not be is if it impacts your performance to the point where it is no longer acceptable from that perspective.

With respect to the second question, as @Jason points out, you are using instance variables not globals within your class. I would say which practice is preferred depends on the context of the method. Certainly if the method can be reused in many contexts, only some of which operate on instance variables, it ought to be parameterized. If you are only ever using the instance variables, you might choose not to have the parameters, and refactor as needed later for usability.

In C#, I would also prefer using instance properties over fields, to further decouple the property from the method using it.

Answer 3

1 - Is there any point creating a method if it will only be called once?

Yes, there are many reasons to do this. Readability is perhaps the most important. If you can make a method more readable and maintainable by breaking it apart, then by all means do so.

In my experience with refactoring legacy code where a method is way too long, there are little pieces of code that appear over and over again. These are usually the best places to look for refactoring opportunities. Creating separate methods for those pieces can greatly decrease a method's length, and, thereby, greating increase its readability.

2 - If I'm calling a private method from within my class, I try not to use instance variables. Is this good or bad practice?

Usually, the smaller you can make a variable's scope the better. Like you, I tend to use parameters whenever possible. If a method has references only to its own parameters, then it becomes much easier to reason about the method, verify its correctness, and use it correctly. (And if a method can do this and not modify any state, then that buys you a lot of maintenance benefits.)

If the purpose of a method is best served by manipulating an object's fields, then that is perfectly acceptable, and in many cases, unaviodable. But, as you indicate, this is especially true with public methods. When refactoring a large method into smaller ones, I will rarely, if ever, access member fields directly in the new methods. This is mainly just to make it easer to reason about the program's behavior.

When refactoring in this manner, make sure you mark the new methods as static if they don't access any fields. This will make the intent explicit.

Answer 4

To quote from Clean Code (an excellent book), a method should do one thing and one thing only. If you're doing multiple things in a method, then it probably means that you need to extract that logic into a separate method. It makes it a lot easier to follow the code as well. So I would say

Question 1: Yes break it up (separation of concern).

Question 2: In general I think it is a bad practice to have global variables unless you absolutely need to. As far as the general question of using the private property (instance variable) by itself over the public getter, I don't see any advantage by using the getter. For example, consider:

function someFunc() {
  anotherFunc(this.a, this.b);
}

function anotherFunc(int paramA, int paramB) {
  //do stuff with paramA and paramB
}

versus

function someFunc() {
  anotherFunc();
}

function anotherFunc() {
  //do stuff with this.a and this.b
}

At some point you'd have to refer to the instance variable by using this. So I don't see an advantage. After all, you are inside your class, so you have complete access to private members.