7
\$\begingroup\$

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);
            }
        }
    }
\$\endgroup\$
  • 2
    \$\begingroup\$ I am downvoting this because there's no description of what the code does, it is basically a "copy-paste" question. Please at least include a short description of what the code does. \$\endgroup\$ – Simon Forsberg Jan 18 '14 at 18:48
  • \$\begingroup\$ I'm not a c# programmer. However, one thing I noticed is that you have multiple if statements who all share the condition value == null. Why do you need like 4 if statements with the same condition? Why not simple take all the blocks from the 4 statements and put them into one? \$\endgroup\$ – Ashwin Gupta Feb 12 '16 at 6:16
19
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I'm pretty sure C# doesn't support var as a return type, or as part of a Func declaration. \$\endgroup\$ – R0MANARMY Oct 21 '11 at 13:35
  • 4
    \$\begingroup\$ Fair enough, if you're feeling particularly ambitious you could also probably replace the foreach loop with a where and FirstOrDefault linq query. Would make the intent of the code a bit clearer. \$\endgroup\$ – R0MANARMY Oct 21 '11 at 16:19
11
\$\begingroup\$

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.

\$\endgroup\$
5
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ Why would you drop the .ToList()? \$\endgroup\$ – Jeff Vanzella Nov 2 '12 at 22:28
  • \$\begingroup\$ @JeffVanzella, 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 like 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. \$\endgroup\$ – Buh Buh Feb 26 '13 at 23:06
  • 1
    \$\begingroup\$ Just wanted you to elaborate :) \$\endgroup\$ – Jeff Vanzella Jan 15 '14 at 22:34
-4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ the problem is not var, but the naming. \$\endgroup\$ – Bart Calixto Feb 12 '16 at 5:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.