Gfrktyl

While reviewing a PR, I found an enum being used like this:

class Entity {

    public enum STATUS { NEW, SENT, FAILED, OK }



    @Column(name = "STATUS") // a NUMBER(1) column in DB

    private int status; // + get/set

    //...

}



// Inside methods...

Entity e = new Entity();

e.setStatus(STATUS.SENT.ordinal());

EntityDAO.save(e);



Entity e = EntityDAO.findByStatus(STATUS.FAILED.ordinal());

logger.info("Found entity with status: " + STATUS.values()[e.getStatus()]);

Assume the necessary imports etc.

I find this use of ordinal() to be an antipattern, and consider that it should be avoided.

Instead, an inner field with the equivalent DB representation (e.g. NEW(1) or NEW("N")) should be used, in order to decouple the semantically meaningful representation of the enum value in the code (NEW) from the DB-optimized representation of such value when persisted (1 or 'N' or whatever).

Now, I don't have this POV for nothing: we've already had bugs stemming from altering the order of enum elements that were being used based on its ordinal() value (some times because of adding new values in between, some times because someone decided the values looked better in alphabetical order ¯_(ツ)_/¯).

That's the case I made in the PR comments -- but in the end the PR got approved anyway, reasoning that my proposed correction would add too much clutter to the enum for little gain, and that we should all just be careful with enums.

And so the enum made its way to production.

Time has passed, and now I've got the chance to change that enum so its usage is not tied to the ordering of its elements.

However, since this has been in production for quite some time, I have to make it so the current DB values (0, 1, 2...) are kept and still interpreted correctly (as changing the DB values or column type would be too costly now).

This is the first version I came up with:

public enum STATUS {

    NEW (0),

    SENT (1),

    FAILED (2),

    OK (3),

    ;



    private final int val;



    STATUS (int val) { this.val = val; }



    public int val() { return val; }



    public static STATUS get(final int val) {

        switch (val) {

            case 0: return NEW;

            case 1: return SENT;

            case 2: return FAILED;

            case 3: return OK;

            default: throw new IllegalArgumentException();

        }

    }

}

I think this is as concise as it can get.

However, if a dev wanted to add a new enum value, they'd have to modify both the enum list and the switch inside the get(int) method. I find this to be inconvenient and, in a way, ammunition for my peers in favor of just using ordinal().

So I came up with this second version:

public static enum STATUS {

    NEW (0),

    SENT (1),

    FAILED (2),

    OK (3),

    ;



    private static final STATUS values;

    static { // like caching values() but sorted by dbValue

        STATUS st = values();

        values = new STATUS[st.length];

        for (STATUS s : st) {

            values[s.dbValue] = s;

        }

    }

    public static STATUS fromDB(final int dbValue) {

        return values[dbValue];

    }



    private final int dbValue;

    public int toDBValue() {

        return dbValue;

    }



    STATUS(int dbValue) {

        this.dbValue = dbValue;

    }

}

In this version, adding a new enum element is as simple as adding it to the list, just like a normal enum. And getting an element from its DB value is O(1) and rather efficient (compared to, say, using Streams to filter values() by dbValue every time). But maybe it is too complex for an enum? Also, this relies on DB values being consecutive numbers, as adding e.g. CANCELLED(99) would throw an AIOOB exception upon execution.

So my question is: how can I enhance this enum so adding elements to it is as simple as possible, getting elements by value is as efficient as possible, and it doesn't depend on ordinal()?

A negative parameter value or a parameter value >= values.length passed to fromDB() will throw an IndexOutOfBoundsException, which is less meaningful than the original IllegalArgumentException since the IndexOutOfBoundsException is complaining about an implementation detail rather than about bad input.

To avoid the IndexOutOfBoundsException, use a Map<Integer, STATUS> instead of an array.

However, if there are only a few defined values as in the question, a Map is probably not needed either. fromDB() can simply iterate over values() and return the match when found, otherwise throw IllegalArgumentException. Add the Map when performance profiling indicates fromDB() is a bottleneck.

Also, consider changing STATUS to Status, to follow standard naming conventions.