Refactoring challenge #2 – functionally illiterate

In the last refactoring challenge, I had a problem with some nasty conditional complexity.  To be honest, the challenge was a subtle way to crowdsource new features in AutoMapper, but hey, it worked.  One of the hard parts of being a sole contributor to an OSS project is that I lose the benefits of teammates and pair programming, to be able to bounce design ideas around and brainstorm solutions.  So instead, I’ll just post my problems here.

The next issue I’m trying to address is the ability to configure the matching algorithm AutoMapper uses to determine that member Foo matches up to another member Foo on two separate types.  Right off the bat, we could simply match by name.  But, AutoMapper has one killer piece of functionality, the ability to flatten source member names to destination member names, so that “src.Foo.Bar.Bazz.Bam” can map to “dest.FooBarBazzBam”, or “dest.FooBarBazz.Bam”, or “dest.FooBar.BazzBam”, or “dest.Foo.Bar.Bazz.Bam”.  But in taking Ayende’s advice to JFHCI, I wound up hardcoding a lot of the algorithm to match as well.  It’s all encapsulated in one class, but it’s not easy to change.

To get to where I want to go, such as being able to supply postfixes to accept (src.CustomerId matching to dest.Customer), I need to shore up the searching algorithm to make it more understandable.  All mapping configuration is contained in a TypeMap class, created by this method:

public TypeMap CreateTypeMap(Type sourceType, Type destinationType)
{
    var sourceTypeInfo = GetTypeInfo(sourceType);
    var destTypeInfo = GetTypeInfo(destinationType);

    var typeMap = new TypeMap(sourceTypeInfo, destTypeInfo);

    foreach (IMemberAccessor destProperty in destTypeInfo.GetPublicReadAccessors())
    {
        var resolvers = new LinkedList<IValueResolver>();

        if (MapDestinationPropertyToSource(resolvers, sourceTypeInfo, destProperty.Name))
        {
            typeMap.AddPropertyMap(destProperty, resolvers);
        }
    }
    return typeMap;
}

This isn’t the ugly part, it’s the MapDestinationPropertyToSource that’s the beast.  But as you can see above, all I really do is loop through the destination properties, trying to find a match on the source type.  But again like I had with the mapping execution, I have a bit of nastiness in the matching:

private bool MapDestinationPropertyToSource(LinkedList<IValueResolver> resolvers, TypeInfo sourceType, string nameToSearch)
{
    var sourceProperties = sourceType.GetPublicReadAccessors();
    var sourceNoArgMethods = sourceType.GetPublicNoArgMethods();

    IValueResolver resolver = FindTypeMember(sourceProperties, sourceNoArgMethods, nameToSearch);

    bool foundMatch = resolver != null;

    if (!foundMatch)
    {
        string[] matches = Regex.Matches(nameToSearch, @"p{Lu}[p{Ll}0-9]*")
            .Cast<Match>()
            .Select(m => m.Value)
            .ToArray();

        for (int i = 0; i < matches.Length - 1; i++)
        {
            NameSnippet snippet = CreateNameSnippet(matches, i);

            IMemberAccessor valueResolver = FindTypeMember(sourceProperties, sourceNoArgMethods, snippet.First);

            if (valueResolver == null)
            {
                continue;
            }

            resolvers.AddLast(valueResolver);

            foundMatch = MapDestinationPropertyToSource(resolvers, GetTypeInfo(valueResolver.MemberType), snippet.Second);

            if (foundMatch)
            {
                break;
            }

            resolvers.RemoveLast();
        }
    }
    else
    {
        resolvers.AddLast(resolver);
    }

    return foundMatch;
}

Again, problems with conditional complexity, and even more, cyclomatic complexity with the loop continuation/breaking.  The actual matching is quite easy to do:

private static IMemberAccessor FindTypeMember(IEnumerable<IMemberAccessor> modelProperties, IEnumerable<MethodInfo> getMethods, string nameToSearch)
{
    IMemberAccessor pi = modelProperties.FirstOrDefault(prop => NameMatches(prop.Name, nameToSearch));
    if (pi != null)
        return pi;

    string getName = "Get" + nameToSearch;
    MethodInfo mi = getMethods.FirstOrDefault(m => (NameMatches(m.Name, getName)) || NameMatches(m.Name, nameToSearch));
    if (mi != null)
        return new MethodAccessor(mi);

    return null;
}

private static bool NameMatches(string memberName, string nameToMatch)
{
    return String.Compare(memberName, nameToMatch, StringComparison.OrdinalIgnoreCase) == 0;
}

private static NameSnippet CreateNameSnippet(IEnumerable<string> matches, int i)
{
    return new NameSnippet
               {
                   First = String.Concat(matches.Take(i + 1).ToArray()),
                   Second = String.Concat(matches.Skip(i + 1).ToArray())
               };
}

private class NameSnippet
{
    public string First { get; set; }
    public string Second { get; set; }
}

I’ve hardcoded the method prefix option (i.e., GetValue() maps to a property called Value), and that would be easy to switch out.  But since I’m in the middle of needing to change all of the matching algorithm, I figured I’d go ahead and take care of the MapDestinationPropertyToSource.  It’s quite complex, with the looping, linked list, and recursion.  But algorithms are definitely not my strong suit, more modeling and patterns.

Given the recursive nature of the matching algorithm, I’m assuming that a better solution exists, but’s it’s probably functional in nature.  Unfortunately, I’m functionally illiterate, but I’m sure some more enterprising folks have a better solution.  So here’s the challenge – fix the “MapDestinationPropertyToSource” method, so it actually makes some sense when I come back to it later.

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.
  • Tom de Koning

    What about CQS in MapDestinationPropertyToSource? The if makes no sense in the conditional flow of CreateTypeMap. I would separate that first, ie introduce another layer before worrying too much about the functional issus..

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

    @tom

    I agree, I don’t like the idea of the method returning a boolean. Ideally, the collecting parameter (LinkedList) would be the indication that the matching was successful or not. The trick is to preserve the order, and be able to follow multiple paths.