Is this fix to "PostSharp complains about CA1800:DoNotCastUnnecessarily" the best one?
- by cad
This question is about "is" and "as" in casting and about CA1800 PostSharp rule. I want to know if the solution I thought is the best one possible or if it have any problem that I can't see.
I have this code (named OriginaL Code and reduced to the minimum relevant). The function ValidateSubscriptionLicenceProducts try to validate a SubscriptionLicence (that could be of 3 types: Standard,Credit and TimeLimited ) by casting it and checking later some stuff (in //Do Whatever).
PostSharp complains about CA1800:DoNotCastUnnecessarily. The reason is that I am casting two times the same object to the same type. This code in best case will cast 2 times (if it is a StandardLicence) and in worst case 4 times (If it is a TimeLimited Licence). I know is possible to invalidate rule (it was my first approach), as there is no big impact in performance here, but I am trying a best approach.
//Version Original Code
//Min 2 casts, max 4 casts
//PostSharp Complains about CA1800:DoNotCastUnnecessarily
private void ValidateSubscriptionLicenceProducts(SubscriptionLicence licence)
{
if (licence is StandardSubscriptionLicence)
{
// All products must have the same products purchased
List<StandardSubscriptionLicenceProduct> standardProducts = ((StandardSubscriptionLicence)licence).SubscribedProducts;
//Do whatever
}
else if (licence is CreditSubscriptionLicence)
{
// All products must have a valid Credit entitlement & Credit interval
List<CreditSubscriptionLicenceProduct> creditProducts = ((CreditSubscriptionLicence)licence).SubscribedProducts;
//Do whatever
}
else if (licence is TimeLimitedSubscriptionLicence)
{
// All products must have a valid Time entitlement
// All products must have a valid Credit entitlement & Credit interval
List<TimeLimitedSubscriptionLicenceProduct> creditProducts = ((TimeLimitedSubscriptionLicence)licence).SubscribedProducts;
//Do whatever
}
else
throw new InvalidSubscriptionLicenceException("Invalid Licence type");
//More code...
}
This is Improved1 version using "as". Do not complain about CA1800 but the problem is that it will cast always 3 times (if in the future we have 30 or 40 types of licences it could perform bad)
//Version Improve 1
//Minimum 3 casts, maximum 3 casts
private void ValidateSubscriptionLicenceProducts(SubscriptionLicence licence)
{
StandardSubscriptionLicence standardLicence = Slicence as StandardSubscriptionLicence;
CreditSubscriptionLicence creditLicence = Clicence as CreditSubscriptionLicence;
TimeLimitedSubscriptionLicence timeLicence = Tlicence as TimeLimitedSubscriptionLicence;
if (Slicence == null)
{
// All products must have the same products purchased
List<StandardSubscriptionLicenceProduct> standardProducts = Slicence.SubscribedProducts;
//Do whatever
}
else if (Clicence == null)
{
// All products must have a valid Credit entitlement & Credit interval
List<CreditSubscriptionLicenceProduct> creditProducts = Clicence.SubscribedProducts;
//Do whatever
}
else if (Tlicence == null)
{
// All products must have a valid Time entitlement
// All products must have a valid Credit entitlement & Credit interval
List<TimeLimitedSubscriptionLicenceProduct> creditProducts = Tlicence.SubscribedProducts;
//Do whatever
}
else
throw new InvalidSubscriptionLicenceException("Invalid Licence type");
//More code...
}
But later I thought in a best one. This is the final version I am using.
//Version Improve 2
// Min 1 cast, Max 3 Casts
// Do not complain about CA1800:DoNotCastUnnecessarily
private void ValidateSubscriptionLicenceProducts(SubscriptionLicence licence)
{
StandardSubscriptionLicence standardLicence = null;
CreditSubscriptionLicence creditLicence = null;
TimeLimitedSubscriptionLicence timeLicence = null;
if (StandardSubscriptionLicence.TryParse(licence, out standardLicence))
{
// All products must have the same products purchased
List<StandardSubscriptionLicenceProduct> standardProducts = standardLicence.SubscribedProducts;
//Do whatever
}
else if (CreditSubscriptionLicence.TryParse(licence, out creditLicence))
{
// All products must have a valid Credit entitlement & Credit interval
List<CreditSubscriptionLicenceProduct> creditProducts = creditLicence.SubscribedProducts;
//Do whatever
}
else if (TimeLimitedSubscriptionLicence.TryParse(licence, out timeLicence))
{
// All products must have a valid Time entitlement
List<TimeLimitedSubscriptionLicenceProduct> timeProducts = timeLicence.SubscribedProducts;
//Do whatever
}
else
throw new InvalidSubscriptionLicenceException("Invalid Licence type");
//More code...
}
//Example of TryParse in CreditSubscriptionLicence
public static bool TryParse(SubscriptionLicence baseLicence, out CreditSubscriptionLicence creditLicence)
{
creditLicence = baseLicence as CreditSubscriptionLicence;
if (creditLicence != null)
return true;
else
return false;
}
It requires a change in the classes StandardSubscriptionLicence, CreditSubscriptionLicence and TimeLimitedSubscriptionLicence to have a "tryparse" method (copied below in the code). This version I think it will cast as minimum only once and as maximum three. What do you think about improve 2? Is there a best way of doing it?
