enums for constants in java - is this good style?

Question

In java <1.5, constants would be implemented like this

public class MyClass {
    public static int VERTICAL = 0;
    public static int HORIZONTAL = 1;

    private int orientation;

    public MyClass(int orientation) {
        this.orientation = orientation;
    }
...

and you would use it like this:

MyClass myClass = new MyClass(MyClass.VERTICAL);

Now, in 1.5 obviously you should be using enums:

public class MyClass {
    public static enum Orientation {
        VERTICAL, HORIZONTAL;
    }

    private Orientation orientation;

    public MyClass(Orientation orientation) {
        this.orientation = orientation;
    }
...

and now you would use it like this:

MyClass myClass = new MyClass(MyClass.Orientation.VERTICAL);

Which I find slightly ugly. Now I could easily add a couple of static variables:

public class MyClass {
    public static Orientation VERTICAL = Orientation.VERTICAL;
    public static Orientation HORIZONTAL = Orientation.HORIZONTAL;

    public static enum Orientation {
        VERTICAL, HORIZONTAL;
    }

    private Orientation orientation;

    public MyClass(Orientation orientation) {
        this.orientation = orientation;
    }
...

And now I can do this again:

MyClass myClass = new MyClass(MyClass.VERTICAL);

With all the type-safe goodness of enums.

Is this good style, bad style or neither. Can you think of a better solution?

Update

Vilx- was the first one to highlight what I feel I was missing - that the enum should be a first-class citizen. In java this means it gets its own file in the package - we don't have namespaces. I had thought this would be a bit heavyweight, but having actually done it, it definitely feels right.

Yuval's answer is fine, but it didn't really emphasise the non-nested enum. Also, as for 1.4 - there are plenty of places in the JDK that use integers, and I was really looking for a way to evolve that sort of code.

Answer 1

Did you know you can import Orientation and say

MyClass myClass = new MyClass(Orientation.VERTICAL);

?

Answer 2

Don't know about Java, but in .NET the good practice is to put enums in parallel to the class that uses them, even if it is used by one class alone. That is, you would write:

namespace Whatever
{
    enum MyEnum
    {
    }
    class MyClass
    {
    }
}

Thus, you can use:

MyClass c = new MyClass(MyEnum.MyValue);

Answer 3

It depends on how many values the enum can take. In your example, with only two, I would just use a boolean. If the enum will only be used by code that you write and won't have to interact with lots of other code, maybe you don't need type safety. But if it's in a 'public' method, I would definitely go for enums, and put the enum in its own file.

Answer 4

You complicated it too much. Let's bring it all together.

Post Java 1.5 you should use the Java Enum class:

public enum Color
{
    BLACK, WHITE;
}

Pre Java 1.5 you should use the type-safe Enum pattern:

public class Color
{
    public static Color WHITE = new Color("white");
    public static Color BLACK = new Color("black");

    private String color;

    private Color(String s)
    {
        color = s;
    }
}

In both ways you call it like so:

drawBackground(Color.WHITE);

Specifically, regarding your question. It's a matter of code style, but I think the preferred way is to keep enums in their seperate classes. Especially once they start to get their own methods like getName(), getId(), etc... Think of it as the same dilemma as regular class vs. anonymous class, once the class starts to get cluttered, it's time to move it out to its own file.

Answer 5

You can also have two static methods on MyClass:

MyClass.Vertical() : MyClass
MyClass.Horizontal() : MyClass

Those will return a new instance with proper enum set.

Answer 6

I agree you were creative but I think it's not a practical solution and I think you just shifted the "ugliness" to a different part of the code. What happens if in addition to VERTICAL and HORIZONTAL, you'll also have DIAGONAL, AA, BB, CC, etc? Are you going to have to duplicate by typing each and every static constant? Your taste that MyClass.Orientation.VERTICAL is ugly might be personal?