Problem
All of these if conditions in the below method have similar pattern, Any ideas to come up with a common method to reduce the duplication in this method?
some case we want all the version, and in some case we want only specific version.
I tried with switch case.
public override Func<JObject, dynamic, string> VersionMethod => (jobject, parameters) =>{
bool hasValidObject = false;
if (jobject["swVersion"] != null)
{
_livetv.SoftwareVersion = new VersionInfo(jobject["swVersion"].Value<string>());
hasValidObject = true;
}
if (jobject["hwVersion"] != null)
{
_livetv.HardwareVersion = new VersionInfo(jobject["hwVersion"].Value<string>());
hasValidObject = true;
}
if (jobject["ltvVersion"] != null)
{
_livetv.LTV2Version = new VersionInfo(jobject["ltvVersion"].Value<string>());
hasValidObject = true;
}
if (jobject["ltv3Version"] != null)
{
_livetv.LTV3Version = new VersionInfo(jobject["ltv3Version"].Value<string>());
hasValidObject = true;
}
if (jobject["cricVersion"] != null)
{
_livetv.KAVersion = new VersionInfo(jobject["cricVersion"].Value<string>());
hasValidObject = true;
}
if (jobject["bbVersion"] != null)
{
_livetv.BasebandVersion = new VersionInfo(jobject["bbVersion"].Value<string>());
hasValidObject = true;
}
if (hasValidObject)
{
return GenerateSuccessful();
}
return GenerateUnsuccessful(
"Unable to parse version from request, try again.");
};
Solution
We can of course try to optimize it but this would be only an ugly workaround trying to minimize the flaws of the current design.
There are more questions then answers:
- Why is the the parameter
dynamic
? - Are you trying to parse json?
JObject
suggests you are and I’m pretty sure this is the wrong way to do it. - Why do you use such a crazy syntax for a method? The expression bodied members are meant for one-liners and not for such huge blocks.
- Why don’t you deserialize the object with
JsonConvert.Deserialize
? - Why don’t you decorate the properties like
SoftwareVersion
with the appropriate attribute like[JsonProperty(PropertyName = "swVersion")]
You should rethink the entire parsing process and not just this helper method.
The real problem is you’re rewriting your own deserialization routine instead of using the existing features in library. I think you’re using this http://www.newtonsoft.com/json.
Assuming _livetv
property has class LiveTv
, you need to construct the class like snippet below.
public class LiveTv
{
[JsonProperty(PropertyName = "swVersion")]
public string SoftwareVersion {get; set;}
[JsonProperty(PropertyName = "hwVersion")]
public string HardwareVersion {get; set;}
[JsonProperty(PropertyName = "ltvVersion")]
public string LTV2Version {get; set;}
[JsonProperty(PropertyName = "ltv3Version")]
public string LTV3Version {get; set;}
[JsonProperty(PropertyName = "cricVersion")]
public string KAVersion {get; set;}
[JsonProperty(PropertyName = "bbVersion")]
public string BasebandVersion {get; set;}
public bool HasValidObject()
{
return !(string.IsNullOrEmpty(SoftwareVersion)
&& string.IsNullOrEmpty(HardwareVersion)
&& string.IsNullOrEmpty(LTV2Version)
&& string.IsNullOrEmpty(LTV3Version)
&& string.IsNullOrEmpty(KAVersion)
&& string.IsNullOrEmpty(BasebandVersion));
}
}
Now you can replace the code in VersionMethod()
with below lines of code although I would recommend another method with clear name and call it within VersionMethod()
_livetv = JsonConvert.DeserializeObject<LiveTv>(jObject.ToString());
if(_livetv.HasValidObject())
return GenerateSuccessful();
return GenerateUnsuccessful(
"Unable to parse version from request, try again.");
There are still assumptions here about class structure and you also need to take into account how to manage the overriden method if you’re using it for polymorphism. Hope this helps.