Safe Intersections

Code Review's April 2017 Community Challenge has us simulating a multi-way intersection. My emphasis was around safety (don't want cars crashing) and ease of use. I plan on adding "smart intersections" (can detect when a car is/isn't there and change the signals accordingly) and a GUI later on.

The sample intersection:



Time | EastWest | NorthSouth
-----+----------+-------------
0s | Red | Green
1s | Red | Green
2s | Red | Green
3s | Red | Green
4s | Red | Yellow
5s | Green | Red
6s | Green | Red
7s | Green | Red
8s | Green | Red
9s | Yellow | Red


Can be created via:

Intersection intersection = Intersection.builder()
.addLights(2, () -> new Light(4, 1, 0)) // greenDuration, yellowDuration, redPause
.setActivationRules((rules, lights) -> {
// These rules specify when to activate a light. This allows for
// more complex intersections.
rules.after(lights.get(0)).thenActivate(lights.get(1));
rules.after(lights.get(1)).thenActivate(lights.get(0));
// One more type of rule can be defined:
// rules.afterAll(lights.get(0), lights.get(1)).thenActivate(lights.get(2));
}).activate(0)
.build();


There are quite a few files, so the code is available on github. I use Google's Guava library.

Light.java

`package trafficlights.intersection;

import com.google.common.collect.ImmutableMap;

import java.util.EnumMap;
import java.util.OptionalDouble;

import static com.google.common.base.Preconditions.checkState;

public class Light {
public enum Color {
RED, GREEN, YELLOW
}

private enum State {
GREEN, YELLOW, WAITING, OFF;

Color getColor() {
switch (this) {
case GREEN:
return Color.GREEN;
case YELLOW:
return Color.YELLOW;
case WAITING:
case OFF:
return Color.RED;

First pass reading:

if (durations.containsKey(state)) return OptionalDouble.of(durations.get(state));
return OptionalDouble.empty();

• The indentation is unconventional. Additionally I strongly recommend always putting curly braces where possible. This helps avoid issues when editing the code.

• What you have here is a Double with null being signified as OptionalDouble.empty().

• You could use durations.getOrDefault(state, OptionalDouble.empty()) to do all the messy work for you :)

public ActivationRules(IdentityHashMap> simpleRules,
                       IdentityHashMap> complexRules) {

I understand why you're forcing IdentityHashMap here, but I don't like seeing it here. The problem is that you're not uniquely identifying Lights. The solution is to use a unique identifier for Lights. Consider keeping a String as identifier.

simpleRules.merge(triggers.iterator().next(), lights, (existing, toAdd) -> {
                existing.addAll(toAdd);
                return existing;
            });

I found this hard to understand, mostly because I haven't encountered merge all that much in live code. I think I'd understand better if this was written as:

simpleRules.computeIfAbsent(triggers.get(0), k -> new ArrayList<>()).addAll(lights);

Note that I replaced .iterator().next() with get(0), which should be semantically equivalent. Also note that this separates the default value from the merging process, which merge doesn't do that explicitly.

Design review

From a design view it's a bit annoying that ComplexActivationRule and SimpleActivationRule do not follow a unified interface. Given these names, I'd have expected these classes to inherit from a shared interface ActivationRule.

public interface ActivationRule {
    boolean matchesTrigger(List freshDisabledLights);
    void performActivation();
}

This would simplify activateTriggered to:

private void activateTriggered(List deactivated) {
    activationRules.stream()
        .filter(rule -> rule.matchesTrigger(deactivated))
        .forEach(ActivationRule::performActivation);
}

It moves the knowledge of how triggers are matched from Intersection into the different rules. The problem here of course is that activating lights is maybe not any of ActivationRule's business. If you want to enforce that, what do you think of the following:

activationRules.stream()
    .filter(rule -> rule.matchesTrigger(deactivated))
    .flatMap(ActivationRule::triggeredLights)
    .distinct() // maybe unnecessary?
    .forEach(Light::activate);

The interface for ActivationRule directly follows :)

I like very much how you enforce a certain order when building an Intersection. I don't like quite as much that your RuleBuilder requires Light instances to work. It might be cleaner to wrap RuleBuilding as an implementation detail of the intersection. Instead of accessing the currently building intersections's lights you can then instead just pass integers to the IntersectionBuilder. Building an intersection could look as follows:

Intersection intersection = Intersection.builder()
    .addLights(2, () -> new Light(4, 1, 0))
    .addActivationRule(1, 0)
    .addActivationRule(0, 1)
    //.addActivationRule(2, 0, 1)
    .activate(0)
    .build();

^^ this would build the same intersection as your usage example. At the end of the day it's convention that you specify triggers before target, but it's not strictly necessary.

This design avoids a possibly complex lambda, as well as the necessity to access Builder internals during the building process.

Summarizing

This code looks pretty clean, but I think the design can be improved a bit. As soon as that happens, the high abstractions will also read more fluently. Good Job :)

StackExchange Code Review Q#159690, answer score: 2