The Singleton Killer

We all hate singletons so here's another useful refactoring pattern: The Singleton Killer.

Here's a common snippet of code we all love to hate:

class Transfer
 def funds(from_number, to_number, amount)
   from = AccountRepository.instance.get(from_number)
   to = AccountRepository.instance.get(to_number)
   from.balance -= amount
   to.balance += amount
 end
end

So how do we kill this beast? Simply follow these idiot proof steps:

Step One: Introduce Field

Stop asking the singleton for it's reference and store your own by creating a field:

class Transfer
 @repository = AccountRepository.instance
 def funds(from_number, to_number, amount)
   from = repository.get(from_number)
   to = repository.get(to_number)
   ...
 end
end

Wow: it's starting to look like normal code!

Step Two: Initialize in constructor

Move the initialization to the constructor:

def initialize()
 @repository = AccountRepository.instance
end

This is good but wouldn't it be better if the dependancy could be injected?

Step Three: Chain constructor

In languages where dependency injection is useful you will need to chain the constructors:

public Transfer() : this(AccountRepository.instance)
public Transfer(AccountRepository repository)
{
 this.repository = repository
}

Now the AccountRepository can be injected which of course means we can mock it out!

Step Four: Extract interface (for statically typed languages)

Except statically languages: they'll need to extract an interface:

public interface IAccountRepository
{
 Account Get(string number);
}

public Transfer(IAccountRepository repository) ...

Good: now you can start mocking and testing.

Step Five: Introduce container

If you haven't already got a container then get one (else ensure you a wiring up your dependencies in one place). Then get that to wire up the Transfer object for you:

container.Add(AccountRepository)
container.Add(Transfer)
transfer = container.Get(Transfer)
transfer.funds("YOUACCNUM", "MYACCNUM", 50000)

Step Six: Remove singleton

Once a container is doing all your injection for you that singleton is a waste of space: get rid of him by removing that horrible instance accessor.

Choose your strategies with this refactoring wisely: if your objective is to get a class tested then first repeat steps one-four within the same class until it is clean of all singletons. This is a good exercise in it's own right - even if you're not going to move on to test or remove the singleton completely - as you start to move all the dependancies to the constructor you gain a better picture of the classes collaborators (and the quality of design). On the other hand if your objective is to remove the singleton repeat steps one-three on every usage you can find then finish up with steps four-six. Another technique is to keep pushing the useage of the singleton up through the code so each client then takes on responsibility for passing the singleton down when it creates an instance of the class. Like so:

  Transfer.new(AccountRepository.Instance)

Then repeat steps one-three on the class you've just pushed into, then push the singleton up again up and so on until you can't push it anymore. This can be an interesting exercise in itself as it starts to push out the paths through the code base as you move through the layers and may also give you a clear point where your container needs to be. Be warned though: it can get messy (though strangely satisfying)!

On larger, messier codebases none of these approaches may be realistic in which case opt for the guerilla approach and pick them off as you find them. Though I would encourage trying to at least clean out a whole class using steps one-three even if you won't end up testing it right away. With all your dependancies in one place you'll still have a better picture of the design.

Make me Whole

This is one of my favourite refactoring patterns and one I find I use a great deal so I thought I'd share it. The basic premise of this refactoring is to encapsulate logic into its own class using small, easy steps. The trick is each step should always leave the code in good working order i.e. the tests should always pass.

To help explain I'm going to walk through the refactoring by encapsulating some simple procedural logic and by making the concept explicit by giving it it's own class. Here's our starting point:

  def say_hello()
    if @friend == "Jeff" || @friend == "Robert" do
       put "Hello my good friend #friend"
    else
       put "Hello #friend"
    end
  end

The first thing we notice is there is some behaviour which depends on the state of friend (is the value "Jeff" or "Robert"). This is a good sign that there's a concept called Friend which needs encapsulating. So here's how:

Step One: Extract method

The first step is easy: extract the logic into it's own method like so:

  def say_hello()
    if is_good_friend do ...
  end

  def is_good_friend
    @friend == "Jeff" || @friend == "Robert"
  end

This is good, now the code is clearer to read and we've encapsulated the logic (albeit only in a method). That's already a big improvement.

Step Two: Introduce parameter

The problem is our method is dependent on its class for the @friend variable. We need to sever this connection so the method can stand on its own two feet. The simple way is to pass friend through as a parameter.

  def say_hello()
    if is_good_friend(friend) do ...
  end

  def is_good_friend(friend)
    friend == "Jeff" || friend == "Robert"
  end

That's better: is_good_friend is decoupled from the class. Its free!

We still need to get that method onto a new class. The problem is it can't be done in one easy non-breaking step. So we're going to have to take a tiny intermediate step to get there:

Step Three: Make method static

By making the method static it severs all connections to the original class: it is explicitly non-dependant on any instance variables the original class has.

  def say_hello()
    if self.is_good_friend(friend) do ...
  end

  def self.is_good_friend(friend) ...

Making the method static is also a good safety check to ensure that you've not got any references to the original class and it is properly isolated before moving it.

Step Four: Move method

Now the method is static move it to the new class:

  def say_hello()
    if Friend.is_good_friend(friend) do ...
  end

  class Friend
    def self.is_good_friend(friend) ...
  end

Excellent, we're really getting somewhere! Though now we've got a static method sitting on a helper class. Yuk: let's get that sorted before someone copies us and that nasty anti-pattern starts to proliferate through our code base:

Step Five: Create instance

This is where the real magic starts to happen: we create an instance of our new class and use it to store the value of friend:

  class Friend
    @value

    def self.is_good_friend(old_friend)
      friend = Friend.new(old_friend)
      friend.value == "Jeff" || friend.value == "Robert"
    end 
  end

Still, this is a bit ugly. What we really need to do is get the client to do the work of creating the instance.

Step Six: Introduce parameter

Rather than have our static method new up Friend get the client to do it and pass it across:

  def say_hello()
    if(Friend.is_good_friend(@friend, Friend.new(@friend)) ...
  end

  class Friend
   ...
    def self.is_good_friend(old_friend, friend)
      friend.value == "Jeff" || friend.value == "Robert"
    end
  end

Step Seven: Remove parameter

The old_friend parameter is redundant and ugly: let's get rid of it!

  def say_hello()
    if(Friend.is_good_friend(Friend.new(@friend)) ...
  end

  class Friend
    ...
    def self.is_good_friend(friend) ...
  end

Excellent, this is starting to look a lot better now. Though look at that repetition: the static method on Friend passes an instance of it's own class! Only one way to sort that out:

Step Eight: Make method non-static

Now the beauty of this is that if you are using a modern IDE it will work out that the method is being passed an instance of its own type and it will magically do this:

  def say_hello()
     if(Friend.new(@friend).is_good_friend) ...
  end

  class Friend
     def is_good_friend
         @value == "Jeff" || @value== "Robert" 
     end
  end

Brilliant: Friend is now nicely encapsulated and ready to grow into a responsible object with a bright future. From here we can start pushing more behaviour onto the object and eventually make @friend reference an instance of the Friend class.

Overall the pattern is very simple: extract the logic into its own method then work to detach it from the class by making in static, then simply move the static method and reattach it to the new class by returning it to an instance. It is also possible to handle more complex behaviour with several variables involved, simply pass them through as parameters and then push them onto the new instance.

Although this example is simple the pattern can be repeated again and again to build up classes into a number of methods (sometimes these classes can again be refactored into smaller classes still).

Your Feet's Too Big

Every piece of code written has a refactoring footprint which is essentially its proliferation through the code base and its level of difficulty to refactor. Some code can have a high proliferation but the refactoring may be simple, for example renaming a variable in modern IDE's is a simple matter regardless of the number of usages. On the other hand changing a variables return type or changing the signature of a method both create a large refactoring effort, especially if that requires knock-on refactorings of other classes and the greater the consumption of the code the greater the footprint. There are other things that can have an impact on the ease of refactoring: using libraries that create code that is difficult to refactor (for example having to place variable names etc. in strings), using external libraries directly without encapsulation, having similar routines across code which isn't encapsulated, the number of entry points to a piece of code (the number of different ways of achieving the same thing) all of these can make refactoring difficult and risk the introduction of bugs. Another area, much neglected when considering ease of refactoring, is test code which very often has a high consumption of production code (sometimes higher than the production code itself) and can make the simplest refactorings painful because large chunks of test code are dependent on your class.

TDD states that code that is easy to test is good code and the same is true for refactoring: writing code which is easier to refactor leads to cleaner code. Refactoring footprints have, in very real terms, an impact on productivity and therefore managing them is quite important. There are various techniques - most of them basic OOP principles - that can help keep the refactoring footprint down. However, due to the natural evolution of code, this can be quite hard to keep track of and unless you have an in-depth knowledge of the codebase - which is increasingly unlikely the bigger and older the codebase is - it can be difficult to 'see' a footprint.

I think it would be interesting to explore the use of metrics to measure refactoring footprints. These metrics would give a very interesting and valuable insight into the codebase and its quality. The interesting thing is that unlike many other code metrics refactoring footprints need to take into account test code as well. From a very basic level counting the number of usages across both production and test code can give a rudimentary indication to a footprint. More sophisticated metrics would measure the impact of a refactoring (are there knock-on refactorings). Different types of refactoring would also require different metrics and some refactorings may be uncommon in certain codebases or may have a degree of acceptable pain (package refactorings for example). As with most metrics they will be open to a degree of interpretation based on codebase and class but none the less the metrics should be useful - especially in recommending refactorings!

I haven't done much research on this so I do not know if any tools already exist to calculate such metrics or even what metrics they provide, though I am sure there will be. I shall try and track some down and have a play to see what sort of data they provide and establish what sort of insight they can give into a codebase.