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 am developing a BlackJack game using Java, and it came to a point that I am using instanceof operator to determine if it is a type of some subclass.

Here's an example:

public void checkForBlackJack(Player player) {
        Hand fHand = player.getHands().get(0);
        if (fHand.getCardScore() == 21) {
            fHand.setBlackjack(true);
        }

        if(player instanceof BlackJackPlayer){
            BlackJackPlayer bjplayer = (BlackJackPlayer) player;

            if(bjplayer.isSplit()){
                //Check the users other hand if it is already blackjacked
            }
        }
    }

Is using instanceof considered to be bad for such example? Just as an overview, here is my class diagram:

enter image description here

I've decided to use instanceof to check for blackjack because The Dealer can also get blackjack.

share|improve this question
    
In the most of the cases you can get rid of instanceof by implementing the visitor pattern. But this would be a bit over-engineered in your case. –  user714965 Oct 30 '12 at 10:46
add comment

4 Answers 4

In this case you could push the isSplit method to the Player class and implement it with return false;. Then the need to instanceof disappears.

I wouldn't say that it is bad to use instanceof but often there are other options that are better.

share|improve this answer
add comment

As a first pass at a refactor, I would structure your code like this:

public class Player {
    public boolean hasBlackJack() {
        // Handle stuff outside of your "if instanceof" above
    }
}

public class BlackJackDealer extends Player {
    // Other methods, not overriding hasBlackJack method
}

public class BlackJackPlayer extends Player {
    @Override
    public boolean hasBlackJack() {
        super();
        // Handle stuff inside your  "if instanceof" above
    }
}

Though this does get rid of the instanceof, the real reason I prefer it is a little deeper. Who is responsible for determining if a Player has a blackjack? The player of course! This is shown in your code when you're doing something like:

Hand fHand = player.getHands().get(0);

Woah; why is this Object reaching inside the Player object to get his hands out of there? Instead, ask the player object whether he has a blackjack, and proceed accordingly. Notice you can still have a "GameRunner" class responsible for deciding what to do next if the player has a blackjack (payout appropriately, remember betsize, etc), but checking if there is a blackjack is a question you can ask the Player object.

Going a little bit further, this code seems dodgy to me:

    if (fHand.getCardScore() == 21) {
        fHand.setBlackjack(true);
    }

An external entity shouldn't be telling the Hand that it is a blackjack and then mutating the Hand. Is there a situation where 21 is not a blackjack? If not, you can remove this call entirely and just use getCardScore() == 21 to determine if it's a blackjack. If there is, then you can write the method isBlackjack() in the Hand class itself.

share|improve this answer
add comment

In your case, it seems like the checkForBlackJack method lives in the wrong place (I am guessing a class containing game logic). If you move1 that method to the Player class you can then get rid of the conditional logic by breaking it out to an abstract method. This is commonly referred to as "replace-conditional-with-polymorphism"2. In case you have common behaviour in the two subclasses, you can put that behaviour in the base class instead of making that one abstract (so you won't have to repeat yourself).

1: move method

2 replace conditional with polymorphism

share|improve this answer
add comment

why don't you declare a function for both types of Player?

public void checkForBlackJack(BlackJackPlayer blackjackplayer) {
  // ....
 }

public void checkForBlackJack(BlackJackDealer blackjackdealer) {
  // ....
 }
share|improve this answer
    
if I did that I would be repeating myself, since both has the sample implementation the only difference is that the BlackJackPlayer can Have two hands. –  user962206 Oct 28 '12 at 10:07
    
@user962206 You can extract everything in common into a separate method called from these two. –  Sulthan Nov 5 '12 at 13:58
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.