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

2 Answers 2

up vote 5 down vote accepted

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 ...]
}
share|improve this answer
1  
With a private constructor there's no need for a class. What's this 'power of a class' that you speak of? –  Simon André Forsberg Sep 1 at 19:36
    
@SimonAndréForsberg I overlooked the private keyword. And I'm talking about extending, etc. –  tim Sep 1 at 19:41
    
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? :) –  Simon André Forsberg Sep 1 at 20:42
    
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. –  tim Sep 1 at 21:08
    
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... –  Simon André Forsberg Sep 1 at 21:37

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));
    }
}
share|improve this answer
    
Your code doesn't compile for me. I think you put the public keyword before Card by mistake. –  tim Sep 1 at 19:42
    
Thanks @tim, good call, removed it –  janos Sep 1 at 19:44
    
+1 for ACE_AS_ONE, given that in some card games 'Ace' can optionally represent the biggest value in a suit as well. –  h.j.k. Sep 2 at 3:53
    
Thanks @h.j.k. but ACE_AS_ONE was the OP's idea, not mine ;-) –  janos Sep 2 at 6:32
    
After a more detailed review, that means OP's code is buggy too... "A" will always give you CardNumber("A", 14). Oh wells... –  h.j.k. Sep 2 at 6:56

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.