acknapp/BlackJack — Issue #2: Java Code Review from Merehap
Author: acknapp
Created: 2012-02-27 01:03:07+00:00
State: open

Run Java Lint in all of your programs: http://www.jutils.com/ . It
will give you a lot of helpful code suggestions. There is a lint for
every useful programming language in existence. I use lints on every
project I work on.

Use enumerations for suits and values instead of String[] :
http://docs.oracle.com/javase/tutorial/java/javaOO/enum.html
This way misspellings and case issues won't ever be a problem as you
won't be comparing strings (or parts thereof) anymore. In your
determineCardValue function, for example, rather than "name.charAt(0)
== 'K'" you will have "name == Cards.King" which is much more safe
(name can't be null, no misspelling or case problem, etc.) and I think
you'll agree more pleasant to look at.

Fix the formatting of the playAgain function; it is quite difficult to
mentally parse.

Separate the logic of whether a player has won or not from the logic
of what you do if they've won. Specifically, from winCheck, pull out
all IO code (println) and probably the gains_losses logic. winCheck
can instead return an enumeration of possible outcomes. This
enumeration would have the following code:

public enum RoundOutcome {
BLACKJACK, PLAYER_BUST, CONTINUE, DEALER_BUST, DEALER_WIN, PLAYER_WIN
}

All the IO and gains_losses code that is currently in winCheck will
come after the call to winCheck and depend on winCheck's return code
(enumeration), rather than be in winCheck itself. While winCheck may
not look bad at this point, when your blackjack program becomes ever
more complicated, tying together those three concerns (round outcome,
winnings, and console feedback) will become more and more tangled and
frustrating. In the end you'll need to develop that intuition yourself
as it isn't fully communicable human-to-human. I'll give you one
concrete example though. In the future, you may want to give your game
a GUI in addition to it's text CLI. With the current code, you would
have to change the winCheck function, with my suggestions you would
not. The change would come in the form of

if(isGUI){
textBox.setText("You got BlackJack! You win!");
} else {
System.out.println("You got BlackJack! You win!");
}

for all instances of IO in the code. A few changes like that, and the
code quickly becomes incomprehensible.

Also, pull out IO from all of the other functions that also do logic.
These functions are betAmount, Hit, and playAgain. Not intro as it
only has IO, no logic.

You can write "-to_play" instead of "-1 * to_play". It's probably
just personal preference though.

Basically, one of the biggest things you want to accomplish is
creating regions of code that you will never have to touch again.
Without separating IO from logic, your entire problem falls under the
category of code-I-will-probably-have-to-modify-again. The ideal to
shoot for is probably 80% to 90% of your code locked down like this.
100% isn't usually achievable because for most programs you'll always
to add more features, and to add new features you have to modify code
somewhere. Some decades old unix utilities are hovering around 99%,
with the only reasonable changes being modifications to the usage
message.

So, at every working iteration of all of your projects, ask yourself,
"What proportion of my functions are 'locked down'?" Answering that
question for your current code would leave us with perhaps 3 out of
all 10 substantial functions that are relatively close to being locked
down: drawCard, buildDeck, and shuffleDeck. These are, for example,
functions that wouldn't have to change just because you move your
project from a CLI to GUI. How can you change the rest of your
functions to be like that?

You'll want to break your main function down into more helper
functions than you currently have. A main function should commonly
look close to pseudo-code (well-structured English).

For Blackjack, that would look similar to this:

GameState gameState = getInitialGameState();
boolean shouldPlay = true;

while(shouldPlay){
printRoundStartInfo();
boolean shouldContinueRound = true;
while(shouldContinueRound){
gameState = playerTurn(gameState);
printPlayerUpdate(gameState);
gameState = dealerTurn(gameState);
printDealerUpdate(gameState);
shouldContinueRound = continueRound(gameState);
}

shouldPlay = shouldPlayAgain(gameState);
}

Note that this code, despite being valid Java, is almost as clean as
pseudo-code. Really the only divergence is passing gameState around
because that would be assumed (poorly) to be global state in most
pseudo-code.

Don't compare booleans to true: "play == true" is bad. I mentioned
this last time too. "play == true" should instead just be
"shouldPlay". That way when put in an "if" statement, it reads like
English: "if should play". If you are going to compare a boolean to
true, why not do it multiple times? "play == true" is just as silly as
"play == true == true == true". Unnecessary and confusing verbosity.

By the way, on second thought, your betAmount function looks fine.
There is no way in Java to untangle IO from logic in that case.