Advertisements
Posted by: jsonmez | September 17, 2010

Refactoring Step-Wise vs Wrapping and Delegating

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:

  1. Make the method you need to be non-static, non static.
  2. Add an interface with just that one method in it.
  3. Implement the interface.
  4. Change the references to that method to use the interface instead.

Let’s take a look at an example:

public static class LostOrderService
{
    public static IEnumerable<Orders> GetLostOrders(int customerId)
    {        
    }

    public static IEnumerable<Customer> GetCustomerWithLostOrders()
    {
    }
}

 

If we are interested in the GetLostOrders method, we can apply steps 1-3 to get:

public interface ILostOrderService
{
    IEnumerable<Orders> GetLostOrders(int customerId);
}
    
public class LostOrderService : ILostOrderService
{
    public IEnumerable<Orders> GetLostOrders(int customerId)
    {        
    }

    [Obsolete("If you touch this refactor to make it not static. 
                      Be a man, do the right thing.")] 
    public static IEnumerable<Customer> GetCustomerWithLostOrders()
    {
    }
}

 

Now we can go in and change references in our code for just that one method.

public void ProcessLostOrders()
{
    var orders = LostOrderService.GetLostOrders(_customerId);
    foreach(var order on orders)
    {
        ReprocessOrder();
    }

}

// Refactored becomes:

public void ProcessLostOrders(ILostOrderService lostOrderService)
{
    var orders = lostOrderService.GetLostOrders(_customerId);
    foreach(var order on orders)
    {
        ReprocessOrder();
    }

}

 

Now let’s look at the 2nd technique, wrapping and delegating.  Here is the outline of the wrapping and delegating approach:

  1. Create a wrapper class that’s going to be used to wrap the static classes calls.
  2. 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.
  3. Create an interface which contains all the methods.
  4. 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:

public interface ILostOrderService
{
    IEnumerable<Orders> GetLostOrders(int customerId);
    IEnumerable<Customer> GetCustomerWithLostOrders()
}

public class LostOrderServiceWrapper : ILostOrderService
{
    public IEnumerable<Orders> GetLostOrders(int customerId)
    {      
        LostOrderService.GetLostOrders(customerId);
    }

    public IEnumerable<Customer> GetCustomerWithLostOrders()
    {
        LostOrderService.GetCustomerWithLostOrders();
    }
}

 

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 can not 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?

As always, you can subscribe to this RSS feed to follow my posts on Making the Complex Simple.  Feel free to check out ElegantCode.com where I post about the topic of writing elegant code about once a week.  Also, you can follow me on twitter here.
Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Categories

%d bloggers like this: