acknapp/BlackJack — Issue #1: Code review from Merehap
Author: acknapp
Created: 2012-02-26 17:38:23+00:00
State: open

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.