I was working with a team on a relatively new code base. They were new to Test Driven Development, Behavior Driven Development, and pairing/mobbing. They were casually familiar with the SOLID principles. A good group of folks, mostly willing to learn some new ideas, and all eager to deliver value for the company in the form of well written software that solved business problems.
Along with our test suite, Jenkins was configured to run some static analysis tools. We reviewed the static analysis trends in our weekly planning meeting where we also looked at throughput, WIP, the story map, and the backlog. Adding quality metrics into the discussion added a new dimension and enriched the conversations. Soon enough, the team was looking at quality metrics with each code push.
Things like extracting methods, eliminating duplication, improving test coverage, and eliminating magic strings and numbers were all easy enough. But reducing Cyclomatic Complexity proved more challenging.
Cyclomatic Complexity
For those unfamiliar with it, Cyclomatic Complexity, also known as McCabe’s Number, is a measure of the number of linear paths through a code base. It effectively tells us how many conditionals and loops we have in the code. As it turns out, this number has a positive correlation to defects. The more conditionals and loops, the higher the number of defects.
Basic InherItance
One common technique for collapsing conditionals, and thereby reducing cyclomatic complexity, is to restructure the code using basic inheritance. This can also be done with compositional techniques, but for this blog post, we’ll briefly cover polymorphism.
Often, when we have a method with a series of if statements, there is a missing object or two hiding in plain sight.
Look at the following pseudo-code:
Double getSpeed(String type) {
if (type.equals(EUROPEAN)) {
return getBaseSpeed();
}
if (type.equals(AFRICAN)) {
return getBaseSpeed() - getLoadFactor() * CoconutCount;
}
if (type.equals(NORWEGIAN_BLUE)) {
return (isNailed()) ? 0 : getBaseSpeed();
}
throw new RuntimeException ("Invalid Swallow Type");
}
It’s pretty obvious that we are trying to determine the air speed of a swallow, laden or unladen, depending on the type of swallow.
Not only does this method have high cyclomatic complexity, but our current solution violates the Open/Closed Principal. If we want to add a new type of swallow, we have to not only create the new swallow type somewhere else, but we have to modify this method in order to make it aware of the new swallow type. This method is not open for extension nor is it closed for modification.
If these are all types of swallows, perhaps we can introduce a Swallow Base Class and then extend it for the various types of Swallows.
class Swallow {
...
Double getSpeed() {
return getBaseSpeed();
}
}
class EuropeanSwallow extends Swallow {
...
}
class AfricanSwallow extends Swallow {
Double getSpeed() {
return super.getSpeed - coconutLoad();
}
Double coconutLoad() {
return getLoadFactor() * _numberOfCoconuts;
}
}
class NorwegianSwallow extends Swallow {
Double getSpeed() {
return (isNailed()) ? 0 : super.getSpeed();
}
}
Hopefully, from the psuedo-code, you get the general idea.
What I really want to focus on here is a second technique that came up when one of the team members brought me a piece of code and asked, “How can I use basic inheritance to reduce this?”
package com.docondev.chain.suspect;
class Detective {
static final boolean SUSPICIOUS = true;
static final boolean TRUSTWORTHY = false;
private Detective() {
throw new IllegalStateException("Static class");
}
static boolean shouldDetainSuspect(Suspect suspect) {
if (isFloatable(suspect) &&
isMadeOfWood(suspect) &&
isDuckWeight(suspect)) {
return SUSPICIOUS;
}
if (isLycanthrope(suspect) && isCanineLike(suspect)) {
return SUSPICIOUS;
}
if (suspect.getDoctor().getLastName()
.equalsIgnoreCase("VanHelsing") &&
suspect.getHomeTown().equals("Wallachian") &&
suspect.getOccupation().equalsIgnoreCase("Prince")) {
return SUSPICIOUS;
}
return TRUSTWORTHY;
}
static boolean isCanineLike(Suspect suspect) {
return suspect.getProperties().stream()
.anyMatch(InterestingFacts.canineAttributes::contains);
}
private static boolean isLycanthrope(Suspect suspect) {
return suspect.getHobbies().stream()
.anyMatch(InterestingFacts.warewolfHobbies::contains);
}
static boolean isFloatable(Suspect suspect) {
Double weight = suspect.getWeight();
Double volume = suspect.getVolume();
return (weight / volume) < InterestingFacts.WATER_DENSITY;
}
static boolean isDuckWeight(Suspect suspect) {
return suspect.getWeight()
.equals(InterestingFacts.DUCK_WEIGHT);
}
static boolean isMadeOfWood(Suspect suspect) {
return suspect.getProperties().stream()
.anyMatch(p -> p.contains("wood"));
}
}
Chain of Command
Please note - This is intended as a means to show the design pattern and to have a little fun with Monty Python jokes. I am quite confident this is insufficient for legal purposes.
The primary issue here is the complexity of the shouldDetainSuspect method. There are slightly different ways to measure cyclomatic complexity, but for the sake of this article, let’s say the cyclomatic complexity of the shouldDetainSuspect method is 9; 1 for the method and one each for the conditionals (if, ||, and &&). Not accounting for the whitespace and returns, there are 7 lines of code in this method. So the ratio is approximately 1.3 logic branches per line of code. That’s astronomical.
In this case, we are going to use a command pattern. Specifically, we are going to chain a series of commands together. We are going to build a series of evaluator command objects. The job of an evaluator command is to evaluate the individual based on specific criteria and determine if they are suspicious or trustworthy. In this implementation, each evaluator will either report the individual is suspicious or having concluded they are not suspicious, call the next evaluator. If there is no next evaluator, then the individual is reported trustworthy.
Let’s start with our base command class. This is the abstraction from which the rest of the command classes will be built.
package com.docondev.chain.suspect;
import org.apache.commons.lang3.StringUtils;
import java.util.logging.Logger;
public abstract class AbstractEvaluatorCommand {
enum Evaluations {
SUSPICIOUS(true),
TRUSTWORTHY(false);
private final boolean decision;
Evaluations(boolean decision) {
this.decision = decision;
}
}
private AbstractEvaluatorCommand nextCommand;
static final Logger logger =
Logger.getLogger(AbstractEvaluatorCommand.class.getName());
public EvaluatorResponse execute(Suspect suspect) {
if (shouldDetain(suspect)) {
return generatedResponse(Evaluations.SUSPICIOUS);
}
if (nextCommand != null) {
return nextCommand.execute(suspect);
}
return generatedResponse(Evaluations.TRUSTWORTHY);
}
private EvaluatorResponse generatedResponse(Evaluations evaluation) {
return new EvaluatorResponse(getCommandName(), evaluation);
}
public void setNextCommand(AbstractEvaluatorCommand command) {
this.nextCommand = command;
}
protected abstract boolean shouldDetain(Suspect suspect);
private String getCommandName() {
return parseToHumanReadableCommandName();
}
private String parseToHumanReadableCommandName() {
return StringUtils.join(
StringUtils.splitByCharacterTypeCamelCase(
this.getClass().getSimpleName()),
' '
).replace("Command", "").trim();
}
}
The execute method is the heart of the command object. It can, of course, be called anything, but I try to name objects and methods consistent with the patterns they implement for the sake of clarity. I think it is better to leave clues for future developers. By using Command in the class name and calling the primary method, execute, anyone familiar with the command pattern will recognize it right away. Anyone not familiar might have a better chance of discovering that it is a pattern implementation if we name it in accord.
Our execute method is pretty simple - it checks to see if we think the suspect should be detained and indicates suspicion. If it determines that the suspect should not be detained, it either calls the next command in the chain or it indicates trustworthiness.
The EvaluatorResponse is a Plain Old Java Object (POJO). It contains the command that responded and the actual response. We have some trickery in here for determining the command that responded - we parse the class name on capital letters, strip off the “Command”, and join the results back together with a space between them. So a class named SillyExampleCommand will report “Silly Example” as the command name.
All the implementation classes need to do here is implement their own version of shouldDetain().
Let’s take a look at our Vampire Evaluator Command. As you might guess, this evaluates whether or not an individual (a suspect) might be a vampire.
package com.docondev.chain.suspect;
public class VampireEvaluatorCommand extends AbstractEvaluatorCommand {
@Override
protected boolean shouldDetain(Suspect suspect) {
return suspect.getDoctor()
.getLastName().equalsIgnoreCase("VanHelsing") &&
suspect.getHomeTown().equals("Wallachian") &&
suspect.getOccupation().equalsIgnoreCase("Prince");
}
}
This is pretty straightforward. If the suspect has a Doctor with the last name of VanHelsing, and the suspect is a prince from Wallachian, then we have reason to think they might be Dracula and should be retained.
Our Werewolf Evaluator Command is similar, except that we look to see if the individual has properties similar to a werewolf, such as fangs or claws, and we check to see if they have hobbies similar to those of a werewolf, such as lycanthropy or howling at the moon.
package com.docondev.chain.suspect;
public class WerewolfEvaluatorCommand extends AbstractEvaluatorCommand {
private static final List canineAttributes =
Arrays.asList("claws", "fur", "fangs");
private static final List werewolfHobbies =
Arrays.asList("lycanthropy", "howling at the moon");
@Override
protected boolean shouldDetain(Suspect suspect) {
return isCanineLike(suspect) && isLycanthrope(suspect);
}
private static boolean isCanineLike(Suspect suspect) {
return suspect.getProperties().stream()
.anyMatch(InterestingFacts.canineAttributes::contains);
}
private static boolean isLycanthrope(Suspect suspect) {
return suspect.getHobbies().stream()
.anyMatch(InterestingFacts.warewolfHobbies::contains);
}
}
And finally, the Witch Evaluator Command is similar again, except that it checks to see if the individual can float, weighs the same as a duck, and is made of wood (of course).
package com.docondev.chain.suspect;
public class WitchEvaluatorCommand extends AbstractEvaluatorCommand {
static final Double DUCK_WEIGHT = 4.2;
private static final double WATER_DENSITY = 8.333;
@Override
protected boolean shouldDetain(Suspect suspect) {
return isFloatable(suspect)
&& isWooden(suspect)
&& isDuckWeight(suspect);
}
private Boolean isWooden(Suspect suspect) {
return suspect.hasProperty("wood");
}
private static Boolean isDuckWeight(Suspect suspect) {
return suspect.getWeight()
.equals(DUCK_WEIGHT);
}
private static Boolean isFloatable(Suspect suspect) {
double suspectDensity = suspect.getWeight() / suspect.getVolume();
return suspectDensity < WATER_DENSITY;
}
}
We can call any individual evaluator command and it will work just fine, as we show in this subset of tests.
@Before
public void setUp() {
suspect.setWeight(WitchEvaluatorCommand.DUCK_WEIGHT);
suspect.setVolume(4.0);
}
@Test
public void confirmWitchEvaluator_ReportsWitchEvaluator() {
assertThat(
witchEvaluator.execute(suspect)
.getCommandName(), is(equalTo("Witch Evaluator")));
}
@Test
public void givenSuspectIsFloatableAndDuckWeightAndNotWooden_Trustworthy() {
assertThat(
witchEvaluator.execute(suspect)
.getEvaluation(), is(equalTo(Evaluations.TRUSTWORTHY)));
}
But the real power comes into play when we chain the individual command objects.
Let’s take a look at our Detective object now. There is essentially one method left in the class; shouldDetainSuspect. This class receives a suspect, wires up a chain of command objects to evaluate the suspect based on significantly different criteria, and returns a response indicating not only if the suspect is trustworthy or suspicious, but also which command last executed. From this, we can determine why the suspect needs to be detained - do we believe they are a witch, a werewolf, or a vampire?
package com.docondev.chain.suspect;
class Detective {
private Detective() {
throw new IllegalStateException("Static class");
}
static EvaluatorResponse shouldDetainSuspect(Suspect suspect) {
WitchEvaluatorCommand witchEvaluator =
new WitchEvaluatorCommand();
VampireEvaluatorCommand vampireEvaluator =
new VampireEvaluatorCommand();
vampireEvaluator.setNextCommand(new WerewolfEvaluatorCommand());
witchEvaluator.setNextCommand(vampireEvaluator);
return witchEvaluator.execute(suspect);
}
}
The cyclomatic complexity of shouldDetainSuspect is now 1 and there are only 5 lines of code. That is .2 logic branches per line of code. That is perfectly reasonable.
Conclusion
There is more that could be done with this code. For example, right now the Detective needs to know about all the possible ways a suspect could be evaluated, so there is a coupling between the Detective and the Evaluators. I’m not sure I like that. The Detective is not as open for extension as I’d like it to be. And the Detective also knows how to chain evaluators. I don’t know if that makes sense either.
But we set out to reduce the cyclomatic complexity of the Detective.shouldDetainSuspect method. And we did just that while actually improving the quality of the data the Detective returns and creating a much simpler way of defining new attribute combinations that may indicate a suspect should be detained.
You can find all of the code referenced in this article on my GitHub page along with some other examples of this same pattern. I hope you found this useful.
Feel free to provide feedback.