Enum with getting subset

Question

I just wrote this but I'm not sure if this is good practice. Can someone give me advice as to whether it's good or how I can do better or even it's bad code?

The point is that I need a sub List-array-set of the enum. The SubType.ELSE shall have another name in reality (I know that was a bad choice, but it's for pointing it out).

public enum ContentType {
 TITLE("$$title$$",SubType.MAIL),
 BODY("$$body$$",SubType.MAIL),
 MESSAGE("$$message$$",SubType.MAIL),
 EVENTS("$$events$$",SubType.ELSE);
 private final String replaceWord;
 private final SubType subType;
 private ContentType(String replaceWord, SubType subType ) {
 this.replaceWord = replaceWord;
 this.subType = subType;
 }
 public String getReplaceWord() {
 return replaceWord;
 }
 private SubType getSubType() {
 return subType;
 }
 public static List<ContentType> values(SubType subType) {
 List<ContentType> subList = new ArrayList<ContentType>();
 for (ContentType type : ContentType.values()) {
 if (type.getSubType() == subType) {
 subList.add(type);
 }
 }
 return subList;
 }
}
enum SubType {
 MAIL,
 ELSE;
}

• palacsint: I would not complicate the code with hashmap/enummap/etc caches as the answers suggest. It seems premature optimization.
• Blessed Geek: The correct way is to avoid for/while/loops when you have an enum and to use EnumSet.
• chillworld: @BlessedGeek thx for the comment, you can see in mine own answer where I did put the revised code that it was taken in account.
• Blessed Geek: stackoverflow.com/questions/25219219/…

Answer 1

I suggest two changes to values(SubType):

• Return a Set instead of a List.
• Prepare all the possible results at class-loading time, so that values(SubType) could be accomplished by a simple lookup. The code is slightly more complicated, but the runtime performance should be better.

private static final Map<SubType, Set<ContentType>> BY_SUBTYPE;
static {
 BY_SUBTYPE = new HashMap<SubType, Set<ContentType>>();
 for (SubType subType : SubType.values()) {
 BY_SUBTYPE.put(subType, new TreeSet<ContentType>());
 }
 for (ContentType type : ContentType.values()) {
 BY_SUBTYPE.get(type.getSubType()).add(type);
 }
 for (SubType subType : BY_SUBTYPE.keySet()) {
 // Make Set<ContentType> values immutable
 BY_SUBTYPE.put(subType, Collections.unmodifiableSet(BY_SUBTYPE.get(subType)));
 }
}
public static Set<ContentType> values(SubType subType) {
 // Returns an unmodifiable view of a SortedSet
 return BY_SUBTYPE.get(subType);
}

Answer 2

I agree mostly with @200_success' answer, though for simplicity and maintainability I do suggest you to use the following, which is only possible in Java 8. It functionally does the same as @200_success' answer.

The code with explanation below:

public enum ContentType {
 TITLE("$$title$$", SubType.MAIL),
 BODY("$$body$$", SubType.MAIL),
 MESSAGE("$$message$$", SubType.MAIL),
 EVENTS("$$events$$", SubType.DEFAULT);
 private static final Map<SubType, Set<ContentType>> SETS_BY_SUBTYPE;
 static {
 SETS_BY_SUBTYPE = Arrays.stream(values())
 .collect(Collectors.groupingBy(
 contentType -> contentType.getSubType(),
 Collectors.toSet()
 ));
 }
 private final String replaceWord;
 private final SubType subType;
 private ContentType(final String replaceWord, final SubType subType) {
 this.replaceWord = replaceWord;
 this.subType = subType;
 }
 public String getReplaceWord() {
 return replaceWord;
 }
 private SubType getSubType() {
 return subType;
 }
 public static Set<ContentType> values(final SubType subType) {
 return SETS_BY_SUBTYPE.get(subType);
 }
}
enum SubType {
 MAIL,
 DEFAULT;
}

To explain it more precisely, consider only the code that generates the mapping:

SETS_BY_SUBTYPE = Arrays.stream(values())
 .collect(Collectors.groupingBy(
 contentType -> contentType.getSubType(),
 Collectors.toSet()
 ));

What it does is:

1. Obtain a ContentType[].
2. Convert the array to a Stream<ContentType>, this is the starting point of functional programming.
3. Collect the stream in a data structure, here we want to have a Map<SubType, Set<ContentType>>.
4. The unoverloaded version of Collectors.groupingBy groups elements on a certain property, here it is contentType.getSubType(). The caveat with the default version is that it returns a List<ContentType> whereas we want a Set<ContentType>.
5. So we need to supply a downstream argument to the method, which in this case becomes a Collectors.toSet() downstream collector.

Another name suggestion would be to change SubType.ELSE to SubType.DEFAULT, where default semantically means that it does not belong to something else.

Other small remarks are about the horizontal white space, please be consistent there:

• Your enum declarations were missing one between the arguments.
• In one method there was too much horizontal whitespace.

I personally value horizontal whitespace and consistent looking code quite a bit as it in my opinion indicates how seriously you are working with your code.

Since the answer of @chillworld included using an EnumMap, which is argubly better, at least for improved performance, we can construct it with the following:

private static final EnumMap<SubType, Set<ContentType>> SETS_BY_SUBTYPE;
static {
 SETS_BY_SUBTYPE = Arrays.stream(values())
 .collect(Collectors.groupingBy(
 ContentType::getSubType,
 () -> new EnumMap<>(SubType.class),
 Collectors.toSet()
 ));
}

Here we explicitely specify an EnumMap over the general Map interface and to obtain it we use the third parameter of Collectors.groupingBy, which is one that takes a mapFactory as argument, which means that you need to provide the map yourself here.

Another difference is that we here use a method reference, ContentType::getSubType, over contentType -> contentType.getSubType() for improved readability.

• chillworld: Thx for the feedback, but I can't do anything with it cause I'm obligatory to use java 6, of course not of free will :D.

Answer 3

Oke after reading all your suggestions I refactored the code to this :

public enum ContentType {
 TITLE("$$title$$"),
 BODY("$$body$$"),
 MESSAGE("$$message$$"),
 EVENTS("$$events$$");
 private static final Map<SubType, Set<ContentType>> BY_SUBTYPE;
 private final String replaceWord;
 static {
 //Get a map with key's all the values of the SubType class.
 BY_SUBTYPE = new EnumMap<SubType, Set<ContentType>>(SubType.class);
 //Fill the key for MAIL.
 BY_SUBTYPE.put(SubType.MAIL, Collections.unmodifiableSet(EnumSet.range(TITLE, MESSAGE)));
 //Fill the key for DEFAULT.
 BY_SUBTYPE.put(SubType.DEFAULT, Collections.unmodifiableSet(EnumSet.of(EVENTS)));
 }
 private ContentType(String replaceWord) {
 this.replaceWord = replaceWord;
 }
 public static Set<ContentType> values(SubType subType) {
 // Returns an unmodifiable view of a SortedSet
 return BY_SUBTYPE.get(subType);
 }
 public String getReplaceWord() {
 return replaceWord;
 }
 public enum SubType {
 MAIL,
 DEFAULT;
 }
}

Explication :

It started with netbeans suggesting to use the EnumMap.
After some reading about EnumMap and EnumSet, I'll come up with this.Don't use that a lot :)

BY_SUBTYPE = new EnumMap<SubType, Set<ContentType>>(SubType.class); :
We create an EnumMap whitch has as keys all the values of SubType.
You can't create more keys and null keys are not allowed.

EnumSet.range(TITLE, MESSAGE); :
This gives us a EnumSet witch contains every Enum value from TITLE to MESSAGE.

EnumSet.of(EVENTS);
This gives us a EnumSet witch contains only EVENTS.

Refactoring is more difficult then @200_succes his answer, but I don't have to iterate 2 times over the SubType and one time over the ContentType.

Edit :

I did have to move the subtype to the EnumClass itself, otherwise I couldn't reach it from outside the Enum.

• Simon Forsberg: I advice against using EnumSet.range(TITLE, MESSAGE); my experience is that it's just a matter of time before someone (most likely you) change the order of the enums, and then you do not want to use code that depends on a specific enum ordering. Trust me, it has happened before...
• chillworld: I could use the EnumSet.of(TITLE,BODY,MESSAGE)

Answer 4

I was just playing with this and came up with this rather neat (IMHO) solution so I thought I should post.

Essentially, I let the SubType enums hold a Set of the ContentType that link to them. You then do your query on the sub types.

enum SubType {
 MAIL,
 ELSE;
 // This way breaks! We are trying to access the ContentType before it is initialised.
 // Set<ContentType> contentTypes = EnumSet.noneOf(ContentType.class);
 Set<ContentType> contentTypes = new HashSet<>();
 public void addContentType(ContentType add) {
 contentTypes.add(add);
 }
 public Set<ContentType> getContentTypes() {
 return contentTypes;
 }
}
public enum ContentType {
 TITLE("$$title$$", SubType.MAIL),
 BODY("$$body$$", SubType.MAIL),
 MESSAGE("$$message$$", SubType.MAIL),
 EVENTS("$$events$$", SubType.ELSE);
 private final String replaceWord;
 private final SubType subType;
 private ContentType(String replaceWord, SubType subType) {
 this.replaceWord = replaceWord;
 this.subType = subType;
 subType.addContentType(this);
 }
 public String getReplaceWord() {
 return replaceWord;
 }
 private SubType getSubType() {
 return subType;
 }
}