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.

Related Articles:

Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Jimmy Bogard

I'm a technical architect with Headspring in Austin, TX. I focus on DDD, distributed systems, and any other acronym-centric design/architecture/methodology. I created AutoMapper and am a co-author of the ASP.NET MVC in Action books.
This entry was posted in Refactoring. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • http://www.agilification.com Jeff Doolittle

    Thanks for being willing to humbly post ugly code for the common good! While I’m interested in seeing the refactoring progress, I’d also be interested to see how this may have turned out if built using TDD and emerging design principles.

  • Karl

    There is also a Law of Demeter violiation in context.TypeMap.CustomMapper and context.InstanceCache.ContainsKey.
    This violation will automatically go away if you move more of the logic into the Resolutioncontext as you pointed out above (Feature Envy).

  • Jay

    So why not do it right the first time (i.e. create your unit tests first and design it incramentally, etc)? This would have avoided all the rework and lost productivity that has occurred.

    None the less, for me and I’m sure everyone else, I am grateful that you did it this way so we can all learn how to code much better!

    Thanks!

  • http://www.lostechies.com/members/bogardj/default.aspx bogardj

    @Jay

    One thing to keep in mind is that code changes over time, and it’s not always feasible to design it “right” the first time. The first time around, I only had one scenario. Tests would have shown me design issues, but only by looking at multiple tests, and the different responsibilities tested.

    This specific code has grown quite gradually over time, so I don’t really feel too bad about it getting “bad”. That’s the whole point of code smells, to recognize when things need to change at the point in time it needs to change, but not before.

  • Jay

    I guess my point was incremental design vs designing it right the first time. I agree that I don’t think it is really possible to design it right the first time. However, designing code (IMO) should be part of the small incremental steps of coding. Using TDD to help you decide what your design should be so that one can see code smells and be able to avoid the rework one would have after the fact.

    My guess is that we could have a long discussion on what exactly is rework and refactoring. I think that’s probably suited for another discussion. I’d rather concentrate on the solution here and improving the process so it doesn’t peek it’s ugly head again because I have written my share of code smells too! :)

  • http://www.twitter.com/freddy_rios freddy

    Maybe something along the lines:

    public object Map(ResolutionContext context, IMappingEngineRunner mapper)
    {
    MappingSession mapping = context.StartMappingObject(mapper);
    foreach (PropertyMap propertyMap in mapping.PropertiesNeedingToBeMapped)
    {
    MapPropertyValue(context, mapper, mapping.MappedObject, propertyMap);
    }
    context.EndMapping(mapping);
    return mapping.MappedObject;
    }

    inside StartMappingObject, u could have something like:

    return TypeMap.GetMappedObject(this, DefaultMapper);

    inside GetMappedObject:

    if ( CustomMapper != null )
    {
    BeforeMap(context.SourceValue, null);
    mappedObject = CustomMapper(context);
    }
    else
    {
    var mappedInfo = defaultMapper(mapper,context);
    mappedObject = mappedInfo.MappedObject;
    if( ! mappedInfo.IsCached )
    {
    BeforeMap(context.SourceValue,mappedObject);
    }
    }
    return mappedObject;

    That was just a quick approach, but I really dunno what that code is supposed to be doing. I don’t get why BeforeMap is called before at one time and after at the other, if u could always call it after, u might want to move that into context instead. If not u could still move the BeforeMap outside the if, and u only get a simple if/else. Also in that case, u can refactor the above to avoid passing in the DefaultMapper, by just calling one or the other right from context (which seems hard know because of the placing of the BeforeMap).

    I hope that helps and that I didn’t say a bunch of senseless stuff for the scenario :)