“Refactoring” Notes

July 8, 2010 — Keith Dahlby

I’m not going to bother with a review of Martin Fowler‘s Refactoring: Improving the Design of Existing Code. It’s good enough that its catalog, available in expanded form online, now provides the definitive vocabulary shared by dozens of refactoring tools across nearly every major development platform. Though I was already familiar with the majority of the catalog, I thought it would be worth reading the other chapters, the notes from which you will find below with my additions indicated with emphasis. I’ve also included thoughts on various entries in the catalog.

Principles in Refactoring

  • 58: You don’t decide to refactor, you refactor because you want to do something else, and refactoring helps you do that other thing.
  • 62: Beck – Maintaining the current behavior of the system, how can you make your system more valuable, either by increasing its quality, or by reducing its cost. … Now you have a more valuable program because it has qualities that we will appreciate tomorrow. … There is a second, rarer refactoring game. Identify indirection that isn’t paying for itself and take it out.
  • 62: Problems with Refactoring
    • Don’t know limitations
    • Is refactoring because a tool tells you to a bad reason?
  • 65: Modify your code ownership policies to smooth refactoring.
  • 66: Code has to work before you refactor.
  • Ward Cunningham: Unfinished refactoring = technical debt.
  • 68: You still think about potential changes, you still consider flexible solutions. But instead of implementing these flexible solutions, you ask yours, “How difficult is it going to be to refactor a simple solution into the flexible solution?” If, as happens most of the time, the answer is “pretty easy,” then you just implement the simple solution.
  • 70: Changes that improve performance usually make the program harder to work with.
  • If you optimize all code equally, you end up with 90 percent of the optimizations wasted, because you are optimizing code that isn’t run much.

Bad Smells in Code

An expanded catalog of code smells is available online.

  1. Duplicate Code
  2. Long Method
  3. Large Class
  4. Long Parameter List
  5. Divergent Change
    • One class that suffers many kinds of changes
  6. Shotgun Surgery
    • One change that alters many classes
  7. Feature Envy
  8. Data Clumps
  9. Primitive Obsession
  10. Switch Statements
  11. Parallel Inheritance Hierarchies
  12. Lazy Class
  13. Speculative Generality
  14. Temporary Field
  15. Message Chains
    • a.k.a. Law of Demeter
  16. Middle Man
  17. Inappropriate Intimacy
    • Circular Reference => Change Bidirectional Association to Unidirectional
  18. Alternative Classes with Different Interfaces
  19. Incomplete Library Class
    • Introduce Foreign Method => Extension Methods
  20. Data Class
    • Favor setters and encapsulated collections
    • Very OO-centric – FP would argue that immutable data classes are preferred
    • Hide Method => Immutability
    • 87: “Data classes are like children. They are okay as a starting point, but to participate as a grownup object, they need to take some responsibility.”
  21. Refused Bequest
    • Smell is stronger if the subclass is reusing behavior but does not want to support the interface of the superclass. (NotSupportedException)
    • Apply Replace Inheritance with Delegation
  22. Comments
    • 88: When you feel the need to write a comment, first try to refactor the code so that any comment becomes superfluous.

Building Tests

  • Tests should be fully automatic and self-checking.
  • Interesting that a distinction is drawn between unit and functional tests, but there’s no mention of integration tests as middle ground
  • 97: “When you gget a bug report, start by writing a unit test that exposes the bug.”
    • This is my #1 use case for test-first
  • 98: “The key is to test the areas that you are most worried about going wrong.”
  • 101: “Don’t let the fear that testing can’t catch all bugs stop you from writing the tests that will catch most bugs.”

If this is the first you’ve read of unit testing, check out a book dedicated to the subject like Pragmatic Unit Testing.

Catalog Notes

  • Composing Methods
    • Replace Method with Method Object
      local variables => method object fields to facilitate extracting methods
  • Moving Features Between Objects
    • Introduce Foreign Method = Extension Methods
    • Introduce Local Extension = Subclass or Proxy
  • Organizing Data
    • Replace Data Value with Object
      replace primitive with a type that means something (decimal => Money)
    • Replace Type Code with State/Strategy
      inheritance is often abused, but this is one of its best use cases
  • Simplifying Conditional Expressions
    • Introduce Null Object
      OO approach to avoiding null checks, also not to be abused
  • Making Method Calls Simpler
    • Separate Query from Modifier
      mutation and query operations don’t mix
    • Replace Parameter with Method
      this rarely applies to new code, but rather is found in methods that have evolved over time
    • Hide Method
      • Related: Remove from Interface — useful to let a method “hide in public” for easier testing without cluttering up its expected usage as implementer of an interface
    • Replace Error Code with Exception
      if a circumstance is truly exceptional, exceptions are often better than error codes…
    • Replace Exception with Test
      …but if it’s not exceptional, don’t incur the overhead; for example, return a Null Object
  • Dealing with Generalization
    • Extract Subclass, Superclass and Interface
    • Collapse Hierarchy
    • Form Template Method
    • Replace Inheritance with Delegation
    • Replace Delegation with Inheritance

Big Refactorings

  • 359: “Do as much as you need to achieve your real task. You can always come back tomorrow.”
  • 360: “You refactor not because it is fun but because there are things you expect to be able to do with your programs if you refactor that you can’t do if you don’t refactor.”
    • I need to remember this!!

These are hard to identify, but provide the biggest return on investment. Think of them as high-level goals accomplished through low-level changes from the above catalog.

  • Tease Apart Inheritance
  • Convert Procedural Design to Objects
    • Also, Convert to Functions
  • Separate Domain from Presentation
    • MVP, MVC, MVVM…use something!
  • Extract Hierarchy
Advertisement
Posted in Books, General. Tags: . Comments Off on “Refactoring” Notes

Refactoring with Iterators: Prime Factors

September 30, 2009 — Keith Dahlby

Andrew Woodward recently posted a comparison of his test-driven Prime Factors solution to one written by Uncle Bob. In the comments, someone suggested that Andrew use an iterator instead so I thought I’d give it a try.

First, let’s repost the original code:

private const int SMALLEST_PRIME = 2;

public List<int> Generate(int i)
{
    List<int> primes = new List<int>();
    int divider = SMALLEST_PRIME;
    while (HasPrimes(i))
    {
        while (IsDivisable(i, divider))
        {
            i = AddPrimeToProductsAndReduce(i, primes, divider);
        }
        divider++;
    }
    return primes;
}

private bool IsDivisable(int i, int divider)
{
    return i%divider == 0;
}

private bool HasPrimes(int i)
{
    return i >= SMALLEST_PRIME;
}

private int AddPrimeToProductsAndReduce(int i, List<int> primes, int prime)
{
    primes.Add(prime);
    i /= prime;
    return i;
}

By switching our method to return IEnumerable<int>, we can replace the primes list with an iterator. We will also remove the AddPrimeToProducts functionality from that helper method since we don’t have the list any more:

public IEnumerable<int> Generate(int i)
{
    int divider = SMALLEST_PRIME;
    while (HasPrimes(i))
    {
        while (IsDivisable(i, divider))
        {
            yield return divider;
            i = Reduce(i, divider);
        }
        divider++;
    }
}

private int Reduce(int i, int prime)
{
    return i / prime;
}

I think this is a good change for three reasons:

  1. There’s nothing about the problem that requires a List<int> be returned, we just want a sequence of the factors.
  2. AddPrimeToProductsAndReduce suggested that it had a side effect, but exactly what wasn’t immediately obvious.
  3. It’s much easier to see what values are being included in the result.

That said, I think we can clean this up even more with a second iterator. Specifically, I think we should break out the logic for our candidate factors:

private IEnumerable<int> Divisors
{
    get
    {
        int x = SMALLEST_PRIME;
        while (true)
            yield return x++;
    }
}

Which allows us to separate the logic for generating a divider from the code that consumes it:

public IEnumerable<int> Generate(int toFactor)
{
    foreach (var divider in Divisors)
    {
        if (!HasPrimes(toFactor))
            break;

        while (IsDivisable(toFactor, divider))
        {
            yield return divider;
            toFactor = Reduce(toFactor, divider);
        }
    }
}

We should also eliminate the negation by flipping HasPrimes to become IsFactored:

public IEnumerable<int> Generate(int toFactor)
{
    foreach (var divider in Divisors)
    {
        if (IsFactored(toFactor))
            break;

        while (IsDivisable(toFactor, divider))
        {
            yield return divider;
            toFactor = Reduce(toFactor, divider);
        }
    }
}

private bool IsFactored(int i)
{
    return i <= 1;
}

This does introduce a (very) minor inefficiency in that the Divisors enumerator will MoveNext() one extra time before breaking out of the loop, which could be mitigated by checking IsFactored both before the foreach and after the while loop. Less readable, insignificantly more efficient…take your pick.

The other advantage to breaking out the logic to generate Divisors is that we can easily pick smarter candidates. One option is to skip even numbers greater than 2. An even better optimization takes advantage of the fact that all primes greater than 3 are of the form x±1 where x is a multiple of 6:

private IEnumerable<int> Divisors
{
    get
    {
        yield return 2;
        yield return 3;
        int i = 6;
        while (true)
        {
            yield return i - 1;
            yield return i + 1;
            i += 6;
        }
    }
}

Implementing this sort of logic in the original version would have been much more difficult, both in terms of correctness and readability.

Posted in .NET. Tags: , . Comments Off on Refactoring with Iterators: Prime Factors

Refactoring with LINQ & Iterators: FindDescendantControl and GetDescendantControls

June 23, 2009 — Keith Dahlby

A while back I put together a quick and dirty implementation of a FindControl extension method:

public static T FindControl<T>(this Control root, string id) where T : Control
{
    Control c = root;
    Queue<Control> q = new Queue<Control>();

    if (c == null || c.ID == id)
        return c as T;
    do
    {
        foreach (Control child in c.Controls)
        {
            if (child.ID == id)
                return child as T;
            if (child.HasControls())
                q.Enqueue(child);
        }
        c = q.Dequeue();
    } while (c != null);
    return null;
}

It got the job done (if the control exists!), but I think we can do better.

Refactoring with Iterators

My first concern is that the method is doing too much. Rather than searching for the provided ID, the majority of the code is devoted to navigating the control’s descendents. Let’s factor out that logic into its own method:

public static IEnumerable<Control> GetDescendantControls(this Control root)
{
    var q = new Queue<Control>();

    var current = root;
    while (true)
    {
        if (current != null && current.HasControls())
            foreach (Control child in current.Controls)
                q.Enqueue(child);

        if (q.Count == 0)
            yield break;

        current = q.Dequeue();
        yield return current;
    }
}

The new method is almost as long as the old one, but now satisfies the Single Responsibility Principle. I also added a check to prevent calling Dequeue() on an empty queue. For those that have studied algorithms, note that this is a breadth-first tree traversal.

Now we can update FindControl:

public static T FindControl<T>(this Control root, string id) where T : Control
{
    Control c = root;

    if (c == null || c.ID == id)
        return c as T;

    foreach (Control child in c.GetDescendantControls())
    {
        if (child.ID == id)
            return child as T;
    }
    return null;
}

With the control tree traversal logic extracted, this updated version is already starting to smell better. But we’re not done yet.

DRY? Don’t Repeat Someone Else, Either

My second concern is how we’re checking for the ID in question. It’s not that the equality operator is a bad choice, as it will work in many scenarios, but rather that it’s not consistent with the existing FindControl method. In particular, the existing FindControl understands naming containers (IDs that contain ‘$’ or ‘:’). Rather than implement our own comparison logic, we should just leverage the framework’s existing implementation:

public static T FindControl<T>(this Control root, string id) where T : Control
{
    if (id == null)
        throw new ArgumentNullException("id");

    if (root == null)
        return null;

    Control c = root.FindControl(id);
    if (c != null)
        return c as T;

    foreach (Control child in c.GetDescendantControls())
    {
        c = child.FindControl(id);
        if (c != null)
            return child as T;
    }
    return null;
}

Fun fact: FindControl will throw a NullReferenceException if id is null.

Refactoring with LINQ

So we have extracted the descendant logic and leaned on the framework for finding the controls, but I’m still not quite satisfied. The method just feels too…procedural. Let’s break down what we’re really trying to do:

  1. Look at the current control and all its descendants.
  2. Use FindControl on each with the specified ID.
  3. When we find the control, return it as type T.

As the subheading might suggest, we can express these steps quite nicely with LINQ:

  1. var controls = root.AsSingleton().Concat(root.GetDescendantControls());
  2. var foundControls = from c in controls
                    let found = c.FindControl(id)
                    where found != null
                    select found;
  3. return foundControls.FirstOrDefault() as T;

Behind the scenes, this is how I might have thought through this code:

  1. We use AsSingleton() (my new preferred name, to align with F#’s Seq.singleton, for AsEnumerable(), which I introduced here) and Concat() to prepend root to the list of its descendants, returned as a lazy enumeration.
  2. We use a query over those controls to retrieve matches from FindControl(), again returned as a lazy enumeration.
  3. We grab the first control found, or null if none match, and return it as T.

Because all our enumerations are lazy, we put off traversal of the entire control tree until we know we need to. In fact, if our ID is found in the root control, GetDescendantControls() won’t even be called! Through just a bit of refactoring, we have both an efficient and readable solution.

For completeness, here’s the final version with a more descriptive name to contrast with the existing FindControl():

public static T FindDescendantControl<T>(this Control root, string id) where T : Control
{
    if (id == null)
        throw new ArgumentNullException("id");

    if (root == null)
        return null;

    var controls = root.AsSingleton().Concat(root.GetDescendantControls());

    var foundControls = from c in controls
                        let found = c.FindControl(id)
                        where found != null
                        select found;

    return foundControls.FirstOrDefault() as T;
}

I have added these methods, along with AsSingleton() and a host of others, to the SharePoint Extensions Lib project. Check it out!

Posted in .NET, WebParts & Controls. Tags: , , . 3 Comments »

Theme-amajig Refactored: Using Feature Properties

July 13, 2008 — Keith Dahlby

In a previous post, I described a feature that would take install and retract modifications to SPTHEMES.XML. Peter Seale suggested providing a method to reapply the changes without a deactivate/activate cycle, specifically for new servers added to a farm. It should be as simple as providing a user interface to call FeatureThemesJob.InstallThemes, but that presents a bit of a problem: InstallThemes expects the name of the themes file, which I declare in the feature receiver. So before we can work on a reapplication interface, let’s move that file name to a more accessible location.

The Revised Feature

A better way to store the theme file name would be as a Feature Property:

<Feature
  Id="E2F8D046-607D-4BB6-93CC-2C04CF04099E"
  Title="SPHOLS Themes"
  Description="Installs SPHOLS and SPHOLSX themes on farm."
  Version="1.0.0.0"
  Scope="Farm"
  ReceiverAssembly="MyBranding, ..."
  ReceiverClass="MyBranding.MyThemesFeatureReceiver"
  xmlns="http://schemas.microsoft.com/sharepoint/">
  <ElementManifests>
    <ElementFile Location="SPTHEMES.XML" />
  </ElementManifests>
  <Properties>
    <Property Key="Solutionizing:ThemesFile" Value="SPTHEMES.XML" />
  </Properties>
</Feature>

And we can remove references to THEMES_FILE from our receiver:

namespace MyBranding {
  public class MyThemesFeatureReceiver : SPFeatureReceiver {
    public override void FeatureActivated(SPFeatureReceiverProperties properties) {
      if (properties == null)
        throw new ArgumentNullException("properties");
      FeatureThemesJob.InstallThemes(properties.Definition);
    }

    public override void FeatureDeactivating(SPFeatureReceiverProperties properties) {
      if (properties == null)
        throw new ArgumentNullException("properties");
      FeatureThemesJob.DeleteThemes(properties.Definition);
    }

    public override void FeatureInstalled(SPFeatureReceiverProperties properties) { }
    public override void FeatureUninstalling(SPFeatureReceiverProperties properties) { }
  }
}

Note that this code is now completely feature-agnostic, reusable for any themes feature.

FeatureThemesJob

Other than purging the logic to persist _themesFile, which I’ll leave as an exercise for the reader, we just need to update Execute to use our new feature property:

    private const string PROP_THEMES_FILE = "Solutionizing:ThemesFile"
    private const string SPTHEMES_PATH = @"TEMPLATE\LAYOUTS\1033\SPTHEMES.XML";
    public override void Execute(Guid targetInstanceId) {
      SPFeatureDefinition fDef = Farm.FeatureDefinitions[_featureID];
      if (fDef != null) {
        SPFeatureProperty themesFileProp = fDef.Properties[PROP_THEMES_FILE];
        if(themesFileProp == null)
          throw new SPException(string.Format("Feature '{0}' is missing property '{1}'.", fDef.DisplayName, PROP_THEMES_FILE));

        DoMerge(SPUtility.GetGenericSetupPath(SPTHEMES_PATH), Path.Combine(fDef.RootDirectory, themesFileProp.Value));
      }
    }

But since we’re in a refactoring mood, we might as well extract the code to retrieve the themes file path:

    internal const string PROP_THEMES_FILE = "Solutionizing:ThemesFile"
    private const string ERR_FEATURE_NOT_FOUND = "Feature '{0}' not found in farm.";
    private const string ERR_MISSING_PROPERTY = "Feature '{0}' is missing property '{1}'.";
    internal string ThemesFilePath {
      get {
        SPFeatureDefinition fDef = Farm.FeatureDefinitions[_featureID];
        if (fDef == null)
          throw new SPException(string.Format(ERR_FEATURE_NOT_FOUND, _featureID));

        SPFeatureProperty prop = fDef.Properties[PROP_THEMES_FILE];
        if (prop == null)
          throw new SPException(string.Format(ERR_MISSING_PROPERTY, fDef.DisplayName, PROP_THEMES_FILE));

        return Path.Combine(fDef.RootDirectory, prop.Value);
      }
    }

Which makes Execute rather elegant:

    private const string SPTHEMES_PATH = @"TEMPLATE\LAYOUTS\1033\SPTHEMES.XML";
    public override void Execute(Guid targetInstanceId) {
      DoMerge(SPUtility.GetGenericSetupPath(SPTHEMES_PATH), ThemesFilePath);
    }

Now everything we need for the timer job is available from the feature definition. The next step is to build an interface to run the job on demand. Stay tuned!

Posted in Branding, Features, Object Model. Tags: , , . 3 Comments »