Temporal Coupling

Situation

This is what I’m looking at.

Board Class
public boolean hasWinner() {
    return new Lines(cells).hasAWinner();
}

public Mark getWinningMark() {
    return new Lines(cells).getWinningMark(); // Temporal coupling.
}
Lines Class
private Mark winningMark;

public Mark getWinningMark() {
    return winningMark; // Temporal coupling.
}

public boolean hasAWinner() {
    return checkLinesForAllMatchingElements();
}

private boolean checkLinesForAllMatchingElements() {
    for (List<Mark> line : possibleCombinations()) {
        if (hasAllMatchingElements(line)) {
            winningMark = line.get(0); // Assignment occurs here.
            return true;
        }
    }
    return false;
}

Complication

As noted by the inline comment, this code suffers from temporal coupling. During gameplay this issue does not occur, since the gameplay loop prevents getWinningMark() from being called before the game has a winner. However, in testing this method, the test fails unless hasWinner() is called in the test first.

Question

How can the code be altered here in order to remove the temporal coupling and make improve the flexibility overall?

Answer One

Modify the Lines class to call checkLinesForAllMatchingElements(). This keeps the temporal coupling, but means the tests pass and the functionality is there. For the sake of running one loop again, it is not particularly problematic, but it feels wrong.

Lines Class
private Mark winningMark;

public Mark getWinningMark() {
    checkLinesForAllMatchingElements();
    return winningMark; // Temporal coupling.
}

public boolean hasAWinner() {
    return checkLinesForAllMatchingElements();
}

private boolean checkLinesForAllMatchingElements() {
    for (List<Mark> line : possibleCombinations()) {
        if (hasAllMatchingElements(line)) {
            winningMark = line.get(0);
            return true;
        }
    }
    return false;
}

Answer Two

By initialising winningMark as null, we can do a check for null here and avoid a null pointer exception. In fact winningMark will be null if it is not initialised, but at least in specifying it that is no longer a surprise. This is sort of OK, but null is pretty grimey, so I’d like to avoid it. This solution doesn’t give much information to anyone using the class why the null check exists.

Lines Class
private Mark winningMark = null;

public Mark getWinningMark() {
    if (winningMark == null) {
        return EMPTY;
    }
    return winningMark;
}

public boolean hasAWinner() {
    return checkLinesForAllMatchingElements();
}

private boolean checkLinesForAllMatchingElements() {
    for (List<Mark> line : possibleCombinations()) {
        if (hasAllMatchingElements(line)) {
            winningMark = line.get(0);
            return true;
        }
    }
    return false;
}

Answer Three

By throwing a unchecked exception, the public API for the Board class is descriptive. However, it is on a par with Answer Two for laziness. It would probably better to avoid the exception in the first place.

PrematureException Class
public class PrematureException extends RuntimeException {

    public PrematureException() {
        super("You have called this method before there is a winner.");
    }

}
Lines Class
private Mark winningMark;

public Mark getWinningMark() {
    if (winningMark == null) {
        throw new PrematureException();
    }
    return winningMark;
}

public boolean hasAWinner() {
    return checkLinesForAllMatchingElements();
}

private boolean checkLinesForAllMatchingElements() {
    for (List<Mark> line : possibleCombinations()) {
        if (hasAllMatchingElements(line)) {
            winningMark = line.get(0);
            return true;
        }
    }
    return false;
}

Answer Four

Java Optionals allow a value to be unspecified without having to deal with checks for null. From my point of view this only marginally better than a null check, the logic is the same but the syntax is improved.

N.B. Following a conversation with some craftspeople this implementation has been revised in Answer Seven.

Lines Class
private Optional<Mark> winningMark = Optional.empty();


public Mark getWinningMark() {
    return winningMark.orElse(EMPTY);
}

public boolean hasAWinner() {
    return checkLinesForAllMatchingElements();
}

private boolean checkLinesForAllMatchingElements() {
    for (List<Mark> line : possibleCombinations()) {
        if (hasAllMatchingElements(line)) {
            winningMark = Optional.of(line.get(0));
                return true;
            }
    }
    return false;
}

Answer Five

In order to avoid the situation entirely, a better solution might be for the game to keep track of each mark that is played. So that if at any point the game ends in a “Win” condition, then it can query for the last mark used since that will always be the winner.

Answer Six

This is much better solution since it maintains the code as it is, no breaking changes to the implementation like Answer Five. It also removes the temporal coupling. It does leave a null check to propogate to other classes that implement it, but it is on the whole a much more elegant solution.

Lines Class
public Mark getWinningMark() {
    for (List<Mark> line : possibleCombinations()) {
        if (hasAllMatchingElements(line)) {
            return line.get(0);
        }
    }
    return null;
}

public boolean hasAWinner() {
    return getWinningMark() != null;
}

Answer Seven

This new version of the optionals implementation is the one I will keep. It is functionally very similar to Answer Six, equally as elegant, but also provides the option to move the decision logic up to another class. So the Game class that actually acts on board.hasAWinner and board.getWinningMark could be passed the optionals instead, and then use the pre-built methods to make decisions from there.

Lines Class
public Optional<Mark> winner() {
    for (List<Mark> line : possibleCombinations()) {
        if (hasAllMatchingElements(line)) {
            return Optional.of(line.get(0));
        }
    }
    return Optional.empty();
}

// Or streaming method!!

public Optional<Mark> winner() {
    return possibleCombinations().stream()
            .filter(line -> hasAllMatchingElements(line))
            .findFirst()
            .map(line -> line.get(0));
}
Board Class
public boolean hasWinner() {
    return new Lines(cells).winner().isPresent();
}

public Mark getWinningMark() {
    return new Lines(cells).winner().get();
}

I have no idea what tomorrow’s blog post will be about because I’ve spent most of it writing this… I think it was worth it though.