15
\$\begingroup\$

I have a method that uses a bunch of if-else statements, and I am thinking how I could simplify it.

public static CardNumber decode(String s) {
    if(s == null) {
        return null;
    } else if(s.equalsIgnoreCase(ACE.toString())) {
        return ACE;
    } else if(s.equalsIgnoreCase(TWO.toString())) {
        return TWO;
    } else if(s.equalsIgnoreCase(THREE.toString())) {
        return THREE;
    } else if(s.equalsIgnoreCase(FOUR.toString())) {
        return FOUR;
    } else if(s.equalsIgnoreCase(FIVE.toString())) {
        return FIVE;
    } else if(s.equalsIgnoreCase(SIX.toString())) {
        return SIX;
    } else if(s.equalsIgnoreCase(SEVEN.toString())) {
        return SEVEN;
    } else if(s.equalsIgnoreCase(EIGHT.toString())) {
        return EIGHT;
    } else if(s.equalsIgnoreCase(NINE.toString())) {
        return NINE;
    } else if(s.equalsIgnoreCase(TEN.toString())) {
        return TEN;
    } else if(s.equalsIgnoreCase(JACK.toString())) {
        return JACK;
    } else if(s.equalsIgnoreCase(QUEEN.toString())) {
        return QUEEN;
    } else if(s.equalsIgnoreCase(KING.toString())) {
        return KING;
    } else {
        return null;
    }
}

The whole class is here:

public class CardNumber {
    private final String name;
    private final int value;

    public static final CardNumber ACE_AS_ONE = new CardNumber("A", 1);
    public static final CardNumber TWO = new CardNumber("2", 2);
    public static final CardNumber THREE = new CardNumber("3", 3);
    public static final CardNumber FOUR = new CardNumber("4", 4);
    public static final CardNumber FIVE = new CardNumber("5", 5);
    public static final CardNumber SIX = new CardNumber("6", 6);
    public static final CardNumber SEVEN = new CardNumber("7", 7);
    public static final CardNumber EIGHT = new CardNumber("8", 8);
    public static final CardNumber NINE = new CardNumber("9", 9);
    public static final CardNumber TEN = new CardNumber("10", 10);
    public static final CardNumber JACK = new CardNumber("J", 11);
    public static final CardNumber QUEEN = new CardNumber("Q", 12);
    public static final CardNumber KING = new CardNumber("K", 13);
    public static final CardNumber ACE = new CardNumber("A", 14);

    private CardNumber(String name, int value) {
        this.name = name;
        this.value = value;
    }

    @Override
    public String toString() {
        return name;
    }

    public static CardNumber decode(String s) {
        if(s == null) {
            return null;
        } else if(s.equalsIgnoreCase(ACE.toString())) {
            return ACE;
        } else if(s.equalsIgnoreCase(TWO.toString())) {
            return TWO;
        } else if(s.equalsIgnoreCase(THREE.toString())) {
            return THREE;
        } else if(s.equalsIgnoreCase(FOUR.toString())) {
            return FOUR;
        } else if(s.equalsIgnoreCase(FIVE.toString())) {
            return FIVE;
        } else if(s.equalsIgnoreCase(SIX.toString())) {
            return SIX;
        } else if(s.equalsIgnoreCase(SEVEN.toString())) {
            return SEVEN;
        } else if(s.equalsIgnoreCase(EIGHT.toString())) {
            return EIGHT;
        } else if(s.equalsIgnoreCase(NINE.toString())) {
            return NINE;
        } else if(s.equalsIgnoreCase(TEN.toString())) {
            return TEN;
        } else if(s.equalsIgnoreCase(JACK.toString())) {
            return JACK;
        } else if(s.equalsIgnoreCase(QUEEN.toString())) {
            return QUEEN;
        } else if(s.equalsIgnoreCase(KING.toString())) {
            return KING;
        } else {
            return null;
        }
    }

    public int getValue() {
        return value;
    }
}
\$\endgroup\$

2 Answers 2

5
\$\begingroup\$

If you still want to have all the power of a class instead of an enum, another solution would be to use a map:

public class CardNumber {
    private final String name;
    private final int value;

    private static final Map<String, CardNumber> cards = new HashMap<>(14);

    static {
        cards.put("A", new CardNumber("A", 1));
        //[...]
        cards.put("K", new CardNumber("K", 13));
    }

    // [... constructor ...]

    public static CardNumber decode(String s) {
        if(s == null) {
            return null;
        } else {
            return cards.get(s);
        }
    }

    // [... toString, getValue ...]
}
\$\endgroup\$
11
  • 1
    \$\begingroup\$ With a private constructor there's no need for a class. What's this 'power of a class' that you speak of? \$\endgroup\$ Sep 1, 2014 at 19:36
  • \$\begingroup\$ @SimonAndréForsberg I overlooked the private keyword. And I'm talking about extending, etc. \$\endgroup\$
    – tim
    Sep 1, 2014 at 19:41
  • \$\begingroup\$ OK, extending I can agree with. Although you don't get much use of extending if the superclass has a private constructor. What's the 'etc.' part? :) \$\endgroup\$ Sep 1, 2014 at 20:42
  • 1
    \$\begingroup\$ Well, you can't create new instances of them. So for example if we have two sets of cards (which is not that uncommon), and the cards have a state (for example open on the table or not), that would not be possible with an enum. Although I get your point, that would require a restructure of the class anyways. \$\endgroup\$
    – tim
    Sep 1, 2014 at 21:08
  • \$\begingroup\$ Additionally, with an enum, you'd get a hashCode + equals implementation for free! Not that you need one right now because essentially, this class has been written as if it was an enum already... \$\endgroup\$ Sep 1, 2014 at 21:37
13
\$\begingroup\$

Convert your constant values to an enum instead, for example:

enum Card {
    ACE_AS_ONE("A", 1),
    TWO("2", 2),
    THREE("3", 3),
    // and so on
    KING("K", 10);

    private final String name;
    private final int value;

    Card(String name, int value) {
        this.name = name;
        this.value = value;
    }

    public static Card decode(String name) {
        for (Card card : Card.values()) {
            if (card.name.equalsIgnoreCase(name)) {
                return card;
            }
        }
        return null;
    }
}

While at it, add some unit tests to verify, for example:

public class CardTest {
    @Test
    public void testDecodeNumeric() {
        assertEquals(Card.TWO, Card.decode("2"));
        assertEquals(Card.THREE, Card.decode("3"));
    }

    @Test
    public void testDecodeLabeled() {
        assertEquals(Card.KING, Card.decode("K"));
    }

    @Test
    public void testNonexistent() {
        assertNull(Card.decode("nonexistent"));
    }

    @Test
    public void testDecodeNull() {
        assertNull(Card.decode(null));
    }
}
\$\endgroup\$
5
  • \$\begingroup\$ +1 for ACE_AS_ONE, given that in some card games 'Ace' can optionally represent the biggest value in a suit as well. \$\endgroup\$
    – h.j.k.
    Sep 2, 2014 at 3:53
  • \$\begingroup\$ Thanks @h.j.k. but ACE_AS_ONE was the OP's idea, not mine ;-) \$\endgroup\$
    – janos
    Sep 2, 2014 at 6:32
  • \$\begingroup\$ After a more detailed review, that means OP's code is buggy too... "A" will always give you CardNumber("A", 14). Oh wells... \$\endgroup\$
    – h.j.k.
    Sep 2, 2014 at 6:56
  • \$\begingroup\$ Yes, he can't get Card.ACE_AS_ONE through .decode, but we don't know the rest of his code and it might be acceptable for him, who knows. \$\endgroup\$
    – janos
    Sep 2, 2014 at 7:27
  • \$\begingroup\$ I'd drop value in favor of ordinal()+1. This may stop working if the requirements change, but they won't (and simpler code now is better than some theoretical flexibility). \$\endgroup\$
    – maaartinus
    Sep 3, 2014 at 0:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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