Refactoring challenge

I don’t like messy, obfuscated code.  But occasionally often, I write it anyway as I can’t quite see the right way to go.  Today is one of those days where I can’t seem to get past some ugly code, none of my normal tricks seem to work.  Instead of me doing any more work on it, I’m curious if anyone else has any ideas.  Here’s the offending function, in the TypeMapMapper class from the trunk (R97) of the AutoMapper codebase:

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;
}

Yes, it’s very, very long. Originally, there were four different exit points all over the place, but that made it difficult to do other things I needed (the BeforeMap and AfterMap calls).  There’s just way too much going on here, so I’m curious if anyone out there in the æther has any other ideas.  Feel free to post ideas here, or for those adventurous folks, submit a patch on the AutoMapper group.  Frankly, it’s making me cross-eyed, so I’ll just turn away in shame.

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.lostechies.com/members/derick.bailey/default.aspx derick.bailey

    to start with, just do some extract-method on those nested if statements. give the extracted methods some meaningful names, and you’re 10 steps in the right direction. :)

  • brad

    Sometimes the problem is with the forest, not the tree.

  • http://blog.robbowley.net Rob Bowley

    if (context.DestinationValue == null && context.InstanceCache.ContainsKey(context))

    feature envy on the context object?

  • http://chaliy.pip.verisignlabs.com/ Mike Chaliy

    1. you have special path for cutom mapper. this is not good. better to have all go throug the same infrastructure.

    var mapper2 = (context.TypeMap.CustomMapper != null)
    ? context.TypeMap.CustomMapper
    : CreateDefaultPropertyMap();

    so we have falttened root if and dublication of before map execution.
    2. caching could be extracted.
    3. at the end, in reallity I think you need full flagged objects, not just anemic data holders for your caontext. this is hard to do something with procedural code and pushing contextes back and forth… this ends with low separation of concerns (your code is best example of critically low sepration). try also follow SRP and this also can help to create more readable code.

  • http://colinjack.blogspot.com colinjack

    I’d flatten it out, change the code so you’ve removed the nested ifs.

    I realize that means duplicating the last two lines multiple times but you can extra a common AfterMap(ResolutionContext context, object mappedObject) method and the next step is to start extracting a lot of top level methods.

    You’d be left with

    if (blah)
    {
    return WellNamedMethod();
    }

    if (foo)
    {
    return AnotherWellNamedMethod();
    }

    return AddToMap(…)

    Obviously AnotherWellNamedMethod might internally have the same sort of thing going on, point is there isn’t a lot of nested if/else going on, its all simple for someone with a small IQ like me to follow.

  • http://neilmosafi.blogspot.com Neil Mosafi

    Well I fancied refactoring it… but there doesn’t seem to be any code to test this method so now I’m scared to change it!

  • http://neilmosafi.blogspot.com Neil Mosafi

    OK, here’s my attempt, though it may well break, but it seems ok! Also bear in mind I didn’t really understand the code I was refactoring too well. What do you think?

    http://pastebin.com/f620201a8

  • zac

    Firstly, you have this line duplicated verbatim in two places:

    context.TypeMap.BeforeMap(context.SourceValue, mappedObject);

    I’d start by seeing how I could get rid of that duplication.

    It seems as though a lot of the cruft before that point is around determining the value of mappedObject. Given how much of the logic around determining the value of mappedObject relates to the state of the context, I’d wonder if this logic probably doesn’t properly belong in the context.

    If that isn’t feasible then maybe try implementing the nested conditionals around determining mappedObject as a Chain of Responsibility just to reduce the nesting.

  • http://james.newtonking.com James Newton-King

    Hi

    Completely off topic from what your post is talking about, but I thought you would be interested…

    I have recently started using AutoMapper (very useful) and I was looking around your source code (as you do) and noticed you might have some threading issues.

    You are doing double check locking but you are calling dictionary’s TryGetValue outside of any lock. Potentially someone in a lock could modify the dictionary at the same time as someone else calls TryGetValue, leading to unpleasantness….

    I recently had this problem pointed out to me on my own project – http://json.codeplex.com/WorkItem/View.aspx?WorkItemId=13457

  • Pat

    Well/mmm,

    The bottom line, does it work? Do you have unit tests w/ coverage?

    Nested if statements like the above is a classic case where it falls in the “bah” or “mmm”… let’s tackle things that are more pressing.

    Also, it’s not that “very, very long” like you say, it fits on one page, and there’s only two “top level if”. Maybe adding comments would suffice for now.

    no change: Don’t add code and/or complexity if the current solution works.

    change: On the other hand, if you find the need to add comments then, yeah, time to refactor!

    mappedObject = tryMethod1(..)

    if (mappedObject != null)
    mappedObject = tryMethod2(..)

    if (mappedObject != null)
    mappedObject = tryMethod3(..)

    return mappedObject;

  • http://www.feddejager.nl Fedde Jager

    Allthough already mentioned..

    I would solve it with a by implementing a Chain of Responsibility pattern. Worked out very well for me when I had a very similar issue a few weeks ago.

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

    @Rob

    Yeah, there’s some of that going around. I don’t that will fix the levels of nested IFs, however.

    @Neil

    I do like that, I think it brings me one step closer to Chain of Responsibility.

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

    @colinjack

    You know, that’s how the code looked at first, multiple exit points. Flattening seems to be key here….

  • http://thomaseyde.blogspot.com Thomas Eyde

    It’s hard to know what to do without specs or tests. Among other things, I see BeforeMap() happens sometimes, but not always. Is that intentional, or is it a bug?

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

    @Thomas

    Yes, not posting the tests was a bad idea. I think I’m also running into the issue where all my tests are black-box integration tests. With the design more locked down, I think it’s time to get unit tests in place.

  • Ollie Riches

    To many If statements and null checking IMO.

    I would try and address this first…

  • Chris Martin

    public object Map(ResolutionContext context, IMappingEngineRunner mapper)
    {
    object mappedObject = GetMappedObject(context, mapper);

    TypeMap map = context.TypeMap;

    map.BeforeMap(context.SourceValue, mappedObject);
    map.AfterMap(context.SourceValue, mappedObject);

    return mappedObject;
    }

    private object GetMappedObject(ResolutionContext context, IMappingEngineRunner mapper)
    {
    TypeMap map = context.TypeMap;
    IConfigurationProvider configurationProvider = mapper.ConfigurationProvider;
    var profileConfiguration = configurationProvider.GetProfileConfiguration(map.Profile);

    if (map.CustomMapper != null)
    {
    return map.CustomMapper(context);
    }

    if (context.DestinationValue == null && context.InstanceCache.ContainsKey(context))
    {
    return context.InstanceCache[context];
    }

    if (context.SourceValue != null || !profileConfiguration.MapNullSourceValuesAsNull)
    {
    object mappedObject = context.DestinationValue ?? mapper.CreateObject(context.DestinationType);

    context.InstanceCache.Add(context, mappedObject);

    map.GetPropertyMaps().ToList().ForEach(pm => MapPropertyValue(context, mapper, mappedObject, pm));
    }

    return null;
    }

    http://pastebin.com/m4cb8f96c
    ^ if unreadable.

  • http://realfiction.net Frank Quednau

    There may be some of what Fowler called “Inappropriate Intimacy” going on with the context. Maybe your context could help you more if you could ask it better questions…apart from the profileconfiguration stuff all booleans are answerable by the context. E.g. if you have something in cache, or destinationvalue is not null what mappedObject becomes is fully answerable by the context. Then you may get something like

    mappedObject = context.TryGetMappedObject(ctx=>mapper.CreateObject(ctx.DestinationType))

    The lambda being a Func that gets called when the context could not resolve the mappedObject. The context can use the lambda’s result to add it to its cache if it so requires (answerable by context).

    my 5 cent :)

  • http://blog.fohjin.com Mark Nijhof

    Hi Jimmy,

    This is what I came up with: http://pastie.org/539064

    Let me know what you think of it.

    -Mark