9
\$\begingroup\$

I have decided to create a console Blackjack game as my final Java project. Please take a look at my code and give me some feedback.

I have tried to make the game as object-oriented as possible. I have five different classes.

Player class:

public class Person extends Dealer{

Random ran = new Random ();
Scanner scan = new Scanner (System.in);
public static int personcard, persontotal;
static ArrayList <String> name = new ArrayList <String> ();


public void shufflePerson () throws InterruptedException {

    name.add("Bert");

    personcard = ran.nextInt(14);

    while (personcard < 1) {
        personcard = ran.nextInt(14);
    }

    switch (personcard) {

    case 11:
        System.out.println(name + " drew a Jack");
        persontotal = 10 + persontotal;
        Thread.sleep(2500);
        System.out.println(name +"s total is: " + persontotal);
        break;

    case 12:
        Thread.sleep(2500);
        System.out.println(name +" drew a Queen");
        persontotal = 10 + persontotal;
        Thread.sleep(2500);
        System.out.println(name +"s total is: " + persontotal); 
        break;
    case 13:
        Thread.sleep(2500);
        System.out.println(name +" drew a King");
        persontotal = 10 + persontotal;
        Thread.sleep(2500);
        System.out.println(name + "s total is: " + persontotal);    
        break;

    case 14:
        Thread.sleep(2500);
        System.out.println(name +" drew an Ace");
        persontotal = 11 + persontotal;
        Thread.sleep(2500);
        System.out.println(name + "s total is: " + persontotal);    
        break;

    default:
        Thread.sleep(2500);
        System.out.println(name + " drew a: " + personcard);
        persontotal = persontotal + personcard;
        Thread.sleep(2500);
        System.out.println(name + "s total is: " + persontotal);
    }

    if (persontotal > 21) {
        System.out.println(name +" busted.");
        playAgain p = new playAgain ();
        p.again();
    }
    else {

    System.out.println("Hit or stay?");

    String hitorstay = scan.nextLine();

    if (hitorstay.equals("hit")) {
        shufflePerson();
    }
    else {
        System.out.println("Okay, dealers turn.");
        Dealer d = new Dealer ();
        d.shuffleDealer();
    }
    }
}

Dealer class:

public class Dealer {
Random ran = new Random ();
public static int dealercard, dealertotal;

public void shuffleDealer () throws InterruptedException {

    dealercard = ran.nextInt(14);

    while (dealercard < 1) { 
        dealercard = ran.nextInt(14);
    }
    switch (dealercard) {
    case 11:
        System.out.println("Dealer drew a Jack");
        dealertotal = 10 + dealertotal;
        Thread.sleep(2500);
        System.out.println("Dealers total is: " + dealertotal);
        break;


    case 12:
        Thread.sleep(2500);
        System.out.println("Dealer drew a Queen");
        dealertotal = 10 + dealertotal;
        Thread.sleep(2500);
        System.out.println("Dealers total is: " + dealertotal);
        break;

    case 13:
        Thread.sleep(2500);
        System.out.println("Dealer drew a King");
        dealertotal = 10 + dealertotal;
        Thread.sleep(2500);
        System.out.println("Dealers total is: " + dealertotal);
        break;


    case 14:
        Thread.sleep(2500);
        System.out.println("Dealer drew an Ace");
        dealertotal = 11 + dealertotal;
        Thread.sleep(2500);
        System.out.println("Dealers total is: " + dealertotal);
        break;


    default:
        System.out.println("Dealers drew a " + dealercard);
        // vår formel för total
        dealertotal = dealercard + dealertotal;
        Thread.sleep(2500);
        System.out.println("Dealers total is: " + dealertotal);
    }

    if (dealertotal > 21) {
        System.out.println("Dealer busted.");
        playAgain p = new playAgain ();
        p.again();
    }

    else if (dealertotal < 15) { 
        System.out.println("Dealer chooses to hit.");
        shuffleDealer ();
    }
    else {
        System.out.println("Dealer chooses to stay.");
        displayWinner w = new displayWinner ();
        w.showWinner();
    }
}

displayWinner class:

public class displayWinner extends Person {

public void showWinner () throws InterruptedException {

    if (persontotal > dealertotal) {
        System.out.println(name +" won!");
        playAgain p = new playAgain ();
        p.again();
    }
    else if (dealertotal > persontotal){
        System.out.println("Dealer won!");
        playAgain p = new playAgain ();
        p.again();

    }
    else {
        System.out.println("Draw.");
        playAgain p = new playAgain ();
        p.again();
    }
}

playAgain class:

public class playAgain {
Scanner scan = new Scanner (System.in);


public void again () throws InterruptedException {

    System.out.println("Game ended. Play again? y/n");
    String yn = scan.nextLine();
    if (yn.equals("y")) {
        Person p = new Person ();
        p.shufflePerson();
    }
    else {
        System.out.println("Goodbye.");
        System.exit(0);
    }
}

Main class:

public static void main(String[] args) throws Exception {
    System.out.println();
    System.out.println("~~~~~~~~~~~~~~~~~~~~~~~~~");
    System.out.println("~   Welcome     ~");
    System.out.println("~   to BlackJack!   ~");
    System.out.println("~~~~~~~~~~~~~~~~~~~~~~~~~");
    System.out.println("\nShuffling cards...");
    Thread.sleep(0);



    Person p = new Person ();
    p.shufflePerson();
}
\$\endgroup\$
2
  • 5
    \$\begingroup\$ If you want to remove this post now after having posted it, I'd suggest flagging it for moderator attention and having a good reason for why you want it removed. It is also possible for the post to remain but unassigning your username from the post (which is more preferred by the community instead of removing the entire post). \$\endgroup\$ Oct 13, 2014 at 14:03
  • \$\begingroup\$ Similar post, different objective: codereview.stackexchange.com/q/46439/37991 \$\endgroup\$ Oct 14, 2014 at 3:54

6 Answers 6

13
\$\begingroup\$
  1. Classes in Java should start with a capital letter, ex: PlayAgain rather than playAgain.

  2. If the class name is a verb like play again this means that this shouldn't be a class. Class names should usually be nouns like Person. If you feel that your class name should be a verb then it is a probably a function and not a class, ex:

    void playAgain(){
    }
    
  3. Your inheritance hierarchy does not make sense. If A extends B, this means A is B all the time, and this condition does not hold in you hierarchy. Is every Person a Dealer? I don't think it is. It is actually the other way around; every Dealer is a Person, inheritance meaning IS. And if you want to avoid the hassle of inheritance, use composition instead. It is usually a safer option

  4. Always try to restrict access to your fields by declaring them private rather than package access. It's Java's fault that everything is not private by default.

  5. Scanner is a field of the Person class, which doesn't make sense. Think of fields as properties: what does this person have? For you to qualify as a person you shouldn't need a Scanner. Maybe you need a name or a date of birth, but not a Scanner.

  6. When you need string equality with a constant, start with the constant. For example, use:

    if ("y".equals(yn)) {
    ////
    }
    

    instead of

    if (yn.equals("y")) {
    ///
    }
    

    because the second version will throw a NullPointerException if yn is null, whereas the first version gives you null checking for free.

\$\endgroup\$
11
\$\begingroup\$

The overall object oriented design is not natural:

  • A Person is a kind of Dealer? Normally it's the other way around: Person is the more general type, Dealer is a sub-type of Person. In real life, everybody is a Person but not everybody is a Dealer, right?

  • A Person has a shufflePerson method... What does that even mean? If you put that in an English sentence, it doesn't make sense: you cannot "shuffle person a person". Shouldn't it be a Deck that you shuffle during a card game? Same goes for Dealer and shuffleDealer

  • Many methods throwing InterruptedException... What would this correspond to in real life, when playing black jack in a casino or with friends? It doesn't correspond to anything, and so it shouldn't be part of the public interface of your implementation (= public methods).

  • displayWinner is a poor name for a class. It sounds like an action. OOP is "object oriented programming", not "action oriented programming". Nouns make good class names instead of action verbs. When you cannot describe what your class does using a noun, ask yourself if the class represents a good object that is a natural model of reality or a concept in the specific problem domain.

    What's even worse, this class extends Person. When a ClassA extends ClassB, ask yourself: "is ClassA a ClassB"? In this case: is displayWinner a Person? It is not. So it should not be a sub-class of Person.

  • A playAgain class... What kind of object is a playAgain object? What does it represent? The object here should have been perhaps the Game: if you give me a Game, I will naturally assume it's something I can play with. But if you give me a "playAgain", what to make of that?

How to redesign this?

  • Rethink the basic classes that should make up the program. It's good to take reality as an example. Here are some classes that intuitively to mind: Dealer, Player, Card, Deck, Hand, Game.

  • Think of the responsibilities of each class, what actions you could perform on them (= methods), what other objects they should have (= fields). For example:

    • A common Deck is needed, that will be used by both the Dealer and the Player
    • The Deck can be shuffled
    • A Player has a Hand
    • A Hand can be a winning hand, or a losing hand, or neither
    • ... (keep thinking)

    All these seem natural and logical assumptions about how the program can work. Having a natural design makes implementation easier, less confusing, less complex.

  • Write down the methods that each class should have, but without implementing anything. It should all fit on 1-2 pages. Read through it, it will serve as a high-level overview of your program. Inspect each class and method to verify it makes logical sense. Rework it, move components around if you spot logical mistakes, like what I wrote about Person.shufflePerson, which should really have been Deck.shuffle.

  • Iterate. After you're done with the first design, you might notice new opportunities for a better design. Even the best programmers do this and throw away their 1st, 2nd, 3rd design until they finally arrive at something that doesn't make them want to throw up.

\$\endgroup\$
8
\$\begingroup\$

I'm going to look at this from a relatively high level aspect, I may edit in some more specific things if they occur to me.

A way to think about the problem

A good way to think about this sort of thing is think about the nouns that are involved, and see if they make sense as classes. In this situation, it's a game of cards, so Person or Player makes sense, and yes, it's possible that Dealer is something else to consider, but they act as a Player as well, just one with extra responsibilities, so subclassing Player seems like a good bet. As well as that, we have the Deck to think about, which is made of Card objects. And that seems to be about everything. Thinking about problems like this in terms of SINGULAR nouns (so no plurals - a Deck is compromised of Card objects, NOT Cards) helps break it down.

Hierarchy

It doesn't make much sense to me that Person extends Dealer, I'd expect it to be the other way around, but that may be an issue with names. Also, it may be me missing something, but there doesn't seem to be any benefit to there being a hierarchy there, as there is no overriding of methods or accessing lower member variables that I am noticing.

I'd recommend that you either had only one class, Person, say, and for the dealer gave them the name "Dealer", and where you need to access the name, access that. Either that, or begin to override methods and make use of the sub-classing that you are using.

Alternate card structure

I feel like you should maybe have a Card and a Deck group of classes, so one representing a Card, which can just be as simple as an enum representing the suit and one representing the rank. In the Deck, it can just be an array of cards, each with a inDeck or a dealt boolean representing whether it has been dealt to a player. As well as this, you can put Shuffle and getCard methods in it, and have everyone draw from the same Deck.

Different responsibilities

I think that Player should just have a currentScore int, possibly a hand, which could be an ArrayList of Card objects (which could mean that there's no need for currentScore, as it could be trivially calculated), and they are just responsible for twisting or sticking.

Dealer needs to do all of these things, but they could also have-a deck, from which all of the players draw.

\$\endgroup\$
7
\$\begingroup\$

Randomize an integer

This is not right!

dealercard = ran.nextInt(14);

while (dealercard < 1) { 
    dealercard = ran.nextInt(14);
}

A better way to randomize a number from 1 to 13 is

dealercard = ran.nextInt(13) + 1;

Static variables

You have way too many static variables for my taste, this will cause big problems when you have two separate instances of an object and they share the same static variables. It is good practice to avoid using static variables whenever you can (and trust me, it is more often than you think).

private final

All class variables ('fields') possible should be marked with private final, such as:

private final Random ran = new Random ();
private final Scanner scan = new Scanner (System.in);

However, I would not recommend creating a Scanner inside the Person class, it would be better to create it in your main() method and pass it along to the Person class.

Scanner scan = new Scanner (System.in);
Person p = new Person (scan);

Sleep no sleep

Thread.sleep(0);

This is utterly and completely useless in your main() method.

\$\endgroup\$
4
\$\begingroup\$

In the end of the Player class, where you check if the player wants to hit or stay, there should be an else-if-statement instead of the else case being to stay. This is not necessary, but in case someone spell wrong it should ask them again untill "hit" or "stay" is entered correctly

\$\endgroup\$
4
\$\begingroup\$

Some quick shots

  • ArrayList<String> name = new ArrayList<String>()
    3 things to notice

    • the field should be named names as a list can contain more than one name
    • you should use the diamond operator (needs Java 7)
    • You shouldn't code against an implementation but against an interface so: List<String> names = new ArrayList<>();
  • DRY (don't repeat yourself)

    • instead of shufflePerson() and shuffleDealer which already are really strange names, you should name it shuffleCardsFor(String name). The name parameter should be used instead of adding a hardcoded player "Bert" to names and a more hardcoded player Dealer.
\$\endgroup\$
0

Your Answer

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

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