This post is part of the “Legacy Code to Testable Code” series. In the series we’ll talk about making refactoring steps before writing unit tests for legacy code, and how they make our life easier. Other posts include:
The Legacy Code To Testable Code Series
General patterns | Accessibility | Dealing with dependencies | Advanced patterns |
---|---|---|---|
Introduction | Add setters | Extract method | Static constructors (initializers) |
Renaming | More accessors | Extract class | More static constructors |
Add overload | Introduce parameter | Instance constructors | |
Testable object | Conditionals to guard blocks |
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 (excuse the pun).
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. Refactoring 101.
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, we should probably unit test. It would be easier to unit test, because it’s smaller. If we keep it private, 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, unit test both separately 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:
Refactoring time! It makes sense to extract the validation:
If we keep it like that, testing the validation is problematic. Instead, we can extract the method into separate class called StreetValidator:
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:
currentAddress is a field in our class, so it’s easy to extract it into a private method:
However, extracting this into separate class, requires us to pass the currentAddress as a parameter. Refactoring time again.
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:
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 unit tests. Instead of the combinatoric order code paths that we need to unit test, 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.
2 Comments
Pablo RM · June 20, 2019 at 2:25 pm
I think this code has typos:
public void UpdateStreet(string newStreet)
{
if(string.IsNullOrEmpty(street) &&
!street.StartsWith(” “) &&
!street.StartsWith(“@”))
{
Address address = new Address(this);
address.SetStreet(newStreet);
}
}
Every call to street should be to newStreet, isn’t it?
Gil Zilberfeld · June 20, 2019 at 4:28 pm
That is correct Pablo! Thanks for spotting it. Fixed (and moved the examples to gist on the way).