Optimize by adding Enum a static Holder

Question

I wonder if adding an Enum a static Holder is always a better implementation over iterating values on "get" Enum method (or similar for getting specific Enum value).

For example for Spring HttpStatus current implementation :

HttpStatus(int value, String reasonPhrase) {
    this.value = value;
    this.reasonPhrase = reasonPhrase;
}
public static HttpStatus valueOf(int statusCode) {
    for (HttpStatus status : values()) {
        if (status.value == statusCode) {
            return status;
        }
    }
    throw new IllegalArgumentException("No matching constant for [" + statusCode + "]");
}

Can be optimized with:

private static class Holder {
    private static Map<Integer, HttpStatus> enumMap = new HashMap<>();
}

HttpStatus(int value, String reasonPhrase) {
    this.value = value;
    this.reasonPhrase = reasonPhrase;
    Holder.enumMap.put(value, this);
}

public static HttpStatus valueOf(int statusCode) {
     return Holder.enumMap.computeIfAbsent(statusCode, statusCode -> {
            throw new IllegalArgumentException("No matching constant for [" + statusCode + "]"); });
}

Code optimization:

The loop version has linear time complexity (every request to get value) whereas the version using a HashMap has a time complexity of O(1).

Can this optimization have a flaw I'm missing?

Once initialized, the holder is held in memory forever. This extra memory may or may not be significant to your application/environment. — Andy Turner, Oct 14 '18 at 07:18
Theoretically, the loop version has linear time complexity (increases with number of enum values), whereas the version using a `HashMap` has a time complexit of O(1). — ernest_k, Oct 14 '18 at 07:18
@ernest_k this is an optimization, I want to know if there's a flaw in this optimization, I'll edit my question — Ori Marko, Oct 14 '18 at 07:19
@AndyTurner That's a good point, can you elaborate it to an answer? — Ori Marko, Oct 14 '18 at 07:22

Andy Turner · Accepted Answer · 2018-10-15T05:58:43.663

3

One thing to bear in mind is that, once initialized, the static map will be held in memory indefinitely; the loop approach will only hold the array created by calling values() for the duration of the loop iteration.

However, it is worth pointing out that there is no advantage in using a holder class: because you are adding to the map in the constructor, the holder class will be initialized straight away when the constructor is invoked.

As such, you may as well use a plain old static (final) field.

It is also worth considering that this approach necessarily requires the map to be mutable. You may not intend to mutate it, but it is worth changing the approach defensively so that you can't.

Instead, you can initialize directly on the field:

static final Map<Integer, HttpStatus> map = 
    Collections.unmodifiableMap(
        Stream.of(HttpStatus.values())
            .collect(toMap(s -> s.value,  s -> s)));

(Or use something like a Guava ImmutableMap)

Another point about the approach of initializing the map directly is that it doesn't add to the map in the constructor - which means that you don't actually have to put this into the enum class itself. This gives you the flexibility to use it on enums where you don't have the ability to change the code, and/or to only add the map to places where you have found there is a performance need.

edited Oct 15 '18 at 05:58

answered Oct 14 '18 at 07:31

Andy Turner

137,514
11
162
243

How will I add values to static (final) field (without holder class) ? – Ori Marko Oct 14 '18 at 07:35
@user7294900 final doesn't stop you changing the map. It just stops you assigning a different map to the field. – Andy Turner Oct 14 '18 at 07:36
I can't compile your example , for example there's no `HttpStatus.stream()` – Ori Marko Oct 14 '18 at 07:41
@user7294900 typo; fixed. – Andy Turner Oct 14 '18 at 07:42
Still compilation error: `The method toMap(( s) -> {}, ( s) -> {}) is undefined for the type HttpStatus` – Ori Marko Oct 14 '18 at 07:44
1

@user7294900 `toMap(...)` will work if you `import static java.util.stream.Collectors.toMap;` or just write `Collectors.toMap(...)`. – LuCio Oct 14 '18 at 14:08

score 0 · Answer 2 · answered Oct 14 '18 at 07:52

I have checked this approach long ago, I decided that NO BENEFITS to use Map instead of loop values manually.

Map takes place in the memory
To make map with O(1) search is really better than O(n) manual loop, enum should have more than 1000 constants (this is experimental number). But in my experience, I had enum Countries with 200+ constants max.
In the worst case, this is NOT A BOTTLENECK of the whole application either.

I always use manual loop and do not worry about performance. Many serialization frameworks (like Jackson to convert enum to json) use Enum.valueOf().

So the flaw is that it takes place in memory? it doesn't seems as a bottleneck also and it can be also lazy loaded as https://stackoverflow.com/questions/23807601/lazily-initialize-a-java-map-in-a-thread-safe-manner — Ori Marko, Oct 14 '18 at 08:07

Optimize by adding Enum a static Holder

2 Answers2