Welcome back to the future-proof series! Today I thought I would present to you a case very close to a code refactoring I made recently during a piece of work for a client. You deserve it as it has been a very long time since my last truly technical post with code and all. Also, I told myself about a year ago when I badly explained refactoring that I should write a post going through an example so here we are. Two birds with one stone. (I’m not doing that one scone stuff)
Once upon a time, an enum, actually a couple, were used to drive access to a bunch of classes through a factory. Nothing wrong with that. I think I read Uncle Bob’s clean code that it is where enum should stay. Not scattered around software with hordes of if/else statements nor switches. Behaviour should be into objects and subclasses should contain exceptions to a certain behaviour. An enum can then be used to represent a constant value. Integers originally but now we can create enums based off a boolean or a string to represent certain cases.
Obviously, the example presented in this post only has a shape similar to what I had to deal with. I did change the domain and present a much simpler and smaller codebase you will be able to find at the end of the post. Let me take you on this journey. A journey to clean code which will result in surprises because I won’t spoil the ending after two paragraphs.
Setting up our code refactoring context
The plot is quite straightforward, we get a bunch of animals grouped by family. For a family of animals, we want to display the average position above the sea. Also, we want to display the list of animals per type along with their own average position above the sea in meters.
A product owner decided it was time to introduce a coding animal family to show information about some coding animals. However, the codebase is quite hostile and duplication ridden. Some work needs to go in so that we can add new families or features at a lower cost.
The original model
You can find the pre-refactoring codebase over there. I invite you to have a look before getting any further. But luckily for you, I have a class diagram and will show the relevant bits of code to deal with.
Round 0: On starting a code refactoring without nets
All code refactorings need to start somewhere, however, this codebase has no tests whatsoever. The first thing to do is to observe and see what can be done to make the code testable. If you cannot test your code you cannot ensure that you maintain the integrity of its intended behaviour as you refactor.
Spotting something easy to tackle at first
Starting a code refactoring requires discipline. You cannot go gun-blazing at a codebase touching everything at once. And yet you are free to do so, as you are free to leave software development to those who won’t do that. You need to pick a piece small enough so that it won’t do much damage if things go wrong. The first piece here will be the SeaAnimalPrinter EnumSet duplication. Just look at this class. Look at it!
package animals;
import animals.model.Animal;
import animals.model.AnimalFactory;
import animals.model.SeaAnimalFamily;
import java.util.EnumSet;
public class SeaAnimalPrinter {
private final AnimalFactory animalFactory;
private int averagePositionAboveSea = 0;
public SeaAnimalPrinter(AnimalFactory animalFactory) {
this.animalFactory = animalFactory;
}
public static SeaAnimalPrinter instance(AnimalFactory animalFactory) {
return new SeaAnimalPrinter(animalFactory);
}
public void print() {
printSeaAnimalsSummary();
printSeaAnimalsDetails();
}
private void printSeaAnimalsSummary() {
int animalsCount = EnumSet.allOf(SeaAnimalFamily.class).size();
averagePositionAboveSea = 0;
EnumSet.allOf(SeaAnimalFamily.class).forEach(
t -> averagePositionAboveSea += animalFactory.get(t).averagePositionAboveSea()
);
System.out.println(String.format(
"There are %d land animals. Their average position above sea is %d meters.",
animalsCount, averagePositionAboveSea));
}
private void printSeaAnimalsDetails() {
EnumSet.allOf(SeaAnimalFamily.class).forEach(type -> {
Animal animal = animalFactory.get(type);
System.out.println(String.format(
"The %s has an average position above sea is %d meters.",
animal.name(), animal.averagePositionAboveSea() ));
});
}
}Can you tell what happens in less than 5 seconds? Of course not. However, I suspect you may see some duplication within this class. In printSeaAnimalsSummary and printSeaAnimalsDetails. Did you spot them?
They look something like this:
EnumSet.allOf(SeaAnimalFamily.class).forEach(type -> {
Animal animal = animalFactory.get(type);
// do stuff with animal
});What we get here is that we go through each existing value of the SeaAnimalFamily enum and retrieves an animal from the animal factory and use it. We should definitely extract that piece of code and move that into one place.
If you read Martin Fowler’s Refactoring or any decent computer science book tackling the art of refactoring you know what’s coming. Writing tests. As mentioned previously, there is no attacking angle here and we have to take a risk. We need to make this code testable enough to confidently refactor it.
Round 0.5: Extract the printing to enable injection
Currently, the information gets printed directly into the System.out output stream. What I want to do is extract that to make it easier to test. System.out is an instance of PrintStream so we pass that instead and decouple the printing implementation for our animal printer. This way we can write a test where use a PrintStream mock and test for the values we expect to print. Let’s go!
package animals;
import animals.model.Animal;
import animals.model.AnimalFactory;
import animals.model.SeaAnimalFamily;
import java.io.PrintStream;
import java.util.EnumSet;
public class SeaAnimalPrinter {
private final AnimalFactory animalFactory;
private int averagePositionAboveSea = 0;
private PrintStream printStream;
public SeaAnimalPrinter(AnimalFactory animalFactory, PrintStream printStream) {
this.animalFactory = animalFactory;
this.printStream = printStream;
}
public static SeaAnimalPrinter instance(AnimalFactory animalFactory, PrintStream printStream) {
return new SeaAnimalPrinter(animalFactory, printStream);
}
public void print() {
printSeaAnimalsSummary();
printSeaAnimalsDetails();
}
private void printSeaAnimalsSummary() {
int animalsCount = EnumSet.allOf(SeaAnimalFamily.class).size();
averagePositionAboveSea = 0;
EnumSet.allOf(SeaAnimalFamily.class).forEach(
t -> averagePositionAboveSea += animalFactory.get(t).averagePositionAboveSea()
);
printStream.println(String.format(
"There are %d land animals. Their average position above sea is %d meters.",
animalsCount, averagePositionAboveSea));
}
private void printSeaAnimalsDetails() {
EnumSet.allOf(SeaAnimalFamily.class).forEach(type -> {
Animal animal = animalFactory.get(type);
printStream.println(String.format(
"The %s has an average position above sea is %d meters.",
animal.name(), animal.averagePositionAboveSea() ));
});
}
}From here, we need to update our App class to pass System.out to our SeaAnimalPrinter. Relatively straightforward so far.
package animals;
import animals.model.AnimalFactory;
public class App {
private final LandAnimalPrinter landAnimalPrinter;
private final SeaAnimalPrinter seaAnimalPrinter;
public App(AnimalFactory animalFactory) {
this.landAnimalPrinter = LandAnimalPrinter.instance(animalFactory);
this.seaAnimalPrinter = SeaAnimalPrinter.instance(animalFactory, System.out);
}
public void run() {
landAnimalPrinter.print();
seaAnimalPrinter.print();
}
public static void main(String[] args) {
new App(AnimalFactory.instance()).run();
}
}That’s already better. Now let’s add the mocking framework and write our test.
Round 1: Buying ourself protection a.k.a. writing tests
First things first, let’s add a mocking framework. If you checked out the repository you know that the project was created using Gradle so we’ll go to the Gradle build file and add the dependency there. Obviously, I will use Mockito because I know it well enough.
Pretty straightforward to set as you can see below, I’ll update dependencies as below.
dependencies {
// This dependency is used by the application.
implementation 'com.google.guava:guava:27.1-jre'
// Use JUnit test framework
testImplementation 'junit:junit:4.12'
testCompile group: 'org.mockito', name: 'mockito-core', version: '2.23.4'
}Now let’s define our test. Keep in mind that here the goal is to maintain the behaviour of the app before you make significant changes. When you run the App you get the following output:
These following lines are printed by the SeaAnimalPrinter class in the same order every single time.
There are 2 land animals. Their average position above sea is -20100 meters.
The Blue Fish has an average position above sea is -100 meters.
The Kraken has an average position above sea is -20000 meters.Now we know that we need a test that makes sure the sea animal printer calls printStream.println with each of these lines. Now I will add a test class originally named SeaAnimalPrinterTest. Yep, I’m that guy.
package animals;
import animals.model.AnimalFactory;
import org.junit.Test;
import org.mockito.InOrder;
import java.io.PrintStream;
import static org.mockito.Mockito.*;
public class SeaAnimalPrinterTest {
@Test
public void print_printsWhatWeExpect() {
PrintStream mockedInputStream = mock(PrintStream.class);
InOrder orderChecker = inOrder(mockedInputStream);
SeaAnimalPrinter printer =
SeaAnimalPrinter.instance(AnimalFactory.instance(), mockedInputStream);
printer.print();
orderChecker.verify(mockedInputStream)
.println("There are 2 land animals. Their average position above sea is -20100 meters.");
orderChecker.verify(mockedInputStream)
.println("The Blue Fish has an average position above sea is -100 meters.");
orderChecker.verify(mockedInputStream)
.println("The Kraken has an average position above sea is -20000 meters.");
}
}From here, you can execute your test either with the Gradle wrapper test command or using your IDE. Here is what I get from IntelliJ:
InOrder allows making sure the calls are made in the order we expect them to be made. You can try and swap the calls in the test and see it fail as it should.
Round 1.5: Back to the EnumSet iteration code refactoring
Now that we have our test, we can head back to our first piece of refactoring. The duplicated part that boils down to this snippet.
EnumSet.allOf(SeaAnimalFamily.class).forEach(type -> {
Animal animal = animalFactory.get(type);
// do stuff with animal
});The code takes more cognitive load than needed to understand. After enough reading, you understand that this code basically accesses each sea animal instance and does stuff with it. The first thing to do would be adding a getSeaAnimals method into AnimalFactory to retrieve all the sea animals at once. Here is our factory code after adding that method.
package animals.model;
public class AnimalFactory {
private static AnimalFactory instance;
Dog dog;
Spider spider;
BlueFish blueFish;
Kraken kraken;
public static AnimalFactory instance() {
if (instance == null) {
instance = new AnimalFactory();
}
return instance;
}
private AnimalFactory() {
dog = new Dog();
spider = new Spider();
blueFish = new BlueFish();
kraken = new Kraken();
}
public Animal get(AnimalFamily type) {
switch (type.description()) {
case "Doggo": return dog;
case "Spider": return spider;
case "Blue Fish": return blueFish;
case "Kraken": return kraken;
default: throw new IllegalArgumentException(type.description());
}
}
public Animal[] getSeaAnimals() {
return new Animal[] {
kraken,
blueFish
};
}
}Now we can change our methods like this:
private void printSeaAnimalsDetails() {
for (Animal animal : animalFactory.getSeaAnimals()) {
printStream.println(String.format(
"The %s has an average position above sea is %d meters.",
animal.name(), animal.averagePositionAboveSea() ));
}
}Alright, let’s run our test again.
Remember that the test is ensuring the information gets printed in the right order? It expects the blue fish description before the Kraken but I wrote them in the wrong order. Let’s fix this now.
public Animal[] getSeaAnimals() {
return new Animal[] {
blueFish,
kraken
};
}And now we’re green again.
Let’s apply the same thing to the marine version.
private void printSeaAnimalsSummary() {
int animalsCount = 0;
int averagePositionAboveSea = 0;
for (Animal animal : animalFactory.getSeaAnimals()) {
animalsCount++;
averagePositionAboveSea += animal.averagePositionAboveSea();
}
printStream.println(String.format(
"There are %d land animals. Their average position above sea is %d meters.",
animalsCount, averagePositionAboveSea));
}
private void printSeaAnimalsDetails() {
for (Animal animal : animalFactory.getSeaAnimals()) {
printStream.println(String.format(
"The %s has an average position above sea is %d meters.",
animal.name(), animal.averagePositionAboveSea() ));
}
}Finally, we got rid of the extra step of going over an EnumSet to get all sea animal instances. However, it still feels like I can remove some more duplication here.
Round 1 3/4: Bringing in the Consumers
So since Java 8 came out about five years ago, we have access to something beautiful. Something that helps refactor further places where we iterate over the same collection in different places. Consumer but with a big C. They pretty much replace what would have been an ugly cluttered anonymous class. Before that, you would need to create an interface and write an implementation in the middle of your code which you make it harder to read than the EnumSet monstrosity from earlier.
I will need to introduce a doThatThingWithAnimals method to put that logic in. After a few adjustments, our SeaAnimalPrinter will look like this:
package animals;
import animals.model.Animal;
import animals.model.AnimalFactory;
import java.io.PrintStream;
import java.util.function.Consumer;
public class SeaAnimalPrinter {
private final AnimalFactory animalFactory;
private PrintStream printStream;
private int averagePositionAboveSea;
private int animalsCount;
public SeaAnimalPrinter(AnimalFactory animalFactory, PrintStream printStream) {
this.animalFactory = animalFactory;
this.printStream = printStream;
}
public static SeaAnimalPrinter instance(AnimalFactory animalFactory, PrintStream printStream) {
return new SeaAnimalPrinter(animalFactory, printStream);
}
public void print() {
printSeaAnimalsSummary();
printSeaAnimalsDetails();
}
private void printSeaAnimalsSummary() {
animalsCount = 0;
averagePositionAboveSea = 0;
doThatThingWithAnimals(animal ->
{
animalsCount++;
averagePositionAboveSea += animal.averagePositionAboveSea();
}
);
printStream.println(String.format(
"There are %d land animals. Their average position above sea is %d meters.",
animalsCount, averagePositionAboveSea));
}
private void printSeaAnimalsDetails() {
doThatThingWithAnimals(animal ->
printStream.println(String.format(
"The %s has an average position above sea is %d meters.",
animal.name(), animal.averagePositionAboveSea() ))
);
}
private void doThatThingWithAnimals (Consumer<Animal> thing) {
for (Animal animal : animalFactory.getSeaAnimals()) {
thing.accept(animal);
}
}
}I am speeding up because I passed the thousand words mark and still not halfway through. Following the same path, we will end up with the following LandAnimalPrinter.
package animals;
import animals.model.Animal;
import animals.model.AnimalFactory;
import java.io.PrintStream;
import java.util.function.Consumer;
public class LandAnimalPrinter {
private final AnimalFactory animalFactory;
private PrintStream printStream;
private int averagePositionAboveSea;
private int animalsCount;
public LandAnimalPrinter(AnimalFactory animalFactory, PrintStream printStream) {
this.animalFactory = animalFactory;
this.printStream = printStream;
}
public static LandAnimalPrinter instance(AnimalFactory animalFactory, PrintStream printStream) {
return new LandAnimalPrinter(animalFactory, printStream);
}
public void print() {
printLandAnimalsSummary();
printLandAnimalsDetails();
}
private void printLandAnimalsSummary() {
animalsCount = 0;
averagePositionAboveSea = 0;
doThatThingWithAnimals(animal ->
{
animalsCount++;
averagePositionAboveSea += animal.averagePositionAboveSea();
}
);
printStream.println(String.format(
"There are %d land animals. Their average position above sea is %d meters.",
animalsCount, averagePositionAboveSea));
}
private void printLandAnimalsDetails() {
doThatThingWithAnimals(animal ->
printStream.println(String.format(
"The %s has an average position above sea is %d meters.",
animal.name(), animal.averagePositionAboveSea() ))
);
}
private void doThatThingWithAnimals (Consumer<Animal> thing) {
for (Animal animal : animalFactory.getLandAnimals()) {
thing.accept(animal);
}
}
}Of course, we will also have a matching test class:
package animals;
import animals.model.AnimalFactory;
import org.junit.Test;
import org.mockito.InOrder;
import java.io.PrintStream;
import static org.mockito.Mockito.inOrder;
import static org.mockito.Mockito.mock;
public class LandAnimalPrinterTest {
@Test
public void print_printsWhatWeExpect() {
PrintStream mockedInputStream = mock(PrintStream.class);
InOrder orderChecker = inOrder(mockedInputStream);
LandAnimalPrinter printer =
LandAnimalPrinter.instance(AnimalFactory.instance(), mockedInputStream);
printer.print();
orderChecker.verify(mockedInputStream)
.println("There are 2 land animals. Their average position above sea is 12 meters.");
orderChecker.verify(mockedInputStream)
.println("The Doggo has an average position above sea is 10 meters.");
orderChecker.verify(mockedInputStream)
.println("The Spider has an average position above sea is 2 meters.");
}
}Now the animal factory method can be completely removed.
public Animal get(AnimalFamily type) {
switch (type.description()) {
case "Doggo": return dog;
case "Spider": return spider;
case "Blue Fish": return blueFish;
case "Kraken": return kraken;
default: throw new IllegalArgumentException(type.description());
}
}Let’s run our tests for good measure.
This completes the first round of code refactoring by which we completely got rid of a method hiding the application logic. Let’s take a look at our model.
As you can see, our printer classes look heavier in there despite having cleaner code. The methods are basically the same. Very similar names, pretty much the same behaviour. If there is a bat signal for code refactoring this is it. The enumerations and animal family abstraction are still there despite their questionable utility. You may take a break and come back later for round 2.
Round 2: Pulling up external dependencies
If you think I won’t put your brain to work in this post, you clearly haven’t been paying attention. We are now getting into the fun stuff. Basically, we will merge the common part of classes doing roughly the same thing to clean up the code. Keeping only very specific details into subclasses.
The animals’ iteration
In both SeaAnimalPrinter and LandAnimalPrinter we have a method that loops over animals and executes a consumer method over each of them. That will be the first one we move up to our new superclass. First, we will split it in two by extracting the method to get animals specific to an environment. Here land animals but the same applies to sea animals.
We go from:
private void doThatThingWithAnimals (Consumer<Animal> thing) {
for (Animal animal : animalFactory.getLandAnimals()) {
thing.accept(animal);
}
}To this.
private void doThatThingWithAnimals (Consumer<Animal> thing) {
for (Animal animal : getAnimals()) {
thing.accept(animal);
}
}
private Animal[] getAnimals() {
return animalFactory.getLandAnimals();
}Now we can move doThatThingWithAnimals to AnimalPrinter.We also need to define getAnimals as an abstract method so that our superclass can reference it. We will make it protected so that only subclasses can access and overwrite it. This is our AnimalPrinter so far:
package animals;
import animals.model.Animal;
import java.util.function.Consumer;
public abstract class AnimalPrinter {
protected void doThatThingWithAnimals (Consumer<Animal> thing) {
for (Animal animal : getAnimals()) {
thing.accept(animal);
}
}
protected abstract Animal[] getAnimals();
}Now you can safely delete the subclasses versions of the doThatThingWithAnimals method and update the getAnimals definition to match the superclass one. Tests still pass, we’re good to go.
Filling up that AnimalPrinter
The next methods we should pull up are the ones doing the printing. However, before we can get to this we need to pull up the class instance variables into AnimalPrinter. You may have realised that we have very similar if not identical properties.
private final AnimalFactory animalFactory;
private PrintStream printStream;
private int averagePositionAboveSea;
private int animalsCount;These need to go up. First, the animal factory. This one is pretty straightforward. I will start by moving the property and adding a constructor to our AnimalPrinter class to initialise it. All we need to do is aside from moving the property up is creating a getter for the subclasses so they can retrieve the animals to display. Thanks to the code refactoring done during the previous steps there is only one place in each subclass that requires access to an animal factory. It’s the getAnimals method.
Remember as properties and method keep moving around it is extremely important that you keep running your tests. Let’s see how our AnimalPrinter looks like now.
Both the LandAnimalPrinter and the SeaAnimalPrinter have an instance of a PrintStream which they print data to. Nothing special is done to it so we can just pull it up and use a getter in the subclasses. We will need to introduce a constructor to AnimalPrinter so we can set up our PrintStream in one place.
package animals;
import animals.model.Animal;
import animals.model.AnimalFactory;
import java.util.function.Consumer;
public abstract class AnimalPrinter {
private final AnimalFactory animalFactory;
protected AnimalPrinter(AnimalFactory animalFactory) {
this.animalFactory = animalFactory;
}
protected AnimalFactory getAnimalFactory() {
return animalFactory;
}
protected void doThatThingWithAnimals (Consumer<Animal> thing) {
for (Animal animal : getAnimals()) {
thing.accept(animal);
}
}
protected abstract Animal[] getAnimals();
}Now let’s peek at LandAnimalPrinter so you can see how I reflect this change to subclasses.
package animals;
import animals.model.Animal;
import animals.model.AnimalFactory;
import java.io.PrintStream;
public class LandAnimalPrinter extends AnimalPrinter {
private PrintStream printStream;
private int averagePositionAboveSea;
private int animalsCount;
public LandAnimalPrinter(AnimalFactory animalFactory, PrintStream printStream) {
super(animalFactory);
this.printStream = printStream;
}
/* ... */
@Override
protected Animal[] getAnimals() {
return getAnimalFactory().getLandAnimals();
}
}Following a similar pattern we pull up our printStream property and we would have this in our AnimalPrinter.
package animals;
import animals.model.Animal;
import animals.model.AnimalFactory;
import java.io.PrintStream;
import java.util.function.Consumer;
public abstract class AnimalPrinter {
private final AnimalFactory animalFactory;
private final PrintStream printStream;
protected AnimalPrinter(AnimalFactory animalFactory, PrintStream printStream) {
this.animalFactory = animalFactory;
this.printStream = printStream;
}
protected AnimalFactory getAnimalFactory() {
return animalFactory;
}
protected void doThatThingWithAnimals (Consumer<Animal> thing) {
for (Animal animal : getAnimals()) {
thing.accept(animal);
}
}
protected abstract Animal[] getAnimals();
protected PrintStream getPrintStream() {
return printStream;
}
}Round 3: Extract the animal information printing
Now that the external dependencies that enable retrieving animals and printing information have been pulled up. It is time to move the method using these dependencies as they are pretty similar in our land and sea animal printers.
Animal details, yup that’s a detail sorry PETA
So the next one is going to be pretty easy to pull up. It iterates over a bunch of animals and prints their details. It does not require to know what the animal is nor what family it belongs to. The only change required beforehand is rename. You already guessed it I am referring to printLandAnimalsDetails. We need a more generic-sounding name for it like printAnimalsDetails.
private void printAnimalsDetails() {
doThatThingWithAnimals(animal ->
getPrintStream().println(String.format(
"The %s has an average position above sea is %d meters.",
animal.name(), animal.averagePositionAboveSea() ))
);
}This one is a freebie once you renamed both methods from both our sea and land animal printers. You can pull up either as a protected class and delete the other. This is one of the big advantages of code refactoring, the further you go, the easier it gets. Now a method we might have had difficulties extracting in a clean way upfront can be pulled up with ease.
Notice how much smaller our sea animal printer now is.
package animals;
import animals.model.Animal;
import animals.model.AnimalFactory;
import java.io.PrintStream;
public class SeaAnimalPrinter extends AnimalPrinter {
private int averagePositionAboveSea;
private int animalsCount;
public SeaAnimalPrinter(AnimalFactory animalFactory, PrintStream printStream) {
super(animalFactory, printStream);
}
public static SeaAnimalPrinter instance(AnimalFactory animalFactory, PrintStream printStream) {
return new SeaAnimalPrinter(animalFactory, printStream);
}
public void print() {
printSeaAnimalsSummary();
printAnimalsDetails();
}
private void printSeaAnimalsSummary() {
animalsCount = 0;
averagePositionAboveSea = 0;
doThatThingWithAnimals(animal ->
{
animalsCount++;
averagePositionAboveSea += animal.averagePositionAboveSea();
}
);
getPrintStream().println(String.format(
"There are %d land animals. Their average position above sea is %d meters.",
animalsCount, averagePositionAboveSea));
}
@Override
protected Animal[] getAnimals() {
return getAnimalFactory().getSeaAnimals();
}
}And yet we can make it even smaller, as in more focused on its specific job that is printing sea animal details. The remaining properties exist in both our printer subclasses and the code is almost identical. No, wait, what the heck?
Code refactoring bonus: Finding that copypasta bug
It looks like our sea animal printer displays a message about land animals. This seems like the dev who implemented it copied then pasted the land animal printer class. This is what you get with duplication when specific behaviours are required. The code is so messy that you miss issues that should be obvious and fixed before getting merged.
However, sometimes weird things you see in code can be intentional as in your product owner demanded it. In these cases always double-check what the requirement is to make sure you actually found a bug.
The requirement is that the summary for sea animals should not start with:
There are 2 land animals...But with this instead:
There are 2 sea animals...This is the moment where you update your tests to match the new requirement. Now I can run the tests to make sure they the sea animal printer one breaks as expected.
Now we can fix this bug pretty easily by changing the summary part of the text from SeaAnimalPrinter. Our bug is fixed but we can go further.
Pull up everything else! Not much but still…
The bug spotted previously provides deeper insight. As it turns out, the difference between our sea and land animal summary printing is pretty small. The method name for a starter but we can do the same thing we did as for the per animal details printing. On top of that, the difference in templates shows that only a single word changes. We can extract it.
This is where we introduce a family qualifier getter to our animal printer superclass
protected abstract String getFamilyQualifier();Then in our subclasses, we can peacefully remove the qualifiers and inject them from the superclass defined getter. This is what we get in our sea animal printer would look like afterwards.
package animals;
import animals.model.Animal;
import animals.model.AnimalFactory;
import java.io.PrintStream;
public class SeaAnimalPrinter extends AnimalPrinter {
private int averagePositionAboveSea;
private int animalsCount;
public SeaAnimalPrinter(AnimalFactory animalFactory, PrintStream printStream) {
super(animalFactory, printStream);
}
public static SeaAnimalPrinter instance(AnimalFactory animalFactory, PrintStream printStream) {
return new SeaAnimalPrinter(animalFactory, printStream);
}
public void print() {
printAnimalsSummary();
printAnimalsDetails();
}
private void printAnimalsSummary() {
animalsCount = 0;
averagePositionAboveSea = 0;
doThatThingWithAnimals(animal ->
{
animalsCount++;
averagePositionAboveSea += animal.averagePositionAboveSea();
}
);
getPrintStream().println(String.format(
"There are %d %s animals. Their average position above sea is %d meters.",
animalsCount, getFamilyQualifier(), averagePositionAboveSea));
}
@Override
protected String getFamilyQualifier() {
return "sea";
}
@Override
protected Animal[] getAnimals() {
return getAnimalFactory().getSeaAnimals();
}
}You definitely realised that now that both print and printAnimalsSummary almost independent of the sea animal printer specifics. We still have references to a couple of fields.
private int averagePositionAboveSea;
private int animalsCount;Now, notice how these fields are only used within printAnimalsSummary. I am sure you know what this means. You’re damn right! It means I can pull properties and methods up all at once. Which gives us this for the abstract animal printer class:
package animals;
import animals.model.Animal;
import animals.model.AnimalFactory;
import java.io.PrintStream;
import java.util.function.Consumer;
public abstract class AnimalPrinter {
private final AnimalFactory animalFactory;
private final PrintStream printStream;
private int averagePositionAboveSea;
private int animalsCount;
protected AnimalPrinter(AnimalFactory animalFactory, PrintStream printStream) {
this.animalFactory = animalFactory;
this.printStream = printStream;
}
protected AnimalFactory getAnimalFactory() {
return animalFactory;
}
protected void doThatThingWithAnimals (Consumer<Animal> thing) {
for (Animal animal : getAnimals()) {
thing.accept(animal);
}
}
protected void printAnimalsDetails() {
doThatThingWithAnimals(animal ->
getPrintStream().println(String.format(
"The %s has an average position above sea is %d meters.",
animal.name(), animal.averagePositionAboveSea() ))
);
}
protected abstract Animal[] getAnimals();
public void print() {
printAnimalsSummary();
printAnimalsDetails();
}
private void printAnimalsSummary() {
animalsCount = 0;
averagePositionAboveSea = 0;
doThatThingWithAnimals(animal ->
{
animalsCount++;
averagePositionAboveSea += animal.averagePositionAboveSea();
}
);
getPrintStream().println(String.format(
"There are %d %s animals. Their average position above sea is %d meters.",
animalsCount, getFamilyQualifier(), averagePositionAboveSea));
}
protected abstract String getFamilyQualifier();
protected PrintStream getPrintStream() {
return printStream;
}
}All the common logic has been moved up to the superclass which means that now our subclasses focus only on their core.
See SeaAnimalPrinter:
package animals;
import animals.model.Animal;
import animals.model.AnimalFactory;
import java.io.PrintStream;
public class SeaAnimalPrinter extends AnimalPrinter {
public SeaAnimalPrinter(AnimalFactory animalFactory, PrintStream printStream) {
super(animalFactory, printStream);
}
public static SeaAnimalPrinter instance(AnimalFactory animalFactory, PrintStream printStream) {
return new SeaAnimalPrinter(animalFactory, printStream);
}
@Override
protected String getFamilyQualifier() {
return "sea";
}
@Override
protected Animal[] getAnimals() {
return getAnimalFactory().getSeaAnimals();
}
}And LandAnimalPrinter:
package animals;
import animals.model.Animal;
import animals.model.AnimalFactory;
import java.io.PrintStream;
public class LandAnimalPrinter extends AnimalPrinter {
public LandAnimalPrinter(AnimalFactory animalFactory, PrintStream printStream) {
super(animalFactory, printStream);
}
public static LandAnimalPrinter instance(AnimalFactory animalFactory, PrintStream printStream) {
return new LandAnimalPrinter(animalFactory, printStream);
}
@Override
protected String getFamilyQualifier() {
return "land";
}
@Override
protected Animal[] getAnimals() {
return getAnimalFactory().getLandAnimals();
}
}These are much more focused. You can actually see it through the model when zooming on the printers.
That’s what we had before at the end of round 2 after pulling up external dependencies:
This is what we have now:
Now I am able to easily add a coding animal printer, but first I will do some more clean up.
Round 4: The other code refactoring I could have done second but less important
Now we have a neater code structure there is still something awkward going on. The enums. Right now despite all the work put in, I would still need to create a new enum representing the coding animals. Then for each new coding animal, I would need to create a new constant for that enum which sole purpose is access to the animal name as you can see below for Dog.
package animals.model;
import static animals.model.LandAnimalFamily.DOG;
public class Dog implements Animal {
public String name() {
return DOG.description();
}
public int averagePositionAboveSea() {
return 10;
}
}The same applies to every Animal in this project. An enum regardless of the language is a constant used to represent a string or a number of particular significance to a model. Now the factory method relying on these is gone, they become simple values that need to be inlined.
package animals.model;
public enum LandAnimalFamily implements AnimalFamily {
SPIDER("Spider"),
DOG("Doggo");
private final String description;
LandAnimalFamily(String description) {
this.description = description;
}
public String description() {
return description;
}
}From there it becomes even simpler. All we need to do is take the values set in the matching animal enum constructor and use them as the Animal name like this:
package animals.model;
public class Dog implements Animal {
public String name() {
return "Doggo";
}
public int averagePositionAboveSea() {
return 10;
}
}And delete the enum value DOG to then run the tests to make sure nothing broke. Repeating this for each animal you will eventually end up with empty enums that lost meaning and do not fit anymore in our model. See below:
package animals.model;
public enum LandAnimalFamily implements AnimalFamily {
private final String description;
LandAnimalFamily(String description) {
this.description = description;
}
public String description() {
return description;
}
}It is now time to (finally!) delete them. There is something else we can delete now, AnimalFamily. As you will see below, it is only attached to the enums and you may remember its only use was in the factory method to pass the enum value description in order to retrieve the right Animal instance.
All these things are gone. It’s time for that interface to go too alongside its enum spawns. Now that the development effort is cheaper for this time and the next, we can head back to delivering business value efficiently.
Final Round: The reason I went through all this code refactoring like a madman
Congrats for working your way through this post so far! Despite the simplicity of the model, this is definitely one of the heaviest posts I ever wrote. No need to feel bad if you took breaks while reading this blog, I took a few while writing it. Alright, enough back-patting, we’re in the Endgame now.
Creating our empty coding animal printer
First, let’s create the class and have it extend AnimalPrinter. This is what it looks like.
package animals;
import animals.model.Animal;
import animals.model.AnimalFactory;
import java.io.PrintStream;
public class CodingAnimalPrinter extends AnimalPrinter {
protected CodingAnimalPrinter(AnimalFactory animalFactory, PrintStream printStream) {
super(animalFactory, printStream);
}
public static CodingAnimalPrinter instance(AnimalFactory animalFactory,
PrintStream printStream) {
return new CodingAnimalPrinter(animalFactory, printStream);
}
@Override
protected Animal[] getAnimals() {
return new Animal[0];
}
@Override
protected String getFamilyQualifier() {
return null;
}
}From here it’s pretty much free for all within boundaries. You could write the same test as the previous animal printer implementations. However, doing so would force you to write tests that will not pass until after the new Animal and factory method to access it gets implemented. Even though it is not a big deal for a model this small, it is preferable when you have a bigger codebase. Taking things on bit by bit is key.
Here I will write a test that will just make sure we get a basic summary for no animal loaded.
There are 0 coding animals. Their average position above sea is 0 meters.There are many ways to make it better, like changing the code to say “No coding animals.” instead but since this is not part of the requirement it would be a waste of time. Obviously, you can suggest whoever plays the product owner role to make that sort of change. But for now, this stays. Here is our test class:
package animals;
import animals.model.AnimalFactory;
import org.junit.Test;
import org.mockito.Mockito;
import java.io.PrintStream;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;
public class CodingAnimalPrinterTest {
@Test
public void print_printsTheSummaryWeExpect() {
PrintStream mockedInputStream = mock(PrintStream.class);
CodingAnimalPrinter printer =
CodingAnimalPrinter.instance(AnimalFactory.instance(), mockedInputStream);
printer.print();
verify(mockedInputStream, Mockito.times(1)).println(
"There are 0 coding animals. Their average position above sea is 0 meters.");
}
}The test makes sure println is called only once and with the value we want to see when there is no coding animal. Let’s run our new test which now should fail.
It fails due to the coding family qualifier not being defined. Let’s fix this now.
public class CodingAnimalPrinter extends AnimalPrinter {
/* collapsed code */
@Override
protected String getFamilyQualifier() {
return "coding";
}
}And now our test passes.
Paving the way for our favourite coding animal
Now we have the basic test confirming our summary does what it should and it passes, we know our coding animal printer is wired properly to our printer superclass.
We want to make sure that we will introduce a single new animal, the Coding Nagger to our bestiary. We know its average position above the sea is 69. It is now time to delete the previous coding animal summary test to leave room for new ones in line with our requirements.
From there we can write a test validating this information is passed properly to our printStream similarly to the sea and land printer tests.
package animals;
import animals.model.AnimalFactory;
import org.junit.Test;
import org.mockito.InOrder;
import java.io.PrintStream;
import static org.mockito.Mockito.*;
public class CodingAnimalPrinterTest {
@Test
public void print_printsWhatWeExpect() {
PrintStream mockedInputStream = mock(PrintStream.class);
InOrder orderChecker = inOrder(mockedInputStream);
CodingAnimalPrinter printer =
CodingAnimalPrinter.instance(AnimalFactory.instance(), mockedInputStream);
printer.print();
orderChecker.verify(mockedInputStream)
.println("There are 1 coding animals. Their average position above sea is 69 meters.");
orderChecker.verify(mockedInputStream)
.println("The Coding Nagger has an average position above sea is 69 meters.");
}
}Following a run, this new test will fail. And nope the faulty turn of phrase was not intended but I ain’t gonna fix it as we near the end of the post. If you didn’t notice it’s this thing:
The Coding Nagger has an average position above sea is 69 meters.When this would be more grammatically correct.
The Coding Nagger has an average position above sea of 69 meters.Enough with the grammar nazis umbrella. Let’s now create our CodingNagger class.
Our final beast
package animals.model;
public class CodingNagger implements Animal {
@Override
public String name() {
return null;
}
@Override
public int averagePositionAboveSea() {
return 0;
}
}Once more the tests will fail as they should since our printer does not have access to it. Giving the error below:
We need to introduce a coding animals getter to the coding animal printer provide our coding animal details. The first place to change is the animal factory to add our new beast and a method to access all the coding animals. Past, present and future. Here is the updated AnimalFactory:
package animals.model;
public class AnimalFactory {
private static AnimalFactory instance;
Dog dog;
Spider spider;
BlueFish blueFish;
Kraken kraken;
CodingNagger codingNagger;
public static AnimalFactory instance() {
if (instance == null) {
instance = new AnimalFactory();
}
return instance;
}
private AnimalFactory() {
dog = new Dog();
spider = new Spider();
blueFish = new BlueFish();
kraken = new Kraken();
codingNagger = new CodingNagger();
}
public Animal[] getSeaAnimals() {
return new Animal[] {
blueFish,
kraken
};
}
public Animal[] getLandAnimals() {
return new Animal[] {
dog,
spider
};
}
public Animal[] getCodingAnimals() {
return new Animal[] {
codingNagger
};
}
}And now we can use our collection getter in the coding animal printer.
package animals;
import animals.model.Animal;
import animals.model.AnimalFactory;
import java.io.PrintStream;
public class CodingAnimalPrinter extends AnimalPrinter {
protected CodingAnimalPrinter(AnimalFactory animalFactory, PrintStream printStream) {
super(animalFactory, printStream);
}
public static CodingAnimalPrinter instance(AnimalFactory animalFactory,
PrintStream printStream) {
return new CodingAnimalPrinter(animalFactory, printStream);
}
@Override
protected Animal[] getAnimals() {
return getAnimalFactory().getCodingAnimals();
}
@Override
protected String getFamilyQualifier() {
return "coding";
}
}We can run the tests once more but the will still fail since the CodingNagger class only contains default values.
Let fix this.
package animals.model;
public class CodingNagger implements Animal {
@Override
public String name() {
return "Coding Nagger";
}
@Override
public int averagePositionAboveSea() {
return 69;
}
}Our new test should now pass.
There is one more thing though. We have not connected our new printer to the rest of the app. We need a new test to make sure we do. The easiest thing to do would be to allow the app to be injected with our PrintStream of choice as well.
Propagate the PrintStream injection to the App
The very first thing we did in this post was changing our printers so we could test we send the right information through it. We could have extended it to App but it wasn’t needed then. We wrote the tests for what we knew we needed to change. But now that we need to change our App class to introduce our new printer, adding a test to maintain its integrity is the thing to do. Let’s add our minimal change to make it testable.
/*
* This Java source file was generated by the Gradle 'init' task.
*/
package animals;
import animals.model.AnimalFactory;
import java.io.PrintStream;
public class App {
private final LandAnimalPrinter landAnimalPrinter;
private final SeaAnimalPrinter seaAnimalPrinter;
public App(AnimalFactory animalFactory, PrintStream printStream) {
this.landAnimalPrinter = LandAnimalPrinter.instance(animalFactory, printStream);
this.seaAnimalPrinter = SeaAnimalPrinter.instance(animalFactory, printStream);
}
public void run() {
landAnimalPrinter.print();
seaAnimalPrinter.print();
}
public static void main(String[] args) {
new App(AnimalFactory.instance(), System.out).run();
}
}Cool, we can finally inject the PrintStream into the app and therefore test the app output. Say hello to AppTest!
package animals;
import animals.model.AnimalFactory;
import org.junit.Test;
import org.mockito.InOrder;
import java.io.PrintStream;
import static org.mockito.Mockito.inOrder;
import static org.mockito.Mockito.mock;
public class AppTest {
@Test
public void run_printsWhatsExpected() {
PrintStream mockedInputStream = mock(PrintStream.class);
InOrder orderChecker = inOrder(mockedInputStream);
App app = new App(AnimalFactory.instance(), mockedInputStream);
app.run();
orderChecker.verify(mockedInputStream)
.println("There are 2 land animals. Their average position above sea is 12 meters.");
orderChecker.verify(mockedInputStream)
.println("The Doggo has an average position above sea is 10 meters.");
orderChecker.verify(mockedInputStream)
.println("The Spider has an average position above sea is 2 meters.");
orderChecker.verify(mockedInputStream)
.println("There are 2 sea animals. Their average position above sea is -20100 meters.");
orderChecker.verify(mockedInputStream)
.println("The Blue Fish has an average position above sea is -100 meters.");
orderChecker.verify(mockedInputStream)
.println("The Kraken has an average position above sea is -20000 meters.");
}
}
That new test will be straightforward to write since it pretty much groups everything we did so far. Also, it passes, unsurprisingly.
Change our App test to cover coding animals
And we are all set to connect our coding animals’ printer to the rest of the application. All we need now is to add update our app test to check for the coding animals’ information printing. Let’s add these lines at the end of the app test.
orderChecker.verify(mockedInputStream)
.println("There are 1 coding animals. Their average position above sea is 69 meters.");
orderChecker.verify(mockedInputStream)
.println("The Coding Nagger has an average position above sea is 69 meters.");That test will fail as it should.
As you can see the message refers to the coding animal summary we expect to see printed after the Kraken info. Let’s correct this. After a few nanoseconds, the App looks like this:
package animals;
import animals.model.AnimalFactory;
import java.io.PrintStream;
public class App {
private final LandAnimalPrinter landAnimalPrinter;
private final SeaAnimalPrinter seaAnimalPrinter;
private final CodingAnimalPrinter codingAnimalPrinter;
public App(AnimalFactory animalFactory, PrintStream printStream) {
this.landAnimalPrinter = LandAnimalPrinter.instance(animalFactory, printStream);
this.seaAnimalPrinter = SeaAnimalPrinter.instance(animalFactory, printStream);
this.codingAnimalPrinter = CodingAnimalPrinter.instance(animalFactory, printStream);
}
public void run() {
landAnimalPrinter.print();
seaAnimalPrinter.print();
codingAnimalPrinter.print();
}
public static void main(String[] args) {
new App(AnimalFactory.instance(), System.out).run();
}
}Let’s run our test again, actually, let’s run all of them!
And we’re done with this code refactoring thing. Let’s have one last look at our model for good measure.
If you compare with what we had before starting this code refactoring you will notice the model is now more focused. All the responsibility to handle the printing moved towards the AnimalPrinter superclass. It also handles access to the animal factory for the subclasses that now only need to care about providing a family qualifier and list of animals to print.
Alright, let’s run our app for the second and last time of this post.
Whoever read this post to the end shall possess the power of code refactoring…
…or at least a better idea of it.
Remember, the most importing thing before you start refactoring any code is to write tests to make sure you don’t break it. And if the test is not testable, make the minimum amount of change to make it testable and preserve its logic. Then balance between refactoring the smallest possible chunks that will make the biggest impact. Finally, refactor only if you need to change the code while fixing a bug or adding a feature.
Thanks for reading, and here is the repository with the source code.
