Parsing an ExpandoObject into a typed class using reflection

Question

I've spent some time writing a library to parse JSON into a statically typed object. Am I following most of the standard coding guidelines? Any thoughts on what I could do better, or any edge cases that are missing and aren't taken care of?

private static T ParseDynamic(ExpandoObject input)
{
 //Parse when given an ExpandoObject
 T output = default(T);
 var dict = input as IDictionary<string, object>;
 ParseDictionary<T>(dict, out output);
 return output;
}
protected static void ParseDictionary(IDictionary<string, object> Dict, out object Target, Type explicitType)
{
 if(Dict == null)
 {
 throw new InvalidOperationException("Dictionary was null, cannot parse a null dictionary");
 }
 if (explicitType.IsArray)
 {
 var length = Dict.Keys.Count();
 Target = (Array)Activator.CreateInstance(explicitType, new object[] { length });
 }
 else
 {
 Target = Activator.CreateInstance(explicitType);
 }
 foreach (var property in Target.GetType().GetProperties())
 {
 var propertyName = property.Name;
 if (Dict.ContainsKey(propertyName) && Dict[propertyName] != null)
 {
 var val = Dict[propertyName];
 var propertyVal = explicitType.GetProperty(propertyName);
 var expectedType = property.PropertyType;
 var valType = val.GetType();
 if(valType == expectedType)
 {
 //Hurray, we matched!
 propertyVal.SetValue(Target, val);
 }
 else if (valType != expectedType && val is IConvertible)
 {
 Type safeType = Nullable.GetUnderlyingType(expectedType) ?? expectedType;
 //Special Case - INT64 to DATETIME Conversion (UNIX Time)
 if((valType == typeof(long) || valType == typeof(long?))
 && (safeType == typeof(DateTime) || safeType == typeof(DateTime?)))
 {
 var longValue = (long)Convert.ChangeType(val, typeof(long));
 var dateValue = UNIX_EPOCH.AddSeconds(longValue);
 val = dateValue;
 }
 //Convert if possible
 var explicitVal = (val == null ? null : Convert.ChangeType(val, safeType));
 propertyVal.SetValue(Target, explicitVal, null);
 }
 else if (val is IDictionary<string, object>)
 {
 //Parse non-simple object
 var propType = propertyVal.PropertyType;
 object explicitVal = Activator.CreateInstance(propType);
 ParseDictionary(val as IDictionary<string, object>, out explicitVal, propType);
 propertyVal.SetValue(Target, explicitVal);
 }
 else if (val is IList)
 {
 //Parse list/enumeration/array
 Type elementType;
 if (expectedType.IsArray)
 {
 //Array type is explicitly included with GetElementType
 elementType = expectedType.GetElementType();
 }
 else if (expectedType.IsGenericType)
 {
 //Get List type by inspecting generic argument
 elementType = expectedType.GetGenericArguments()[0];
 }
 else
 {
 //Not sure how we'd get here if we're neither an array nor generic, but we can't really do much
 continue;
 }
 //Create the necessary List implementation that we need
 var listType = typeof(List<>);
 var typedList = listType.MakeGenericType(elementType);
 var explicitList = (IList)Activator.CreateInstance(typedList);
 foreach(var element in val as IList<object>)
 {
 object explicitElement;
 ParseDictionary(element as IDictionary<string, object>, out explicitElement, elementType);
 explicitList.Add(explicitElement);
 }
 if(property.PropertyType.IsArray)
 {
 //Convert from list to array if necessary
 var arrayType = elementType.MakeArrayType();
 var array = (Array)Activator.CreateInstance(arrayType, new object[] { explicitList.Count });
 explicitList.CopyTo(array, 0);
 propertyVal.SetValue(Target, array);
 }
 else
 {
 propertyVal.SetValue(Target, explicitList);
 }
 }
 else
 {
 //Attempt to set it - will error if not compatible and all other checks are bypassed
 propertyVal.SetValue(Target, val);
 }
 }
 }
} 
protected static void ParseDictionary<K>(IDictionary<string, object> Dict, out K Target) where K : class, new()
{
 Target = new K();
 var explicitType = Target.GetType();
 var outObject = new object();
 ParseDictionary(Dict, out outObject, explicitType);
 Target = outObject as K;
}

• SirPython: Welcome to Code Review! I hope you get some helpful answers.
• akw5013: Thanks! Looking at it on my own, I just now realized that I've mixed my naming styles.
• RubberDuck: @akw5013 feel free to write a self answer pointing it out. We encourage self review.

Answer 1

protected static void ParseDictionary(IDictionary<string, object> Dict, out object Target, Type explicitType)

I would say this method declaration is a big code smell. Having a void method with an out parameter just isn't right.

Why don't you let the method return an object ? There is just no reason.

Although it had been already mentioned by @akw5013 in his answer, based on the NET naming guidelines methodparameters should be named using camelCase casing.
If you choose a style for yourself, which is a valid decision, you should stick to this style. At this declaration you are mixing styles which is always a bad idea.

---

if (Dict.ContainsKey(propertyName) && Dict[propertyName] != null)
{
 var val = Dict[propertyName];
 var propertyVal = explicitType.GetProperty(propertyName);

This will be "incredible" slow. Internally this calls 3 times the FindEntry(key) method through the following methods

public bool ContainsKey(TKey key) {
 return FindEntry(key) >= 0;
} 
public TValue this[TKey key] {
 get {
 int i = FindEntry(key);
 if (i >= 0) return entries[i].value;
 ThrowHelper.ThrowKeyNotFoundException();
 return default(TValue);
 }
 set {
 Insert(key, value, false);
 }
}

if you would use Dictionary.TryGetValue() this would result in only one call to

public bool TryGetValue(TKey key, out TValue value) {
 int i = FindEntry(key);
 if (i >= 0) {
 value = entries[i].value;
 return true;
 }
 value = default(TValue);
 return false;
} 

but fortunately this problem is easy to fix like so

 object val;
 var propertyName = property.Name;
 if (Dict.TryGetValue(propertyName, out val) && val != null)
 {
 var propertyVal = explicitType.GetProperty(propertyName);

that had been easy, hadn't it ?

if(valType == expectedType)
{
 //Hurray, we matched!
 propertyVal.SetValue(Target, val);
}
else if (valType != expectedType && val is IConvertible) 

if the execution comes to the else if there is a 100% chance that valType != expectedType will be true.

else if (valType != expectedType && val is IConvertible)
{
 Type safeType = Nullable.GetUnderlyingType(expectedType) ?? expectedType;
 //Special Case - INT64 to DATETIME Conversion (UNIX Time)
 if ((valType == typeof(long) || valType == typeof(long?))
 && (safeType == typeof(DateTime) || safeType == typeof(DateTime?)))
 {
 var longValue = (long)Convert.ChangeType(val, typeof(long));
 var dateValue = UNIX_EPOCH.AddSeconds(longValue);
 val = dateValue;
 }
 //Convert if possible
 var explicitVal = (val == null ? null : Convert.ChangeType(val, safeType));
 propertyVal.SetValue(Target, explicitVal, null);
}

the use of dateValue can just be skipped. Assigning the return value of UNIX_EPOCH.AddSeconds() to val is enough.

The tenary expression would IMO be better just an if..else like so

if (val == null)
{
 propertyVal.SetValue(Target, null, null);
}
else 
{
 propertyVal.SetValue(Target, Convert.ChangeType(val, safeType), null);
} 

else if (val is IDictionary<string, object>)
{
 //Parse non-simple object
 var propType = propertyVal.PropertyType;
 object explicitVal = Activator.CreateInstance(propType);
 ParseDictionary(val as IDictionary<string, object>, out explicitVal, propType);
 propertyVal.SetValue(Target, explicitVal);
} 

Here you are creating an instance of propType which is overwritten inside the recursively called ParseDictionary() method. Just skip it.

var listType = typeof(List<>);
var typedList = listType.MakeGenericType(elementType); 

the var listType is only used once at the next line of code. You can compact this like so

var typedList = typeof(List<>).MakeGenericType(elementType); 

and if you want you can just keep your pattern of typeType so typedList will become listType.

The creation of the List of this else if branch should be extracted to a separate method like so

private static IList ParseAsList(object val, Type expectedType, PropertyInfo property)
{
 Type elementType = null;
 if (expectedType.IsArray) //Array type is explicitly included with GetElementType
 {
 elementType = expectedType.GetElementType();
 }
 else if (expectedType.IsGenericType) //Get List type by inspecting generic argument
 {
 elementType = expectedType.GetGenericArguments()[0];
 }
 var listType = typeof(List<>).MakeGenericType(elementType);
 var explicitList = (IList)Activator.CreateInstance(listType);
 foreach (var element in val as IList<object>)
 {
 object explicitElement = ParseDictionary(element as IDictionary<string, object>, elementType);
 explicitList.Add(explicitElement);
 }
 return explicitList;
}

which results in the branch beeing

else if (val is IList)
{
 //Parse list/enumeration/array
 if (!(expectedType.IsArray || expectedType.IsGenericType)
 {
 //Not sure how we'd get here if we're neither an array nor generic, but we can't really do much
 continue;
 }
 //Create the necessary List implementation that we need
 var explicitList = ParseAsList(val, expectedType, property);
 if (expectedType.IsArray)
 {
 //Convert from list to array if necessary
 var arrayType = expectedType.GetElementType().MakeArrayType();
 var array = (Array)Activator.CreateInstance(arrayType, new object[] { explicitList.Count });
 explicitList.CopyTo(array, 0);
 propertyVal.SetValue(Target, array);
 }
 else
 {
 propertyVal.SetValue(Target, explicitList);
 }
} 

I couldn't come up with a better name than ParseAsList() so its just that.

The former ParseDictionary() method will now look like so

protected static object ParseDictionary(IDictionary<string, object> dict, Type explicitType)
{
 if (dict == null)
 {
 throw new ArgumentNullException("dict", "Dictionary was null, cannot parse a null dictionary");
 }
 object target;
 if (explicitType.IsArray)
 {
 var length = dict.Keys.Count();
 target = (Array)Activator.CreateInstance(explicitType, new object[] { length });
 }
 else
 {
 target = Activator.CreateInstance(explicitType);
 }
 foreach (var property in target.GetType().GetProperties())
 {
 var propertyName = property.Name;
 object val;
 if (dict.TryGetValue(propertyName, out val) && val != null)
 {
 var propertyVal = explicitType.GetProperty(propertyName);
 var expectedType = property.PropertyType;
 var valType = val.GetType();
 if (valType == expectedType)
 {
 propertyVal.SetValue(target, val);
 }
 else if (val is IConvertible)
 {
 Type safeType = Nullable.GetUnderlyingType(expectedType) ?? expectedType;
 //Special Case - INT64 to DATETIME Conversion (UNIX Time)
 if ((valType == typeof(long) || valType == typeof(long?))
 && (safeType == typeof(DateTime) || safeType == typeof(DateTime?)))
 {
 var longValue = (long)Convert.ChangeType(val, typeof(long));
 val = UNIX_EPOCH.AddSeconds(longValue);
 }
 if (val == null)
 {
 propertyVal.SetValue(target, null, null);
 }
 else
 {
 propertyVal.SetValue(target, Convert.ChangeType(val, safeType), null);
 }
 }
 else if (val is IDictionary<string, object>)
 {
 //Parse non-simple object
 var propType = propertyVal.PropertyType;
 object explicitVal = ParseDictionary(val as IDictionary<string, object>, propType);
 propertyVal.SetValue(target, explicitVal);
 }
 else if (val is IList)
 {
 //Parse list/enumeration/array
 if (!(expectedType.IsArray || expectedType.IsGenericType))
 {
 //Not sure how we'd get here if we're neither an array nor generic, but we can't really do much
 continue;
 }
 //Create the necessary List implementation that we need
 var explicitList = ParseAsList(val, expectedType, property);
 if (expectedType.IsArray)
 {
 //Convert from list to array if necessary
 var arrayType = expectedType.GetElementType().MakeArrayType();
 var array = (Array)Activator.CreateInstance(arrayType, new object[] { explicitList.Count });
 explicitList.CopyTo(array, 0);
 propertyVal.SetValue(target, array);
 }
 else
 {
 propertyVal.SetValue(target, explicitList);
 }
 }
 else
 {
 //Attempt to set it - will error if not compatible and all other checks are bypassed
 propertyVal.SetValue(target, val);
 }
 }
 }
 return target;
}

As you can see I have added some vertical space (new lines) to structure the code a little bit better which makes it more readable too.

• RubberDuck: And I wasn't disappointed. Always a pleasure reading your answers. ++
• akw5013: Thank you very much for the feedback! The main reason I am stuffing values into variables when they are overridden/used once is just for easier debugging while I step through. That way I can hover over the variable and get the value without having to add a watch to it.

Answer 2

if(Dict == null)
{
 throw new InvalidOperationException("Dictionary was null, cannot parse a null dictionary");
}

Yes! I'm a huge fan of this kind of defensive coding. Plus you gave a meaningful message! Awesome! There is a problem though, you should use an ArgumentNullException and pass it the parameter that failed to muster. It's more semantically correct and provides even more information back to the developer. I encourage you to look up the documentation on the two different exceptions to see more about when each is appropriate.

---

 else
 {
 //Not sure how we'd get here if we're neither an array nor generic, but we can't really do much
 continue;
 }

Well, if you don't think you could ever get there, maybe you should throw an exception? If you don't think an exception is necessary, or don't want to throw one in production, then consider using Debug.Assert(false) to ensure that you're aware of it if you ever do end up in this logic branch.

• akw5013: I knew there was an exception type that I was missing. Thanks!
• RubberDuck: @akw5013 ^ Heslacher is right.

Answer 3

ParseDictionary(IDictionary<string, object> Dict, out object Target, Type explicitType)

You should properly case the parameter names using camel casing.

ParseDictionary(IDictionary<string, object> inputDictionary, out object targetObject, Type explicitType)