lbsa71.net Just another WordPress weblog

Refactoring Tactics Primer

• Download RefactoringPrimer.zip - 26 Kb

Introduction

In this article I will show how to employ some relatively advanced code refactoring tactics to change a reasonably hairy piece of spaghetti code into something a bit more structured.

Basic familiarity with the JetBrains ReSharper tool is required.

Background

The code in this sample reflects an actual piece of code that a colleague of mine got handed to him. Amongst other abominations, it employed the 'transferring values in and out of Windows Forms by getting and setting public fields on an global static class' anti-pattern. He realized that the code actually introduced a bug (since the static fields would retain their values between subsequent calls) so I thought I'd show him how to wrestle out of some of the more nasty bits of the code. After doing that, I figured I'd write this article so that the world brain could benefit from it.

Instructions

Download the RefactoringPrimer.zip and extract and load the Visual Studio 2008 solution Seminar1.sln, make sure you have ReSharper handy, and follow the walkthrough below.

The solution is split into six steps, Step 1 being the original spaghetti code, and Step 6 the final result.

The Scenario is built around an application (Program) that calls up a (faked) Windows Form (FakeForm), and uses a static utility (Util) class to pass initial values and fetch returned data.

Each solution project corresponds to the starting point in each step, and builds a console project that when run tries to call up forms twice, each time emulating a user entering some data, and reports 'wrong: using tainted maxValue' or 'correct: using default maxValue'.

Points of Interest

This walkthrough is really only a primer to what can be done with taking a "automated refactoring tactics" approach; "tactics" referring to laying out a 'strategy', a chain of automated refactorings to get from a code state a to a code state b.

To create this particular set of refactoring tactics, I made extensive use of scratch refactoring, which is something I've found to be very useful.

Walkthrough

Step1 - Deciding what to break out

1. Manually create a public inner class called 'FormData' inside the Utils class.

       static class Utils
       {
           public class FormData
           {
           }  
   
           private const int DEFAULT_MAX_VALUE = 5;
           public static int MinValue = 1;
           public static int MaxValue = DEFAULT_MAX_VALUE;
           public static int MeanValue = 10;

2. Right-click on the MinValue member field, choose "Refactor", then "Move". Check static fields MinValue, MeanValue and MaxValue, and specify Step1.FormData as the target. Press "Next". After the move, note that DEFAULT_MAX_DATA is accessible, since FormData is an inner class. Also, inspect FakeForm and Program to see the updated references.
3. Run and show wrong result

From now on, I will not write out the 'right-click', "Refactor" or "Click Next" et c - if the istructions aren't obvious to you, I recommend you spend some time familiarize yourself with ReSharper. Either that, or place a request for clarification in the comments.

Step2 - Breaking out of the static cage

1. "Copy Type" Utils.FormData to InstanceFormData - it will also be an inner class.
2. Remove all static declarations on the copied type -this will become our 'new' FormData.

           public class InstanceFormData
           {
               public int MinValue = 1;
               public int MaxValue = DEFAULT_MAX_VALUE;
               public int MeanValue = 10;
           }

3. "Move" InstanceFormData to "Outer scope" giving it the new name "FormData"
4. The refactoring tool will complain that DEFAULT_MAX_VALUE will no longer be accessible. Turn DEFAULT_MAX_VALUE public and hit 'Refresh'
5. Change 'internal' on new FormData to "public" (or you will get a build error when we do the manual switch later)

       public class FormData
       {
           public int MinValue = 1;

6. Manually remove inner FormData type definition and replace with a static Singleton field initializer:

       static class Utils
       {
           public static FormData FormData = new FormData();
           public const int DEFAULT_MAX_VALUE = 5;  
   
           public static bool IsDefaultMaxValue( int val )
           {
               return val == DEFAULT_MAX_VALUE;
           }      
   
           public static bool TestMinGreaterThan3()
           {
               return FormData.MinValue > 3;
           }
       }      
   
       public class FormData
       {
           public int MinValue = 1;
           public int MaxValue = Utils.DEFAULT_MAX_VALUE;
           public int MeanValue = 10;
       }

7. Run and show wrong result - we're still running on the same instance.

Step3 - Passing the temporary singleton around

1. "Introduce field" m_formData on FakeForm, initialized in constructor.
2. "Introduce Parameter" formData on m_formData initializer.
3. Manually change FakeForm constructor member initializations to

   m_min = formData.MinValue;
   m_mean = formData.MeanValue;
   m_max = formData.MaxValue;

   (remember that the member fields are faking textboxes)

4. "Safe Delete" all unused (greyed out) constructor parameters
5. "Introduce Parameter" formData on TestMinGreaterThan3

       static class Utils
       {
           public static FormData FormData = new FormData();
           public const int DEFAULT_MAX_VALUE = 5;      
   
           public static bool IsDefaultMaxValue( int val )
           {
               return val == DEFAULT_MAX_VALUE;
           }      
   
           public static bool TestMinGreaterThan3(FormData formData)
           {
               return formData.MinValue > 3;
           }
       }

6. "Move" static method TestMinGreaterThan3 to FormData
7. "Make method non-static" on TestMinGreaterThan3

    public class FormData
    {
        public int MinValue = 1;
        public int MaxValue = Utils.DEFAULT_MAX_VALUE;
        public int MeanValue = 10;  

        public bool TestMinGreaterThan3()
        {
            return MinValue > 3;
        }
    }

Step4 - Getting rid of the temporary singleton

1. "Introduce variable" formData on first Utils.FormData in Run()

      FormData formData = Utils.FormData;  
   
      FakeForm a = new FakeForm(formData);

2. Manually Change Utils.FormData to new FormData();

      FormData formData = new FormData();  
   
      FakeForm a = new FakeForm(formData);

3. Manually add another initializer.'

       FormData formData = new FormData();  
   
       FakeForm a = new FakeForm(formData);      
   
       bool result = a.RunCase1(3, 4); // Setting min and max      
   
       formData = new FormData();      
   
       FakeForm b = new FakeForm(formData);

4. Run and revel in succesful result

Step5 - Some final cleanup

While we've fixed the main issue, there is still some simple refactoring that can be done to give that "cleaner, fresher feeling".

1. "Inline method" SetMeanValue() in FakeForm
2. "Rename" FakeForm method SetStatics() to SetFormData
3. "Move" FormData to its own file FormData.cs

Step6 - The final result

We have now moved from a spaghetti code mess to nice decoupled code, and hopefully you have gotten a glimpse of what chaining automated refactorings can do for your code.

If you've done this excercise and found it worthwhile, please consider giving me your feedback so I can make this article even better - also, illustrative screenshots taken along the way would be greatly appreciated!

Happy tinkering!