An example of introducing symmetry to enable duplication removal

Symmetry is a subtle concept that may seem only related to code aesthetics. However, as Kent Beck states in Implementation Patterns,

"...finding and expressing symmetry is a preliminary step to removing duplication. If a similar thought exists in several places in the code, making them symmetrical to each other is a first good step towards unifying them"

In this post we'll look at an example of expressing symmetry as a way to make duplication more visible. This is the initial code of a version of a subset of the Mars Rover kata that Álvaro García and I used in a refactoring workshop some time ago:

	public class Rover {
	private String direction;
	private int y;
	private int x;
	public Rover(int x, int y, String direction) {
	this.direction = direction;
	this.y = y;
	this.x = x;
	}
	public void receive(String commandsSequence) {
	for (int i = 0; i < commandsSequence.length(); ++i) {
	String command = commandsSequence.substring(i, i + 1);
	if (command.equals("l") || command.equals("r")) {
	// Rotate Rover
	if (direction.equals("N")) {
	if (command.equals("r")) {
	direction = "E";
	} else {
	direction = "W";
	}
	} else if (direction.equals("S")) {
	if (command.equals("r")) {
	direction = "W";
	} else {
	direction = "E";
	}
	} else if (direction.equals("W")) {
	if (command.equals("r")) {
	direction = "N";
	} else {
	direction = "S";
	}
	} else {
	if (command.equals("r")) {
	direction = "S";
	} else {
	direction = "N";
	}
	}
	} else {
	// Displace Rover
	int displacement1 = -1;
	if (command.equals("f")) {
	displacement1 = 1;
	}
	int displacement = displacement1;
	if (direction.equals("N")) {
	y += displacement;
	} else if (direction.equals("S")) {
	y -= displacement;
	} else if (direction.equals("W")) {
	x -= displacement;
	} else {
	x += displacement;
	}
	}
	}
	}
	}

view raw initial_mars_rover_code.java hosted with ❤ by GitHub

This code is using conditionals to express two consecutive decisions:

1. Which command to execute depending on a command code.
2. How to execute the command depending on the direction the rover faces.

These decisions are repeated for different responsibilities, displacing the rover and rotating the rover, so the code presents a Case Statements smell. Since those decisions are completely independent, we could mechanically refactor the code to start using polymorphism instead of conditionals. This way we'll end having two consecutive single dispatches, one for each decision.

If we start applying Replace Type Code with State/Strategy refactoring to the code as it is now, there is a subtle problem, though.

Looking carefully more carefully, we can observe that in the case of the rover's displacement, the sequence of two decisions is clearly there:

However, that sequence is not there in the case of the rover's rotations. In this case there's a third decision (conditional) based on the command type which as, we'll see, is not necessary:

This difference between the two sequences of decisions reveals a lack of symmetry in the code. It's important to remove it before starting to refactor the code to substitute the conditionals with polymorphism.

I've seen many cases in which developers naively start extracting methods and substituting the conditionals with polymorphism starting from asymmetrical code like this one. This often leads them to create entangled and leaking abstractions.

Particularly, in this case, blindly applying Replace Type Code with State/Strategy refactoring to the left and right rotations can very likely lead to a solution containing code like the following:

	public enum Direction {
	private final String value;
	Direction(String value) {
	this.value = value;
	}
	public static Direction from(String value) {
	for (Direction current : values()) {
	if (current.value.equals(value)) {
	return current;
	}
	}
	throw new RuntimeException("Passed value is not correct: " + value);
	}
	public Direction rotate(Command command) {
	if (Command.RIGHT == command) {
	return rotateRight();
	} else {
	return rotateLeft();
	}
	}
	protected abstract Direction rotateRight();
	protected abstract Direction rotateLeft();
	NORTH("N") {
	@Override
	protected Direction rotateRight() {
	return EAST;
	}
	@Override
	protected Direction rotateLeft() {
	return WEST;
	}
	},
	SOUTH("S") {
	@Override
	protected Direction rotateRight() {
	return (WEST);
	}
	@Override
	protected Direction rotateLeft() {
	return (EAST);
	}
	},
	WEST("W") {
	@Override
	protected Direction rotateRight() {
	return (NORTH);
	}
	@Override
	protected Direction rotateLeft() {
	return (SOUTH);
	}
	},
	EAST("E") {
	@Override
	protected Direction rotateRight() {
	return (SOUTH);
	}
	@Override
	protected Direction rotateLeft() {
	return (NORTH);
	}
	};
	}

view raw direction_knowing_about_commands.java hosted with ❤ by GitHub

	public enum Command {
	RIGHT("r"),
	LEFT("l");
	private String value;
	Command(String value) {
	this.value = value;
	}
	public static Command from(String value) {
	for (Command current : values()) {
	if (current.value.equals(value)) {
	return current;
	}
	}
	throw new RuntimeException("Passed value is not correct: " + value);
	}
	}

view raw command_leaking_encoding_knowledge.java hosted with ❤ by GitHub

Notice how the Direction class contains a decision based on the encoding of the command. To be able to take that decision, Direction needs to know about the encoding, which is why it's been made public in the Command class.

This is bad... Some knowledge about the commands encoding has leaked from the Command class into the Direction class. Direction shouldn't be taking that decision in the first place. Neither should it know how commands are encoded. Moreover, this is a decision that was already taken and doesn't need to be taken again.

How can we avoid this trap?

We should go back to the initial code and instead of mechanically applying refactoring, we should start by removing the asymmetry from the initial code.

You can see one way of doing it in this video:



After this refactoring all cases are symmetrical (they present the same sequence of decisions)

We've not only removed the asymmetry but we've also made more explicit the case statements (Case Statements smell) on the command encoding and the duplicated switches on the direction the rover is facing:

	public void receive(String commandsSequence) {
	for (int i = 0; i < commandsSequence.length(); ++i) {
	String command = commandsSequence.substring(i, i + 1);
	if (command.equals("l")) {
	// Rotate Rover to the left
	if (direction.equals("N")) {
	direction = "W";
	} else if (direction.equals("S")) {
	direction = "E";
	} else if (direction.equals("W")) {
	direction = "S";
	} else {
	direction = "N";
	}
	} else if (command.equals("r")) {
	// Rotate Rover to the right
	if (direction.equals("N")) {
	direction = "E";
	} else if (direction.equals("S")) {
	direction = "W";
	} else if (direction.equals("W")) {
	direction = "N";
	} else {
	direction = "S";
	}
	} else {
	// Displace Rover
	int displacement1 = -1;
	if (command.equals("f")) {
	displacement1 = 1;
	}
	int displacement = displacement1;
	if (direction.equals("N")) {
	y += displacement;
	} else if (direction.equals("S")) {
	y -= displacement;
	} else if (direction.equals("W")) {
	x -= displacement;
	} else {
	x += displacement;
	}
	}
	}
	}

view raw mars_rover_symmetric.java hosted with ❤ by GitHub

Now it's clear that the third decision (third nested conditional) in the original rover's rotations code was unnecessary.

If we start the Replace Type Code with State/Strategy refactoring now, it's more likely that we'll end with a code in which Direction knows nothing about how the commands are encoded:

	public enum Direction {
	NORTH {
	@Override
	public Coordinates move(Coordinates coordinates, int displacement) {
	return coordinates.incrementY(displacement);
	}
	@Override
	public Direction rotateRight() {
	return EAST;
	}
	@Override
	public Direction rotateLeft() {
	return WEST;
	}
	}, SOUTH {
	@Override
	public Coordinates move(Coordinates coordinates, int displacement) {
	return coordinates.incrementY(-displacement);
	}
	@Override
	public Direction rotateRight() {
	return WEST;
	}
	@Override
	public Direction rotateLeft() {
	return EAST;
	}
	}, EAST {
	@Override
	public Coordinates move(Coordinates coordinates, int displacement) {
	return coordinates.incrementX(displacement);
	}
	@Override
	public Direction rotateRight() {
	return SOUTH;
	}
	@Override
	public Direction rotateLeft() {
	return NORTH;
	}
	}, WEST {
	@Override
	public Coordinates move(Coordinates coordinates, int displacement) {
	return coordinates.incrementX(-displacement);
	}
	@Override
	public Direction rotateRight() {
	return NORTH;
	}
	public Direction rotateLeft() {
	return SOUTH;
	}
	};
	public static Direction pointingTo(String directionCode) {
	if (directionCode.equals("N")) {
	return NORTH;
	} else if (directionCode.equals("S")) {
	return SOUTH;
	} else if (directionCode.equals("E")) {
	return EAST;
	} else {
	return WEST;
	}
	}
	abstract public Coordinates move(Coordinates coordinates, int displacement);
	abstract public Direction rotateRight();
	abstract public Direction rotateLeft();
	}

view raw direction.java hosted with ❤ by GitHub

and the encoding of each command is known in only one place:

	public class CommandCodeInterpreter {
	private static final int DISPLACEMENT_LENGTH = 1;
	private static final String FORWARD_MOVEMENT = "f";
	private static final String BACKWARD_MOVEMENT = "b";
	private static final String RIGHT_ROTATION = "r";
	private static final String LEFT_ROTATION = "l";
	private static Map<String, Command> knownCommands = knownCommands();
	public static Command interpret(String commandCode) {
	return getCommandFor(commandCode);
	}
	private static Command getCommandFor(String commandCode) {
	return knownCommands.get(commandCode);
	}
	private static Map<String, Command> knownCommands() {
	Map<String, Command> knownCommands = new HashMap<>();
	knownCommands.put(FORWARD_MOVEMENT, new Movement(DISPLACEMENT_LENGTH));
	knownCommands.put(BACKWARD_MOVEMENT, new Movement(-DISPLACEMENT_LENGTH));
	knownCommands.put(RIGHT_ROTATION, new RightRotation());
	knownCommands.put(LEFT_ROTATION, new LeftRotation());
	return Collections.unmodifiableMap(knownCommands);
	}
	}

view raw command_factory.java hosted with ❤ by GitHub

As we said at the beginning, symmetry is a very useful concept that can help you guide refactoring. Detecting asymmetries and thinking why they happen, can help you to detect hidden duplication and, as in this case, sometimes entangled dimensions of complexity ^[1].

Then, by removing those asymmetries, you can make the duplication more visible and disentangle the entangled dimensions of complexity. The only thing you need is to be patient and don't start "obvious" refactorings before thinking a bit about symmetry.

^[1] Dimension of Complexity is a term used by Mateu Adsuara in a talk at SocraCan16 to name an orthogonal functionality. In that talk he used dimensions of complexity to group the examples in his test list and help him choose the next test when doing TDD. He talked about it in this three posts: Complexity dimensions - FizzBuzz part I, Complexity dimensions - FizzBuzz part II and Complexity dimensions - FizzBuzz part III. Other names for the same concept that I've heard are axes of change, directions of change or vectors of change.