Which one of the two enum implementation is better?

Question

First Implementation

public enum ReviewFlowExample {
 Draft {
 @Override
 public ReviewFlowExample getNext() {
 return Review;
 }
 @Override
 public ReviewFlowExample getPrevious() {
 return null;
 }
 },
 Review {
 @Override
 public ReviewFlowExample getNext() {
 return Final;
 }
 @Override
 public ReviewFlowExample getPrevious() {
 return Draft;
 }
 },
 Final {
 @Override
 public ReviewFlowExample getNext() {
 return null;
 }
 @Override
 public ReviewFlowExample getPrevious() {
 return Review;
 }
 };
 public abstract ReviewFlowExample getNext(); 
 public abstract ReviewFlowExample getPrevious();
 public boolean isDraft() {
 return this.equals(Draft);
 }
}

---

Second Implemenation

public enum ReviewFlowExample {
 Draft,
 Review,
 Final;
 private ReviewFlowExample next;
 private ReviewFlowExample previous;
 static{
 Draft.setNext(Review);
 Review.setNext(Final);
 Review.setPrevious(Draft);
 Final.setPrevious(Review);
 }
 private ReviewFlowExample(){
 }
 public ReviewFlowExample getNext(){
 return next;
 }
 public ReviewFlowExample getPrevious(){
 return previous;
 }
 private void setNext(ReviewFlowExample next){
 this.next = next;
 }
 private void setPrevious(ReviewFlowExample previous){
 this.previous = previous;
 }
 public boolean isDraft(){
 return this == Draft;
 }
}

• Thilo: The first one. No need to set up boilerplate getters and setters when you don't need any dynamic wiring. The first one is much more concise and clearer. You should trim it down even more: What is the need for the wrappers (getNextState() and so on)?
• Shahzeb: @Thilo What would that be did not get you ? please elaborate.
• Peter Lawrey: I would say the state before Draft and after Final is null as it is in the second example.
• Peter Lawrey: Personally, I wouldn't do either. Instead I would use a state machine approach which is involves integrating how this as used as well. i.e. you introduce code as well as data in to your enums. vanillajava.blogspot.com/2011/06/…
• Shahzeb: @PeterLawrey while the link you have posted is brilliant and they way to go forward. I however need to break the tie between two approaches . So if you post your comment as an answer along with tie breaking between the two approaches in terms of better implementation. I will accept your answer.First of these implementations is mine and second one is from a colleague who thinks I am wrong and the other one is better :( I think otherwise. But yes I will go with state machine that's exactly what I needed but please help is break the tie for now :) cheers...

Answer 1

Why not use the order you are setting up in the enum declaration? I played with this a while ago and came up with something like this (modified to fit your implementation above):

private enum Planet { MERCURY, VENUS, EARTH, MARS, JUPITER, SATURN, URANUS, NEPTUNE;
 public Planet getNext() {
 return this.ordinal() < Planet.values().length - 1
 ? Planet.values()[this.ordinal() + 1]
 : this;
 }
 public Planet getPrevious() {
 return this.ordinal() > 0
 ? Planet.values()[this.ordinal() - 1]
 : this;
 }

• digitaljoel: You can find the original in my blog post at digitaljoel.nerd-herders.com/2011/04/05/… but there's not much more info there than you'll find here.

Answer 2

Just to spice up the discussion, you could also do something like this:

 public ReviewFlowExample getNext() {
 ReviewFlowExample[] values = values();
 int next = ordinal() + 1 == values.length ? ordinal() : ordinal() + 1;
 return values[next];
 }

• Shahzeb: is this dependent on the order of appearance.
• Muel: You bet :) Luckily tests can confirm order!
• Muel: Saw your comment below (I don't have general comment privileges yet). Please tell me your employer doesn't enforce alphabetical order of members!? We use IDEs for a reason!
• Dave Newton: @Muel Two jobs ago they had Jalopy set up to alphabetize properties and methods as a CVS checkin hook. It drove me insane--it essentially randomized the class. It took quite awhile before I was allowed to remove the hook :(

Answer 3

Although the other answers are correct, their readability suffers and they work only for linear workflows (and I don't like solutions which rely on declaration). I would go for readability, which is better in the second implementation. But it could be shortend:

public enum ReviewFlowExample {
 Draft,
 Review,
 Final;
 private ReviewFlowExample next = null;
 private ReviewFlowExample previous = null;
 static{
 Draft.next = Review;
 Review.previous = Draft;
 Review.next = Final;
 Final.previous = Review;
 }
 public ReviewFlowExample getNext(){
 return next;
 }
 public ReviewFlowExample getPrevious(){
 return previous;
 }
 public boolean isDraft(){
 return this == Draft;
 }
}

---

Peter suggested using a state machine instead. I think the first solution is already a state machine, there is no need to add extra interfaces. You just have to move your workflow logic into the enum. But it's hard to tell, if a state machine is worth it, without knowing the other code.