Although it had been already mentioned by @akw5013@akw5013 in his answeranswer, 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.
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.
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.
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.