How to make an enum isXXX() method?

Question

How would I insert isRED() and isBLACK() methods into this enum? I can't figure it out - even after googling for some time.. I don't know what value to access.

enum Suit {
   SPADES,
   HEARTS,
   DIAMONDS,
   CLUBS;
};

the main benefit of this for me is to simplify my calls.. (card.isRED()) is much shorter than (card == EnclosingClass.Suit.HEARTS || card == EnclosingClass.Suit.DIAMONDS);

and I have many such in my code

Possible duplicate of [Java Enum Methods](http://stackoverflow.com/questions/18883646/java-enum-methods) — Cory Klein, Oct 15 '15 at 21:20

Jon Skeet · Accepted Answer · 2015-10-15T21:46:40.980

The simplest approach would probably be to have a boolean field indicating whether or not that suit was red. For example:

enum Suit {
   SPADES(false),
   HEARTS(true),
   DIAMONDS(true),
   CLUBS(false);

   private final boolean red;

   private Suit(boolean red) {
     this.red = red;
   }

   public boolean isRed() {
     return red;
   }
}

I would probably not add an isBlack method, instead relying on callers to use if (!foo.isRed()), but that's a separate matter. As noted in comments, if "red or black" aren't strictly opposites, or you anticipate them not being opposites in the future, you might want isBlack() - although in that case I'd at least start with an implementation which returned !isRed() and then changed it later for suits which were either both red and black or neither, as the need arose.

This feels right to me simply because the colour is essentially a piece of state about the value. While you obviously can determine it by checking against known-red suits, I tend to regard fields as the most natural way of expressing state. It's not like it's going to add much memory :)

Three alternatives:

1: Put the logic into the method itself:

enum Suit {
   SPADES,
   HEARTS,
   DIAMONDS,
   CLUBS;

   public boolean isRed() {
     return this == HEARTS || this == DIAMONDS;
   }
}

The downside of this solution is that it's error-prone when you add a new value - the compiler's not going to prompt you to look at the isRed method and consider whether or not you want to add another case there.

2: (Ugly) make it an abstract method that each suit overrides.

enum Suit {
   SPADES {
     @Override public boolean isRed() { return false; }
   },
   HEARTS,
     @Override public boolean isRed() { return true; }
   },
   DIAMONDS,
     @Override public boolean isRed() { return true; }
   },
   CLUBS {
     @Override public boolean isRed() { return false; }
   };

   public abstract boolean isRed();
}

3: Like 2, but give a "default" implementation returning one result, and only override it in the others.

Personally I'd go with the field, as per the first solution.

4: Define a `Color` enum, and have pass an instance of that instead of the boolean parameter. It might be a little clearer as to the meaning of that parameter, and allows you to return a meaningful `getColor()` method. — Andy Turner, Oct 15 '15 at 21:29
@AndyTurner: Yes, that's true - although if you only want Red/Black you'd need to decide whether you want your `Color` enum to only represent those values, or perform extra validation. But yes, basically this is an extension to the first approach. — Jon Skeet, Oct 15 '15 at 21:30
I know it's basically the same as the boolean approach, that's why I'm not providing it in my own answer :) — Andy Turner, Oct 15 '15 at 21:31
I would say that you *should* add an `isBlack` method, rather than making callers infer stuff, because you next poker game uses Jokers, and they don't have a color. *I like alternative #1 better* — Andreas, Oct 15 '15 at 21:39
@Andreas: At that point I suspect that you'd already have a bunch of code which assumes a card is either red or black. Point taken, but the redundancy still feels off to me. — Jon Skeet, Oct 15 '15 at 21:40
I don't know.. my opinion is that it is cleaner to have `isBlack()` as well.. I think the point of OOP is to shove a lot into objects so that calling them is cleaner.. sure you've added a few lines of code in the class, but that's where it belongs. Meanwhile your code is easier to read on the outside.Avoiding multiple `!isRed()` is desirable in my opinon.What if it was Left/Right or Up/Down.. `!isLeft(), !isUp()`... cleaner just to put an extra few lines in the class. — ycomp, Oct 16 '15 at 00:52

score 6 · Answer 2 · answered Oct 15 '15 at 21:25

6

For a small enum like this, you can simply enumerate the red and black values:

enum Suit {
   SPADES,
   HEARTS,
   DIAMONDS,
   CLUBS;

   boolean isRED() {
     return this == HEARTS || this == DIAMONDS;
   }

   boolean isBLACK() {
     return this == SPADES || this == CLUBS;
   }
}

answered Oct 15 '15 at 21:25

Andy Turner

137,514
11
162
243

ah, this is what I was looking for - I didn't realize you could just test **this** – ycomp Oct 15 '15 at 21:26
1

@ycomp if you're doing it like I suggest, it's not in a static context. – Andy Turner Oct 15 '15 at 21:32
@ycomp: If you can show an example in the question, it'll be easier to help you... I've edited my answer to explain a bit about why I prefer the field approach over this one. – Jon Skeet Oct 15 '15 at 21:32
yeah I was being stupid, I had the static modifier in the methods I had moved into the enum..removed those and it works fine ) – ycomp Oct 15 '15 at 21:39
@JonSkeet, my enum is really very simple - no need but thanks, it's working now – ycomp Oct 15 '15 at 21:42

How to make an enum isXXX() method?

2 Answers2