Just a quick warning - neither of the below are refactors as such, just ways of rewriting what you already have. Given the variable nature of the conditions and their imposed ordering (eg basetype of baseproperty taking precedence over data), it is hard to think of anything better or clearer than an if-else. IMO you can't really improve on what you have by just refactoring that one function. But I'll present some possible re-writes anyway in case either appeals.
Could use elses (simply for readability):
Note: Under some coding guidelines, this is less preferential to what you already have. Personally, I find it more readable however.
private void DeserializeProperty(object value,
PropertyInfo property, Format format) {
if (value == DBNull.Value || value == null) {
property.SetValue(this, null);
}
else if (property.PropertyType == typeof(Id)) {
SetIdProperty(value, property);
}
else if (property.PropertyType.BaseType == typeof(BaseProperty)) {
SetBaseProperty(value, property);
}
else if (property.PropertyType == typeof(Data)) {
DeserializeData(value, property, format);
}
else {
DeserializeNormalProperty(property, value);
}
}
An Alternative (if you so desire)
If you are doing lots of similar switches, using a custom CleverSwitch class similar to this:
https://stackoverflow.com/a/299120/3940783
might work for you. It'd be rather similar to your idea about the dictionary of Actions - but due to the variation in your conditions, it'd be cleanest to have Actions with no inputs and just use the variables from the DeserializeProperty's scope without passing them into the action.
So, for example, you could replace the function with the following:
private void DeserializeProperty(object value,
PropertyInfo property, Format format) {
CleverSwitch.Do(
CleverSwitch.If(() => value == DBNull.Value || value == null, () => property.SetValue(this, null))
CleverSwitch.IsType<Id>(property.PropertyType, () => SetIdProperty(value, property)),
CleverSwitch.IsType<BaseProperty>(property.PropertyType.BaseType, () => SetBaseProperty(value, property)),
CleverSwitch.IsType<Data>(property.PropertyType, () => DeserializeData(value, property, format)),
CleverSwitch.Default(() => DeserializeNormalProperty(property,value))
);
}
Where CleverSwitch works similarly to JaredPar's TypeSwitch at the above link - and could be coded as follows:
static class CleverSwitch {
public class CaseInfo {
public Func<bool> Condition { get; set; }
public Action Action { get; set; }
}
public static void Do(object source, params CaseInfo[] cases) {
var type = source.GetType();
foreach (var entry in cases) {
if (entry.Condition()) {
entry.Action();
break;
}
}
}
public static CaseInfo IsType<T>(Type T2, Action action) {
return new CaseInfo() { Condition = () => T2 == typeof(T), Action = action };
}
public static CaseInfo If(Func<bool> condition, Action action) {
return new CaseInfo() { Condition = condition, Action = action };
}
public static CaseInfo Default(Action action) {
return new CaseInfo() { Condition = () => true, Action = action };
}
}
But on balance:
Any class to help you refactor it in the manner of the dictionary approach is going to have to be rather general in this case, and so essentially just be confusing and add performance overheads.
So whilst the above code should work, I really can't see how it improves on an if statement in any of the following areas:
- time to write
- readability or
- maintainability
In fact, I'd argue that a simple if, return or if, else combination is more readable, maintainable and easier to write straight off... But that's just my two cents!
