In Working Effectively with Legacy Code Michael Feathers describes legacy code as "code without tests". I describe it as the crap I wrote 4 years ago.
The code block that I was working with looked like the image to the left. It was a single method that was 278 lines long, had a cyclomatic complexity of 80 and (according to DevExpress' DXCore) a horrific maintenance complexity of 1766. The method was used to validate warranty claims and had conditional branching for "Save", "Get Price Details", and "Place". In addition, the "Get Price Details" and "Place" blocks had conditional statements to handle cases for specific countries.
The first step in refactoring this to something resembling a maintainable state was to extract the method in to it's own WarrantyValidator class. This meant creating a constructor that accepted all of the variables and fields from the original object that would be required to do the validation. In this case I needed the Warranty Claim being validated, the user's country, a list to store the failure messages in (a discussion for another day) and the number of items on the claim.
The end result is a class that looked like this (this is "blog" code)
public class ValidateClaim
{
private readonly WarrantyClaim ClaimToValidate;
private int NumberOfItemsOnClaim;
private readonly string CountryCode;
private readonly List<string> ErrorCodes;
public ValidateClaim(WarrantyClaim claimToValidate, int numberOfItemsOnClaim, string countryCode, List<string> errorCodes)
{
ClaimToValidate = claimToValidate;
NumberOfItemsOnClaim = numberOfItemsOnClaim;
CountryCode = countryCode;
ErrorCodes = errorCodes;
}
public bool Validate(string ActionName)
{
if (ActionName == "SAVE")
{
//save-specific validation for example
if (string.IsNullOrEmpty(ClaimToValidate.AuthorizationNumber))
{
ErrorCodes.Add("12345678-1234-1234-123456789012");
}
}
else if (ActionName == "PRICE")
{
//pricing-specifi validation
if (CountryCode == "US")
{
//us specific validation
}
else if (CountryCode == "GB")
{
//gb specific valudation
}
}
else if (ActionName == "PLACE")
{
if (CountryCode == "US")
{
//us specific validation
}
else if (CountryCode == "GB")
{
//gb specific valudation
}
}
/*
* lots of validation no matter what the action is
* ...
* ...
* ...
*/
return ErrorCodes.Count == 0;
}
}
The next step (which I guess probably should have been the first) was generate tests to cover as much code as possible. In doing this I found that I needed a couple of additional parameters in my constructor as data that was exposed through the properties of the WarrantyClaim wasn't available without banging against the database. At the end of this phase, I had managed to get about 95% test coverage, and some analysis of the WarrantyClaim suggested that a couple of cases for which validation code existed were no longer possible. With 76 "green" tests I began breaking up the object and re-running the tests for verification after every change.
The first change was to create the WarrantyValidatorBase. This abstract class contained a constructor identical to my WarrantyValidator class and an abstract method called Validate.
public abstract class WarrantyValidatorBase
{
protected readonly WarrantyClaim ClaimToValidate;
protected int NumberOfItemsOnClaim;
protected readonly string CountryCode;
protected readonly List<string> ErrorCodes;
public WarrantyValidatorBase(WarrantyClaim claimToValidate, int numberOfItemsOnClaim, string countryCode, List<string> errorCodes)
{
ClaimToValidate = claimToValidate;
NumberOfItemsOnClaim = numberOfItemsOnClaim;
CountryCode = countryCode;
ErrorCodes = errorCodes;
}
public abstract void Validate();
}From here I moved all of the Save validation to it's own class, and repeated this with the Price Details and Place validation. Next, I extracted the country specific validation into separate classes - WarrantyPlaceValidationUS, WarrantyPlaceValidationGB, etc.
public class WarrantyPlaceValidator : WarrantyValidatorBase
{
public WarrantyPlaceValidator(WarrantyClaimWarrantyClaim claimToValidate, int numberOfItemsOnClaim,
string countryCode, List<string> errorCodes)
: base(claimToValidate, numberOfItemsOnClaim, countryCode, errorCodes)
{
}
public override void Validate()
{
//place specific validation
if (CountryCode == "US")
{
new WarrantyPlaceValidatorUS(ClaimToValidate, NumberOfItemsOnClaim, CountryCode, ErrorCodes).Validate();
}
else if (CountryCode == "GB")
{
//gb specific valudation
new WarrantyPlaceValidatorGB(ClaimToValidate, NumberOfItemsOnClaim, CountryCode, ErrorCodes).Validate();
}
}
}
public class WarrantyPlaceValidatorUS : WarrantyValidatorBase
{
public WarrantyPlaceValidatorUS(WarrantyClaim claimToValidate, int numberOfItemsOnClaim,
string countryCode, List<string> errorCodes)
: base(claimToValidate, numberOfItemsOnClaim, countryCode, errorCodes)
{
}
public override void Validate()
{
//us specific validation
}
}
public class WarrantyPlaceValidatorGB : WarrantyValidatorBase
{
public WarrantyPlaceValidatorGB(WarrantyClaimclaimToValidate, int numberOfItemsOnClaim,
string countryCode, List<string> errorCodes)
: base(claimToValidate, numberOfItemsOnClaim, countryCode, errorCodes)
{
}
public override void Validate()
{
//GB specific validation
}
}
After this refactoring, my original diagram looked a little like this:
Again, after each change, I re-ran my tests and made sure that everything was still ok.
So far, so good. The new code was a far easier to maintain than the previous approach. I now had small testable classes, which were easy to read and instantly understandable. They all had very specific purposes and were very loosely coupled. However, there was still an awful lot of duplication in the constructors, and I was not happy with the country specific cases being handled this way.
I knew this needed to be resolved, but I was a little stuck. I am pretty clueless when it comes to design patterns and to be perfectly honest, the GoF book makes me want to gouge my eyes out. Fortunately, the Head First book and Alex Henderson's Windsor tutorial gave me a little insight into the Decorator pattern. Time for some more refactoring...
The WarrantyValidatorBase ended up looking like this.
public abstract class WarrantyValidatorBase
{
private List<string> _ErrorPhrases;
private WarrantyClaim _WarrantyClaim;
private WarrantyValidatorBase _ChildValidator;
private int _NumberOfPartsInBasket;
public WarrantyValidatorBase()
{
}
public WarrantyValidatorBase(WarrantyValidatorBase decorateWith)
{
_ChildValidator = decorateWith;
}
protected void AddErrorId(string errorGuid)
{
_ErrorPhrases.Add(errorGuid);
}
internal ToolCommerce.Warranty WarrantyClaim
{
get { return _WarrantyClaim; }
set { _WarrantyClaim = value; }
}
internal List<string> Phrases
{
get { return _ErrorPhrases; }
set { _ErrorPhrases = value; }
}
internal int NumberOfPartsInBasket
{
get { return _NumberOfPartsInBasket; }
set { _NumberOfPartsInBasket = value; }
}
internal WarrantyValidatorBase ChildValidator
{
get { return _ChildValidator; }
set { _ChildValidator = value; }
}
public abstract void Validate();
protected virtual void ValidateChild()
{
if (_ChildValidator!=null)
{
_ChildValidator.Phrases = _ErrorPhrases;
_ChildValidator.WarrantyClaim = _WarrantyClaim;
_ChildValidator.NumberOfPartsInBasket = _NumberOfPartsInBasket;
_ChildValidator.Validate();
}
}
}
Note the addition of the _ChildValidator field and the constructor changes to accept another object deriving from WarrantyValidatorBase. I also moved the arguments from the constructor to properties - this just seemed a little cleaner to me. The ValidateChild method is key here. It passes the validation request down the "decorator chain" to its ChildValidator, and its ChildValidator to its ChildValidator and so on. This approach allowed me to change my WarrantyPlaceValidator to this:
public class WarrantyPlaceValidator : WarrantyValidatorBase
{
public WarrantyPlaceValidator(WarrantyValidatorBase childValidation)
{
ChildValidator = childValidation;
}
public override void Validate()
{
//place specific validation
//notice that there is nothing specific to countries in here any more
ValidateChild();
}
}
End Result
The awesomeness of the decorator is that I can chain these together and add functionality without having to clutter up my classes with conditionals and branching logic.
public void ValidateForPlace(Warranty claimToValidate, int numberOfItemsOnClaim, string countryCode, List<string> errorCodes)
{
WarrantyValidatorBase validator = null;
if (countryCode == "US")
{
validator = new WarrantyValidator(new WarrantyPlaceValidator(new WarrantyPlaceValidatorUS()));
}
else if (countryCode == "GB")
{
validator = new WarrantyValidator(new WarrantyPlaceValidator(new WarrantyPlaceValidatorGB()));
}
else
{
//no country specific validation
validator = new WarrantyValidator(new WarrantyPlaceValidator());
}
validator.NumberOfPartsInBasket = numberOfItemsOnClaim;
validator.Phrases = errorCodes;
validator.WarrantyClaim = claimToValidate;
validator.Validate();
}
If new functionality is required, it can be encapsulated and then "chained" in.