Dave's Blog

Refactoring legacy code: a case study.


April 24, 2019

Often refactoring examples are fairly straightforward, leaving you wondering what to do with the difficult cases of legacy code.

This process took me some ten commits and a couple of hours but it's worth looking at the steps to see a more complex example.

Starting point

We have a legacy class which downloads a file from Sharepoint.

There are no tests which is something I wanted to remedy. To do that we need to add interfaces around the Sharepoint objects as they are near-enough impossible to mock having constructors which take Sharepoint context objects with constructors like:

    public Folder(ClientRuntimeContext context, ObjectPath objectPath);

where ClientRuntimeContext is :

    protected ClientRuntimeContext(string webFullUrl);

and so on, which means that the only (or at least one) sensible option is to wrap this class (in fact a number of classes) in Adapter pattern implementations.

In full, the starting point for this exercise:

    private static void DownloadFile(ClientContext clientContext, File file, string fileName)
    {
      var maxAttempts = 10;
      for (var attemptNumber = 1; attemptNumber < maxAttempts + 1; attemptNumber++)
      {
        try
        {
          using (var fileStream = System.IO.File.Create(fileName))
          {
            clientContext.Load(file);
            var stream = file.OpenBinaryStream();

            clientContext.ExecuteQueryAsync().Wait();

            stream.Value.CopyTo(fileStream);
          }

          attemptNumber = maxAttempts + 1;

        }
        catch (AggregateException ex) when (ex.Message.Contains("(503) Service Unavailable"))
        {
          OutputWriter.Write(
            $"   >> Getting file {fileName}: 503 returned. Waiting before call number {attemptNumber + 1} of {maxAttempts} attempts.");
          Thread.Sleep(350 + 200 * attemptNumber);
        }
      }
    }

OK, so here is what I did, with at each step the commit message I used.

Commit 1 : Using R# refactored the to-be-wrapped code into a new method

Using Resharper I extracted out one of the method calls that use an object that I will need to mock to add some tests. Purely using Resharper here, as I do at almost every stage until I am able to write some tests.

refSp c1

Commit 2 : Added an interface for the refactored method and a wrapper class to apply the Adapter pattern to (but not used yet)

I added a class which implements the Adapter pattern to wrap the problematic ClientContext. I then extracted an interface for it so that I can mock it later on.

I also moved the call to LoadSharepointFile as it does not use the fileStream object so does not need to be in the using statement.

refSp c2anon

Commit 3 : Wrap the concrete Sharepoint object in the new adapter

I use the new ClientContextWrapper (adapter) to wrap the clientContext and then update LoadSharepointFile to accept an object which implements the new IClientContext interface I added in the last commit.

Ideally the tests would come first, to protect the refactoring. In reality the code cannot easily support this, which is why I had to follow this process. However I can test empirically that the code is not being broken by these changes as I can run the application to assert that the files are still being downloaded as ever they were.

That's not ideal, I agree, but to get the code into a testable state so that it is protected from changes sometimes you need to be pragmatic about how you get it there.

refSp c3

Commit 4 : Remove the redundant method and call the interface method

Now I call the method on the extracted interface (implemented through the new ClientContextWrapper).

refSp c4

Commit 5 : Add a second method to the interface and move the wrapper creation up so that the method to be tested does not rely on the concrete Sharepoint type

DownloadFile method is now accepting an IClientContext, taking it closer to being testable.

refSp c5

Commit 6 : Move the instantation of the FileStream to where it is needed

Here I limit the scope of the using block to what is necessary. I think I could have used Resharper but some times you can just eyeball things. Yes, that's OK. The commit is small enough to be able to look back and think "Yeah, that was fine" .

refSp c6

Commit 7 : Extract CreateFile into a new class; extract interface and use this in the code. DownloadFile has 4 params but we can live with that for the moment

Moving a bit quicker in this commit. I extract a new method, CreateFileFromSharepointClientResult, and then extract that into a new class, FileCreator, and then extract an interface for it, IFileCreator.

refSp c7p0

In the calling method, create a FileCreator, which DownloadFile will accept as an IFileCreator. We can move these instantiations up the chain later but for just now this is fine and keeps the commits simple.

refSp c7p1

Commit 8 : Extracted ISharepointFile which is used in a call to IClientContext but there is still a dependency on ClientContext which we cannot instantiate in the tests. Hmm

Similar to commit 7, I extracted an interface for a new adapter around the Sharepoint File object.

refSp c8p0

In the calling method, create a SharepointFileAdapter, as DownloadFile will accept as an ISharepointFile now.

refSp c8p1

Commit 9 : Updated the wrapper to use its stored ClientContext so that the interface can lose its dependency on the Sharepoint ClientObject type in favour of an ISharepointFile which can be mocked

IClientContext now loads a ISharepointFile, breaking the dependency on a Sharepoint ClientObject.

refSp c9p0

In DownloadFile I can now pass in the ISharepointFile, tidying this up.

refSp c9p1

Commit 10 : Quick tidyup

refSp c10

Now, finally, with the removal of the Load method from IClientContext, I have removed the dependencies on the Sharepoint types, in favour of interfaces.

So, at last, I can write some tests to protect this code.

Commit 11 : Added tests for DownloadFile and its collaborators

Firstly a test for the happy path, when Sharepoint is playing nicely.

refSp c11p0

Experience has shown us that it can return a 503 so there is a retry mechanism in place. Now I test the case when a 503 is thrown in the first Sharepoint call.

refSp c11p1

Which uses these helper methods:

refSp c11p2

Now I test the case when a 503 is thrown in the second Sharepoint call.

refSp c11p3

Tests, at last

There are a few more commits (which I've omitted here as I think that this is long enough already) where the code is tidied up further, now that I have the security of the unit tests in place: renamed, reduce the number of parms to a method, move the classes & interfaces into their own files.

The code is not yet perfect as you can see by the complexity of the unit tests: there is still a lot going on, but that's a matter for another day.

I hope that this gives somebody a helpful view point of taking some slightly messy code whic some awkward dependencies and shows how it can be refactored safely (almost entirely using Resharper to refactor) into a state where it can be protected by unit tests.