How can I make this method shorter and easier to follow?

Question

It's long, repetitive, and hard to follow.

    private void Map(object x, JToken json)
    {
        var objType = x.GetType();
        var props = objType.GetProperties().Where(p => p.CanWrite).ToList();

        foreach (var prop in props)
        {
            var type = prop.PropertyType;

            var name = prop.Name;
            var value = json[name];
            var actualName = name;

            if (value == null)
            {
                // try camel cased name
                actualName = name.ToCamelCase(Culture);
                value = json[actualName];
            }

            if (value == null)
            {
                // try lower cased name
                actualName = name.ToLower(Culture);
                value = json[actualName];
            }

            if (value == null)
            {
                // try name with underscores
                actualName = name.AddUnderscores();
                value = json[actualName];
            }

            if (value == null)
            {
                // try name with underscores with lower case
                actualName = name.AddUnderscores().ToLower(Culture);
                value = json[actualName];
            }

            if (value == null)
            {
                // try name with dashes
                actualName = name.AddDashes();
                value = json[actualName];
            }

            if (value == null)
            {
                // try name with dashes with lower case
                actualName = name.AddDashes().ToLower(Culture);
                value = json[actualName];
            }

            if (value == null || value.Type == JTokenType.Null)
            {
                continue;
            }

            // check for nullable and extract underlying type
            if (type.IsGenericType && type.GetGenericTypeDefinition() == typeof(Nullable<>))
            {
                type = type.GetGenericArguments()[0];
            }

            if (type.IsPrimitive)
            {
                // no primitives can contain quotes so we can safely remove them
                // allows converting a json value like {"index": "1"} to an int
                var tmpVal = value.AsString().Replace("\"", string.Empty);
                prop.SetValue(x, tmpVal.ChangeType(type), null);
            }
            else if (type.IsEnum)
            {
                string raw = value.AsString();
                var converted = Enum.Parse(type, raw, false);
                prop.SetValue(x, converted, null);
            }
            else if (type == typeof(Uri))
            {
                string raw = value.AsString();
                var uri = new Uri(raw, UriKind.RelativeOrAbsolute);
                prop.SetValue(x, uri, null);
            }
            else if (type == typeof(string))
            {
                string raw = value.AsString();
                prop.SetValue(x, raw, null);
            }
            else if (type == typeof(DateTime) || type == typeof(DateTimeOffset))
            {
                DateTime dt;
                if (DateFormat.HasValue())
                {
                    var clean = value.AsString();
                    dt = DateTime.ParseExact(clean, DateFormat, Culture);
                }
                else if (value.Type == JTokenType.Date)
                {
                    dt = value.Value<DateTime>().ToUniversalTime();
                }
                else
                {
                    // try parsing instead
                    dt = value.AsString().ParseJsonDate(Culture);
                }

                if (type == typeof(DateTime))
                    prop.SetValue(x, dt, null);
                else if (type == typeof(DateTimeOffset))
                    prop.SetValue(x, (DateTimeOffset)dt, null);
            }
            else if (type == typeof(Decimal))
            {
                var dec = Decimal.Parse(value.AsString(), Culture);
                prop.SetValue(x, dec, null);
            }
            else if (type == typeof(Guid))
            {
                string raw = value.AsString();
                var guid = string.IsNullOrEmpty(raw) ? Guid.Empty : new Guid(raw);
                prop.SetValue(x, guid, null);
            }
            else if (type.IsGenericType)
            {
                var genericTypeDef = type.GetGenericTypeDefinition();
                if (genericTypeDef == typeof(List<>))
                {
                    var list = BuildList(type, value.Children());
                    prop.SetValue(x, list, null);
                }
                else if (genericTypeDef == typeof(Dictionary<,>))
                {
                    var keyType = type.GetGenericArguments()[0];

                    // only supports Dict<string, T>()
                    if (keyType == typeof(string))
                    {
                        var dict = BuildDictionary(type, value.Children());
                        prop.SetValue(x, dict, null);
                    }
                }
                else
                {
                    // nested property classes
                    var item = CreateAndMap(type, json[actualName]);
                    prop.SetValue(x, item, null);
                }
            }
            else
            {
                // nested property classes
                var item = CreateAndMap(type, json[actualName]);
                prop.SetValue(x, item, null);
            }
        }
    }

Answer 1

To expand on Carl's answer a little more, this would be a good candidate for the strategy pattern. You could add each case as a strategy callback:

private var toValue(var name, params Func<var, var>[] matchingStrategies)
{
    foreach(var strategy in matchingStrategies)
    {
        value = json[strategy(name)];

        if (value != null)
            return value;
    }

    return null;
}

// usage:

var name = prop.Name;

var value = toValue(name,
    (n) => n.ToCamelCase(Culture),
    (n) => n.ToLower(Culture),
    (n) => n.AddUnderscores(),
    (n) => n.AddUnderscores().ToLower(Culture),
    (n) => n.AddDashes(),
    (n) => n.AddDashes().ToLower(),
    (n) => n.ToCamelCase(Culture));

So you're passing a list of matching strategies in order of execution, with the function returning the result of the first successful matching strategy.

Or you could encapsulate each strategy into its own class - which would enable you to unit test them individually.

Answer 2

Step 1: extract the body of the loop into its own method that takes prop as a parameter. Now your primary method looks like

private void Map(object x, JToken json)
    {
        var objType = x.GetType();
        var props = objType.GetProperties().Where(p => p.CanWrite).ToList();

        foreach (var prop in props)
        {
             process(prop);
        }
    }

and that's a good start.

Now we spend a good amount of code getting the value from the name. Extract that:

private var toValue(var name)
    {
        var value = json[name];
        if (value != null) return value;

        value = json[name.ToCamelCase(Culture)];
        if (value != null) return value;

        value = json[name.ToLower(Culture)];
        if (value != null) return value;

        value = json[name.AddUnderscores()];
        if (value != null) return value;

        value = json[name.AddUnderscores().ToLower(Culture)];
        if (value != null) return value;

        value = json[name.AddDashes()];
        if (value != null) return value;

        value = json[name.AddDashes().ToLower(Culture)];
        if (value != null) return value;

        value = json[name.ToCamelCase(Culture)];
        if (value != null) return value;

        return null;
    }

Notice how much cleaner this section can be, because we've extracted it. We simply return the value when it's non-null, rather than checking over and over again whether it's null. And this extracted method has a clear single purpose; if you came up with another variant of how to process the name to make it a valid key, it would be clear where to put it. Smaller methods pay off in many ways.

I think that is a good start and you should be able to see where to go from here.

Answer 3

I'd also drop the .ToList() on this line:

var props = objType.GetProperties().Where(p => p.CanWrite).ToList();

because props is only used in one place, to be enumerated over in the foreach loop.

Calling ToList forces the computer to loop all the way through the collection, just to convert it into a list.

Then on the very next line it loops all the way through a second time. Why do all that work just to get a list?

The code would behave the same, only faster, if we just loop over the collection once

Answer 4

I personally try to stay away from using the var keyword, as sometimes it tends to make code unreadable for example var result = ReturnResult(); when reading this I don't really know what the type it is at a glance. And try to initialize your variables outside of the foreach loop. I would normally declare all my variables at the top of the method and use them later on if needed.