In working with legacy code, I often come across the problem of having to refactor classes that contain static methods or are entirely static methods.
I talked about refactoring helper classes before, but this is slightly different.
In this case I want to talk about refactoring classes that you want to keep around, but have all or many static members. A good example of this is some kind of service class that returns data from the database.
It’s not always very clear whether that kind of class really is some sort of helper class. It is a bit of a judgment call. If you do find a helper class though, don’t just leave it there.
So, basically if you have determined the class you are working with is going to stay, but it does have static methods and you need to get rid of them because you are doing dependency injection or mocking, read on.
Defining the two approaches
What do I mean by step-wise refactoring?
Here is the basic outline of the first approach:
- Make the method you need to be non-static, non static.
- Add an interface with just that one method in it.
- Implement the interface.
- Change the references to that method to use the interface instead.
Let’s take a look at an example:
If we are interested in the GetLostOrders method, we can apply steps 1-3 to get:
Now we can go in and change references in our code for just that one method.
Now let’s look at the 2nd technique, wrapping and delegating. Here is the outline of the wrapping and delegating approach:
- Create a wrapper class that’s going to be used to wrap the static classes calls.
- Implement all the methods in the static class as non-static methods in the wrapper class. Each method just delegates to the static method in the static class.
- Create an interface which contains all the methods.
- Have the wrapper class implement the interface.
Here is an example of doing it this way, given the same original code as the first example:
The references to the LostOrderService will be refactored exactly the same as in the first example, so I won’t include it here.
You can see in this example, we didn’t touch LostOrderService itself. Except you probably want to put an Obsolete attribute on the class to tell users to not use this, but use the wrapper class instead.
Which is more bettah?
I’ve tended to use the wrapping and delegating approach in the past, but I am starting to think the step-wise approach is better for a few reasons.
- The step-wise approach is a bit more obvious to someone later using the class. When you wrap and delegate, someone has to know there is a wrapper class. With the step-wise approach, there is no choice.
- With the step-wise approach, you are actually getting rid of the bad and evil static methods. When you wrap and delegate, you are still leaving them there, just hiding them behind a wrapper.
To me, the wrapping approach feels more like I am working around things rather than cleaning them up. I also feel like someone can see what I started and pick it up from there step-by-step. Where with the wrapping approach, the mess may never get cleaned up, because there is a workaround.
Where wrap and delegate shines
There is a place that wrap and delegate wins hands down though.
If you don’t have control over the source code of the static class or static calls, you cannot do the step-wise approach.
The wrap and delegate approach can be a lifesaver when you are dealing with static references in your code to an external library that you cannot change. You can simply wrap the external library calls and instead reference the wrapper in your code. Now you can actually unit test that code.
Anytime you are using an external library, you should consider putting some kind of protective wrapper around it. You never know when you may want to replace it or upgrade the library. You don’t want to go hunting through all your code looking for references.
So, while either way will work, I prefer to use the step-wise method if I have access to the source code of the static class.
What do you think? Do you have any other solutions?