Refactoring challenge Part 1 – Examination

Most of the time I post code on my blog, it’s something I’m proud of.  Other times, it’s code I didn’t write, which I promptly lambaste.  In my last post, I threw up code I did write, but couldn’t see the design coming out.  From the responses, I think I have some direction.  But first, let’s take a closer look at the design issues with the original code, and try to catalog the code smells.  This should give us direction on how we need to refactor it.  Here’s the original code:

public object Map(ResolutionContext context, IMappingEngineRunner mapper)
{
    object mappedObject = null;
    var profileConfiguration = mapper.ConfigurationProvider.GetProfileConfiguration(context.TypeMap.Profile);

    if (context.TypeMap.CustomMapper != null)
    {
        context.TypeMap.BeforeMap(context.SourceValue, mappedObject);
        mappedObject = context.TypeMap.CustomMapper(context);
    }
    else if (context.SourceValue != null || !profileConfiguration.MapNullSourceValuesAsNull)
    {
        if (context.DestinationValue == null && context.InstanceCache.ContainsKey(context))
        {
            mappedObject = context.InstanceCache[context];
        }
        else
        {
            if (context.DestinationValue == null)
            {
                mappedObject = mapper.CreateObject(context.DestinationType);
                if (context.SourceValue != null)
                    context.InstanceCache.Add(context, mappedObject);
            }
            else
            {
                mappedObject = context.DestinationValue;
            }

            context.TypeMap.BeforeMap(context.SourceValue, mappedObject);
            foreach (PropertyMap propertyMap in context.TypeMap.GetPropertyMaps())
            {
                MapPropertyValue(context, mapper, mappedObject, propertyMap);
            }
        }

    }

    context.TypeMap.AfterMap(context.SourceValue, mappedObject);
    return mappedObject;
}

So let’s list out the code smells, there are quite a few.

No tests

I forgot to post the tests, but even more telling, I only really have large-grained, scenario based tests that cover this class.  No unit tests, and that problem starts to show here, as tests would have surfaced the code smells earlier.

Feature Envy

There’s lots of poking around the ResolutionContext object (which is responsible for holding all information pertaining to a mapping operation).  For example, the first nested “if” asks the ResolutionContext a lot of things that don’t seem to make any sense by themselves.  Additionally, it’s not clear at all what each conditional has to do with the work being done.

####

Long Method

This should be quite obvious.  For me, the ideal maximum method length is about 10 lines.  That’s pretty much the limit of my understanding of what’s going on.  Any more than that, and I have to put too much context into my head.

Conditional Complexity

Way too much branching going on here, and even worse, it looks like arrowhead programming.  Flip it sideways, it looks like a mountain range with all the indention.  The cyclomatic complexity is off the charts here, and a tool like NDepend would let me know right away where my maintenance problems will be.

Those are a lot of code smells, but each one has their corresponding refactoring to take care of it.  In retrospect, I’m not sure why I just didn’t look for code smells in the first place.  In the next few posts, I’ll apply refactorings one by one to get to my destination – clean code.