HiveBrain v1.2.0
Get Started
← Back to all entries
snippetcsharpModerate

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

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
thiscanmethodmakefollowhowandshortereasier

Problem

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

Solution

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[] 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.

Code Snippets

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

Context

StackExchange Code Review Q#5450, answer score: 19

Revisions (0)

No revisions yet.