What is the explanation of null values in a cyclic dependency in final private enum fields?

Question

Consider these enum declarations:

enum Color {
    RED(Shape.CIRCLE),
    GREEN(Shape.TRIANGLE),
    BLUE(Shape.SQUARE);

    private final Shape shape;

    Color(Shape shape) {
        this.shape = shape;
    }

    Shape getShape() {
        return shape;
    }
}

enum Shape {
    CIRCLE(Color.RED),
    TRIANGLE(Color.GREEN),
    SQUARE(Color.BLUE);

    private final Color color;

    Shape(Color color) {
        this.color = color;
    }

    Color getColor() {
        return color;
    }
}

There are cyclic dependencies between the enum fields. There are no compiler warnings (using Java 8).

However, all these tests will fail in the second line:

@Test
public void testRedAndCircle() {
    assertThat(Color.RED.getShape()).isNotNull();
    assertThat(Shape.CIRCLE.getColor()).isNotNull(); // fails
}

@Test
public void testCircleAndRed() {
    assertThat(Shape.CIRCLE.getColor()).isNotNull();
    assertThat(Color.RED.getShape()).isNotNull(); // fails
}

@Test
public void testGreenAndTriangle() {
    assertThat(Color.GREEN.getShape()).isNotNull();
    assertThat(Shape.TRIANGLE.getColor()).isNotNull(); // fails
}

@Test
public void testBlueAndSquare() {
    assertThat(Color.BLUE.getShape()).isNotNull();
    assertThat(Shape.SQUARE.getColor()).isNotNull(); // fails
}

How can the null value in the enum field be explained?

It seems that the enum object in the private final fields is not yet completely instantiated.

Answer 1

One way to break the cycle is to make the evaluation lazy in at least one of the enums, thus allowing the other to get initialised before being used. E.g.:

enum Color {
    RED(() -> Shape.CIRCLE),
    GREEN(() -> Shape.TRIANGLE),
    BLUE(() -> Shape.SQUARE);

    private final Supplier<Shape> shape;

    Color(Supplier<Shape> shape) {
        this.shape = shape;
    }

    Shape getShape() {
        return shape.get();
    }
}

enum Shape {
    CIRCLE(Color.RED),
    TRIANGLE(Color.GREEN),
    SQUARE(Color.BLUE);

    private final Color color;

    Shape(Color color) {
        this.color = color;
    }

    Color getColor() {
        return color;
    }
}

But I would strongly advice that you revisit your design, as cyclic dependencies are usually a code smell. If shapes are not of a certain color by their nature and the other way around, you probably need a separate abstraction that defines "red circles" and "blue squares", which has nothing to do with merely being a "circle/square" or being "red/blue".

enum Color { RED, GREEN, BLUE }
enum Shape { CIRCLE, TRIANGLE, SQUARE }

enum ColoredShape {

    RED_CIRCLE(Shape.CIRCLE, Color.RED),
    GREEN_TRIANGLE(Shape.TRIANGLE, Color.GREEN),
    MAGENTA_TRIANGLE(...) // because why not? :)
    // ......

    private final Shape shape;
    private final Color color;


    ColoredShape(Shape shape, Color color) {
        this.shape = shape;
        this.color = color;
    }

    Color color() { return color; }
    Shape shape() { return shape; }

}

This makes it all explicit and renders colors and shapes reusable in contexts other than this very specific one.

---

Another major defect of your aproach is that it does not enforce that the pair choices match both ways. I.e. nothing stops me from defining e.g. Color.RED(Shape.CIRCLE) and Shape.CIRCLE(Color.BLUE). So if you want this mapping to be consistent, you should define it in a single place and infer/reuse it the second time (or add some validation logic to ensure consistency).

Answer 2

The reason for the problem is that when Color was being initialized, Shape was not initialized yet, hence the shape of the color ends up being null. Circular dependencies are signs of bad design, so I would fix the error in the plan. Instead, you could use the same numeric values for the pairs, like this:

enum Color {
    RED,
    GREEN,
    BLUE;

    Shape getShape() {
        return Shape.values()[ordinal()];
    }
}

enum Shape {
    CIRCLE,
    TRIANGLE,
    SQUARE;

    Color getColor() {
        return Color.values()[ordinal()];
    }
}

This way you have a reliable mapping and you can get the shape of the color or the color of the shape.

Answer 3

Explanation

Enum constants are initialized like static fields, so you get a similar outcome as described here. Basically, to initialize one enum correctly, the other already needs to be fully initialized, and vice versa. Obviously, this is a circular dependency and cannot work.

As a result, the constants of the Shape enum are initialized before the initialization of Color is complete. Therefore, the fields corresponding to the Color constants RED, GREEN, and BLUE are still null when Shape accesses them during initialization.

By the way, it seems like any of the two enums could be the one that ends up incorrectly initialized, because the two form a symmetric cycle. However, the order of initialization is determined by the order in which a class is first used in the program code. In your tests, the Color enum is used first, and so its initialization is started first. Hence, the JVM loads the code of Color, which references Shape. Before the initalization of Color continues, the code of Shape is loaded. Shape in turn references Color, but since Color is already in the process of being intialized, Shape's initialization continues, using the uninitialized fields of Color. Finally, Color's initialization completes, and by now the fields of Shape contain the Shape constants, so Color initializes correctly.

Solution

I would replace one of the getters, e.g., the one in Shape with something like this:

Color getColor() {
    for (Color c : Color.values()) {
        if (c.getShape() == this) {
            return c;
        }
    }
    throw new AssertionError();
}

This way, you don't need the color field in Shape and the circular dependency is avoided.

An added benefit is that the two enums are guaranteed to be consistent. A change in the Color enum will automatically be reflected in Shape.

And if you worry about performance (I wouldn't), you could also keep the color field in Shape and initialize it the first time getColor is called:

Color getColor() {
    if (color == null) {
        for (Color c : Color.values()) {
            if (c.getShape() == this) {
                color = c;
                return color;
            }
        }
        throw new AssertionError();
    }
    return color;
}

Regarding thread-safety, it's not necessary to have any sort of synchronization here or to make the color field volatile. In case some thread does not see the initialization performed by another thread, it will simply redo the bit of work, but the result will be the same.