Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
}
share|improve this question
add comment

4 Answers

up vote 6 down vote accepted

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);
}
share|improve this answer
    
Thx for the feedback and the suggestions, I'll refactor mine code following your suggestions. –  chillworld 21 hours ago
add comment

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.

share|improve this answer
    
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. –  chillworld 20 hours ago
add comment

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;
    }
}

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.

share|improve this answer
1  
I'm against deletion of this answer. You are encouraged to answer your own questions. The problem I see with this is, that you are asking for new advice. This belongs into a new question or possibly in a separate section of the current question (as edit). For clarification purposes you should comment instead. If you want to ask for opinions, it's probably best, if you visit us in Code Review Chat. –  Vogel612 19 hours ago
    
@Volgel612 edited the answer so the asking for advice is gone. I'm coming now on the chat ;) –  chillworld 19 hours ago
add comment

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;
  }

}

Warning

Do not do it this way! I get a NullPointerException for some reason. Investigating...

Found the problem - do not use EnumSet.noneOf! It accesses the enum before it has been initialised.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.