This post is part of the “Legacy Code to Testable Code” series. In the series we’ll talk about making refactoring steps before writing tests for legacy code, and how they make our life easier.
A few years ago I got this from Erik Talboom: “A private method is a design smell”. It took me a while to fully understand it and to apply it.
There’s a nice logic in there.
If at some point, we’re extracting a private method from inside a public method, it probably means the public method did too much. It was long, and procedural, and probably made a number of operations. Under those circumstances, it made sense to extract some of the logic into the private method. It made sense that we could name it more clearly, as the extracted method was simpler.
It also means that the public method broke the Single Responsibility Principle. After all, we just broke it in two (at least). If that’s the case, the private method we extracted contains a separate functionality from the rest of the public method.
This functionality we extracted should probably be tested. It would be easier to test, because it’s smaller. If we keep it private, but called from the public method, we’d prefer not to test it directly, but through the public interface. If however we extract it to a new class, testing both will be easier, because both are simpler.
Testing and simplicity go hand in hand, and extracting into a separate class makes sense in so many cases. Too bad it’s not applied often.
Here’s a simple example:
public void UpdateStreet(string newStreet)
{
if(string.IsNullOrEmpty(street) &&
!street.StartsWith(" ") &&
!street.StartsWith("@"))
{
Address address = new Address(this);
address.SetStreet(newStreet);
}
}It makes sense to extract the validation:
public void UpdateStreet(string newStreet)
{
if (ValidateStreet(newStreet))
{
Address address = new Address(this);
address.SetStreet(newStreet);
}
}
private bool ValidateStreet(string street)
{
return string.IsNullOrEmpty(street) &&
!street.StartsWith(" ") &&
!street.StartsWith("@");
}If we keep it like that, testing the validation is problematic. Instead, we can extract the method into separate class called StreetValidator:
public void UpdateStreet(string newStreet)
{
if (StreetValidator.Validate(newStreet))
{
Address address = new Address(this);
address.SetStreet(newStreet);
}
}Now we can test the Validate method, and then the original UpdateStreet method separately.
We could also expose the method as public, or make it static since it doesn’t change any state. However, this may not always be the case. Sometimes in order to perform the separation, we need to actually cut the cord.
Suppose that our validation now includes comparison to the current address’ street:
public void UpdateStreet(string newStreet)
{
if(string.IsNullOrEmpty(street) &&
!street.StartsWith(" ") &&
!street.StartsWith("@")) &&
currentAddress.GetStreet().CompareTo(street) == 0)
{
Address address = new Address(this);
address.SetStreet(newStreet);
}
}currentAddress is a field in our class, so it’s easy to extract it into a private method:
private bool ValidateStreet(string street)
{
return string.IsNullOrEmpty(street) &&
!street.StartsWith(" ") &&
!street.StartsWith("@") &&
currentAddress.GetStreet().CompareTo(street) == 0;
}However, extracting this into separate class, requires us to pass the currentAddress as a parameter. We can do this in two steps. First, we change the signature of the method, and add a parameter with the same name as the field:
private bool ValidateStreet(string street, Address currentAddress)
{
return string.IsNullOrEmpty(street) &&
!street.StartsWith(" ") &&
!street.StartsWith("@") &&
currentAddress.GetStreet().CompareTo(street) == 0;
}Now that we “shadowed” the field, we decoupled the method from its class. The method can now be extracted to a separate class.
I find people accept extract class (if it’s safe and easy) more than exposing methods, or creating accessors. The effect is the same (and the risk that someone will call it is the same), but the simplicity makes it more bearable, I guess.
Extracting a class reduces the complexity of the code and testing. Instead of the combinatorial order code paths that need testing, we lowered the test cases into linear order. Testing is not only possible, but also more probable – we are always likely to test more when the there is less to test.
That trick we did when we added the parameter? We’ll discuss it with more details next.
| Reference: | Legacy Code to Testable Code #5: Extract Class from our JCG partner Gil Zilberfeld at the Geek Out of Water blog. |
