4
\$\begingroup\$

I am looking to see which aspects of this code could use improvement.

In both design, and basic implementation and coding.

I'm using IntelliJ and Java SE 20.

import java.util.ArrayList;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import java.util.Scanner;

public class BlackJack {
    public static void main(String[] args) {
        BlackJack blackJack = new BlackJack();
        blackJack.round();
    }

    List<Character> cards = new ArrayList<>();
    Iterator<Character> iterator;

    BlackJack() {
        init();
    }

    void init() {
        for (int count = 0; count < 8; count++)
            cards.addAll(List.of('A', '2', '3', '4', '5', '6', '7', '8', '9', 'T', 'J', 'Q', 'K'));
        Collections.shuffle(cards);
    }

    void round() {
        Scanner scanner = new Scanner(System.in);
        iterator = cards.iterator();
        int score = 0, dealerScore = 0;
        int card;
        String string;
        boolean exit = false, exitAce;
        do {
            System.out.print("hit / stand:  ");
            string = scanner.nextLine().toLowerCase();
            switch (string) {
                case "hit" -> {
                    if ((card = draw()) != 1) {
                        score += card;
                        System.out.printf("hit +%d, score %d%n", card, score);
                    } else {
                        do {
                            System.out.print("ace, 1 or 11:  ");
                            string = scanner.nextLine();
                            exitAce = true;
                            if (!string.matches("11?")) {
                                System.out.println("invalid input, try again");
                                exitAce = false;
                            } else {
                                if (string.equals("1")) score += 1;
                                else if (string.equals("11")) score += 11;
                                System.out.printf("hit +%s, score %d%n", string, score);
                            }
                        } while (!exitAce);
                    }
                }
                case "stand" -> { }
                case "" -> exit = true;
                default -> System.out.println("invalid input, try again");
            }
            if (!exit) {
                if (score == 21) {
                    System.out.println("21, you win");
                    exit = true;
                } else if (score > 21) {
                    System.out.println("you've bust, dealer wins");
                    exit = true;
                } else {
                    dealerScore += card = draw();
                    System.out.printf("dealer hit +%d, score %d%n", card, dealerScore);
                    if (dealerScore == 21) {
                        System.out.println("dealer 21, you loose");
                    } else if (dealerScore > 21) {
                        System.out.println("dealer busts, you win");
                        exit = true;
                    }
                }
            }
        } while (!exit);
    }

    int draw() {
        char card;
        return switch (card = iterator.next()) {
            case 'A' -> 1;
            case 'K', 'Q', 'J', 'T' -> 10;
            default -> Character.digit(card, 10);
        };
    }
}

Here are a few input and outputs.

hit / stand:  hit
hit +4, score 4
dealer hit +4, score 4
hit / stand:  hit
hit +10, score 14
dealer hit +4, score 8
hit / stand:  stand
dealer hit +10, score 18
hit / stand:  hit
hit +6, score 20
dealer hit +4, score 22
dealer busts, you win
hit / stand:  hit
hit +6, score 6
dealer hit +10, score 10
hit / stand:  hit
hit +10, score 16
dealer hit +4, score 14
hit / stand:  hit
hit +6, score 22
you've bust, dealer wins
hit / stand:  hit
hit +10, score 10
dealer hit +10, score 10
hit / stand:  hit
ace, 1 or 11:  11
hit +11, score 21
21, you win
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

you should apply IOSP ans SLAP

instead of having one big method, where you do calculation and flow control split this big method (void round()) into smaller sub-method (IOSP)

that makes your code more easy to read (SLAP) - because you can see on top level whats going on

do {
    handlePlayer();
    handleDealer();
}while(!exit);

IOSP can be applied on many levels of your code

think in objects, abjure primitive obsession

it is not so hard to create a class for your cards, or at least an enum for cards. that would clean up the code on some levels. Especially the values of the cards would then have a proper place to live.

and these statement would be cleared up (card = draw()) != 1) --> (card = draw()).isAce())

tripping hazard (lack of cohesion)

why is the initialition of your iterator somewhere in the code? it should be inside the init method - it took me some time to find it.

void init() {
    for (int count = 0; count < 8; count++){ //java code style requires ALWAYS bracket on for & if
        cards.addAll(List.of('A', '2', '3', '4', '5', '6', '7', '8', '9', 'T', 'J', 'Q', 'K'));
    }
    Collections.shuffle(cards);
    iterator = cards.iterator(); //misssed it on this place!
}

minor issue: Nespresso - What else

avoid else statementens. they bloat up your code and increase complexity in an unnecessary way.

if (score == 21) {
    System.out.println("21, you win");
    break; 
} //no else here

if (score > 21) {
    System.out.println("you've bust, dealer wins");
    break;
} //no else here, too 

this issue is not so important and can be skipped.

\$\endgroup\$
4
  • 2
    \$\begingroup\$ avoid else statement[s] is not useful advice either generally or in this specific case. The second case can only be true if the first case is false, so an else (1) increases natural legibility and (2), if interpreted literally by a compiler, offers an optimization path in the form of short circuiting. \$\endgroup\$
    – Reinderien
    Jun 14 at 14:33
  • \$\begingroup\$ Thanks for the review. The reason I'm not using the break keyword is because I wanted to avoid early-returns. If I ever do utilize an early-return it's typically when I'm re-assigning a local value, thus subsequent code should be skipped. It's specific to design, I guess. As for the last recommendation, in terms of the brackets, that is not true, my code runs fine without the brackets there. And, for the iterator, I was going for a lazy-instantiation. In the event that round was called more than once. Now that I look at it, I guess I should also re-shuffle the cards. \$\endgroup\$
    – Reilas
    Jun 14 at 19:59
  • 1
    \$\begingroup\$ honestly the improved if/else findings are low prio - if the OP could apply IOSP / SLAP & get rid of Primitive Obsession then that would be sufficient and alreadyx improve the code greatly @Reilas \$\endgroup\$ Jun 15 at 7:25
  • 1
    \$\begingroup\$ i will adjust my answer according to your both feedback @Reinderien \$\endgroup\$ Jun 15 at 7:25

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.