acknapp / blackjack Goto Github PK

Your code:

if(ans == 'y'){

return true;

} else {

return false;

}

Better:

if(ans =='y'){

return true;

}

return false;

or

return ans == 'y' ? true : false;

Best:

return ans == 'y';

or

bool passed = ans =='y';
return passed;

Return statements terminate a function so there is no need for an else statement when you have two return statements. Also, you want to avoid code nesting when possible, as "flat" code is easier to read.

Branching of code ("if" statements or loops) should be avoided when not necessary for code cleanliness as this reduces bugs and improves readability.
There is an older school of thought in which functions should only ever have one return statement. While most people don't believe you should follow this exactly, when you can without much extra effort (in fact less effort, in this case), you should. This way you won't have to guess which exit point was used when you are debugging code.

In general try to use booleans directly, rather than putting a lot of extra syntax around them. This is similar to how many novice programmers will write "if(foo == true)" rather than the simpler (and closer to natural language) "if(foo)". If you are going to use the first way, why not "if((foo == true) == true)" or "if(((foo == true) == true) == true)"? If you want to use the first case, with it's redundancy, you can hardly reject the latter cases, with their increasingly absurd levels of redundancy. The point is use booleans directly in all cases, or else you are making your code more arbitrary and complex with no benefit.

In your main function you wrote:

//Initial deal.
...

//Player's decision.
...
//Dealer's play
...
//Check who won
...

//Sanitize structures
Comments are nice and all, but their main uses are documenting a file's (or class's) purpose, a function's purpose, or not immediately obvious logic in one's code. These comments sort of fall in the third category, but this code would be much better off with new functions with similar names instead.

For example:

while(shouldPlay){
int roundBet = makeBet(money);

state = getInitialDeal();
updateWithPlayerDecision(state); //**or** state = getPlayerDecision(state);

updateWithDealerDecision(state); //**or** state = getDealerDecision(state);
updateWinnings(state); //**or** state = getWinnings(state);

shouldPlay = askIfShouldPlayAgain();

}

With refactoring and good function names, we've eliminated the need for clarifying comments. All of our functions called within the main function have high level, descriptive names. You can think of the way the main is in this example as a more precise, better form of English.

int initialMoney = 100;
int money = initialMoney;

...
if(money >= initialMoney){
cout << "You made: " << money - initialMoney << endl;

} else {
cout << "You lost: " << initialMoney - money << endl;

}
Here we made initialMoney a constant so we don't have to repeat it in multiple places. This means that if we decide to make initialMoney a different value, it updates in every place at the same time. It is pretty much inevitable that a mistake will be made updating the old code, such that you have inconsistent values in different places.

Even better:
String message = money >= initialMoney ? "You made: " : "You lost: "){

cout << message << Math.abs(money - initialMoney) << endl;

We've removed the branching and removed redundant calls to I/O and subtraction.

Just a few things to get you started.

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.