Title: Student Code Critique

To start with, it is best to have the rules of how to find the value of a blackjack hand spelt out in English. In other words we need a specification. We can then decide whether the program is doing what is should. After a bit of hunting I find that the rules for adding the cards of a blackjack hand are as follows.

The score of a hand is the sum of the values of the cards. An ace counts as either 11 or one, whichever results in a better score for the hand. Any face card counts as ten, and any other card counts as its face value. Any hand with a total greater than 21 "busts" and cannot win.

Well let's have a quick scan through the program to see if we can understand how it works and to pick out any immediate problems. After a welcome message the program enters a loop that will end only when the user doesn't want to have another go. So far, so good. The first line of the loop is a call to a function named number. The function takes a single parameter of type int as can be gleaned from the prototype. The int value is passed by a variable by the name of num_cards. This is where things aren't quite right. The bad news is that num_cards has not been initialized to any particular value. Its value could be anything (within the range of an int). The good news is that when this unknown value is passed to the function number, the function does not take any notice of it anyhow. Of course this is not really good news because it shows that the student has got somewhat muddled in defining the function, but at least it will not prevent the program from working.

What the student should have done is to declare the function number with no parameters and to have defined num_cards as a local variable within the number function. With this out of the way we will press on.

The function number returns with the number of cards as entered by the user. This number must be within the range two to five inclusive. If the number entered by the user is not within this range the function will insist that the user enters another number.

The main function now repeatedly calls card_val, once for each card in the hand. The sequence number of each card is passed to the function by means of an int parameter. The card_val function invites the user to enter a card. It then calculates and returns the value of the card. Things get a little confusing here because the student has declared a variable within the card_val function with the same name as the function, namely card_val. Another name, such as value_of_card would have been a better choice.

While we are on the subject of the naming of variables, there are one or two things that should be born in mind. The name of the variable should reflect the use put to the variable. The name should be clear and easy to read. Non-standard abbreviations should be avoided. There is no reason to call local variables card_val, for example, when the name card_value does not take a great deal more typing and is explicit. I actually agree with the general way the student constructs variable and function names, i.e. a short phrase, all in lower case and separated with underscores. In every day life most text is written in lower case and separated with spaces and this, presumably, is what most people are used to.

There is a slight complication if the user chooses an ace for a card. The number of aces entered has to be counted for use later on. This is where the student comes unstuck.

The student has declared a variable within the card_val function with the name of aces. Now, there is already a variable with the name aces declared in the main function. This is bound to cause confusion. The aces variable that was declared in the card_val function is created and initialised to zero each time the function is called. It is a reference to this variable (that has the value zero) that is passed to ace_count (or as the student incorrectly types, ace_card). The aces variable (the one declared in card_val) is incremented by one by the ace_count function. Unfortunately, when the card_val function finally returns, the aces variable no longer exists. What should be used is the aces variable that was declared in the main function. There is no need to declare the aces variable in card_val at all.

The only problem now is that aces declared in main is not directly accessible from within card_val. One way to get out of that problem is to add another parameter to card_val that passes the main aces variable by reference. The call to card_val would have to be modified as well, of course.

Having summed the value of all the cards within the hand, the main function now attempts to display the value of the hand. So far the program has assumed that all aces are worth 11 points each. This is the correct thing to do as long as sum does not exceed a value of 21. If the hand does exceed 21, then, for each ace in the hand 10 points can be subtracted until the value of the hand becomes 21 or less. This is where the hand_value function comes in. I think the student was getting a bit fed up at this stage as there are a couple of simple errors in as many lines here. The definition of the function is called new_value whereas the prototype and the call refer to a function called hand_value. Presumably, the definition should be written as hand_value. The hand_value function contains a while loop that refers to ace_counts, I think what the student really means is aces.

Now that all these little problems have been put right, does the program work? Well, not quite. The number of aces is not set to zero when the user wishes to choose another hand. The student has remembered to zero the total value of the hand, but not the number of aces. After rectifying this omission, we now have a working program (I think).

There are, however, one or two things that could be done to improve the general structure of the program. There is a rule of thumb that states that variables should be declared as late as possible, i.e., as near to the point of use as possible. This helps when maintaining the program; you don't have to look so far to find the declaration of variables. It also keeps the scope of variables limited to where they are used. There is no point in spreading their influence too far a field. num_cards, total and aces and be declared and initialised within the do loop of main(), for example.

Code layout is always a matter of personal taste, but I would like to see a space either side of an operator, such as value = 10. I also prefer opening and closing braces to be vertically aligned. I simply do not understand the idea of having the opening brace and the end of controlling line. It makes working out what code constitutes a block much more difficult. I know many experts use this asymmetric approach, so if anyone can explain to me how it makes the code more readable I would like to hear about it. (I can see that the asymmetric approach is of some use when printing code as it uses one less line per pair of braces on the written page. Exactly, so you should not judge layout on the basis of what is printed. Layout should be a feature of function not religion. Francis)

The code that deals with displaying the total hand could be tidied up a bit. If new_hand was called regardless of the calculated total, then the system of nested if statements could be simplified and now becomes as shown below.

total = hand_value(total, aces);
if (total <= 21)
  cout << "\nYour hand is: " << total << ".";
else
  cout << "\nYou busted with " << total;

It does not matter if hand_value is called with total less than or equal to 21, as it will return immediately with the same value as total.

It is useful to know that, within a switch statement, a case can have more than one label. In this way, duplicated code can be avoided, as shown below.

case 't': case 'j': case 'q': case 'k':
  value = 10; break;

Here is my version.

#include<cctype>
#include<iostream>
using namespace std;
int card_val(int card_no, int & aces);
int number();
int ace_count(int & aces);
int hand_value(int total, int aces);
int main(){
  cout << "\nWelcome to Blackjack Counter.\n";
  char again;
  do {
    int num_cards = number();
    int total = 0;
    int aces = 0;
    for (int card_no = 1; 
        card_no <= num_cards; card_no++)
      total += card_val(card_no, aces);
    total = hand_value(total, aces);
    if (total <= 21)
      cout<< "Your hand is: " << total;
    else
      cout<< "You busted with " << total;
    cout << endl;
    cout << "Do you wish to continue?\n";
    cout << "Enter 'Y' (Yes) or 'N' (No): ";
    cin >> again;
  } while ((again == 'Y') || (again == 'y'));
  return 0;
}
int number(){
  int num_cards;
  do {
    cout << "How many cards do you have? ";
    cin >> num_cards;
    if ((num_cards <= 1) || (num_cards >= 6))
      cout << "You must have between 2 and 5
                     cards (inclusive).";
  } while((num_cards <= 1)||(num_cards >= 6));
  return num_cards;
}
int card_val(int card_no, int & aces){
  bool invalid_card;
  int value;
  do {
    invalid_card = false;
    cout << "Enter value of card number " 
        << card_no << ": ";
    char card_val;
    cin >> card_val;
    switch (tolower(card_val)){
      case 'a':
        value = ace_count(aces); break;
case '2':
<similar lines snipped>
      case '9': value = 9; break;
      case 't': case 'j': case 'q': case 'k':
        value = 10; break;
      default:
      cout << "Not a valid entry. 
              Please try again. \n";
      invalid_card = true;
    }
  } while (invalid_card);
  return value;
}
int ace_count(int & aces){
  aces++;
  return 11;
}
int hand_value(int total, int aces){
  while ((total > 21) && (aces >= 1)){
    aces--;
    total -= 10;
  }
  return total;
}

There are several problems with the code as published - some of which are so odd it looks as if the code has been deliberately mangled :-)

The function ace_count() is declared and defined, but never referenced. An undeclared and undefined function ace_card() is called from within card_val. I suspect, given the function signatures and results that the call to ace_card() should be replaced with a call to ace_count().

The function hand_value is declared, called by main, but never defined. Again given the signatures and function I suspect that the undeclared function new_value should be renamed hand_value. A more informative name would indicate that it really recalculates the value of the hand. The new_value() function has a problem with the logic in that it refers to ace_count as a variable and not the aces value actually passed as the second argument.

The function number(), takes an unnecessary argument. While it does mean that a temporary value need not be used in number(), it is misleading. The function itself should retrieve the number of cards held by the user, but has insufficient error handling and duplicates the test for valid input. This would have been better coded as per The Harpist's code critique in C Vu 13.6 p14 or equivalently using code based on getInput() from my code in the same article.

Both main() and card_val() could handle requests for character input in a cleaner and safer fashion than they do. Again I refer to the code referenced above for how this might be better done.

The problem with counting aces is rooted in the card_val() function. The count of the number of aces held is a local variable in this function, and while it is updated by the ace_count() function, it is destroyed when card_val() returns. As card_val() is called once per card by main() the value of the variable aces in main() will always be zero. Given the current code structure, it is difficult to find an elegant solution to the problem, short of using a global variable (yeuch!).

In card_val() the large number of case statements can be simplified to three essential cases - numbers, tens and court cards and aces. I note in passing that there is a missing semicolon after the break statement associated with case '9'.

I think that covers the majority of errors in the code. With regard to structure I feel that the intertwining of user input and calculation, and indeed user input from multiple routines is messy. The solution I have proposed probably betrays my prejudice against handling user input. In almost all the code I write, it either never interacts directly with a person or obtains all the information required "up front" and then proceeds to completion.

// Replacement code to work out the value of a 
// blackjack hand. 
// Catriona O'Connell 23.11.2001
#include <iostream>
using namespace std;
enum ifailValues {Success = 0,
  Error_Too_Few_Cards,  Error_Too_Many_Cards,
  Error_Invalid_Card,  Error_Busted};
char *ifailMessages[] = {"Success", 
  "Too Few Cards", "Too Many Cards", 
  "Invalid Card(s)", "Busted"};
bool CheckValidCharacters(
    const char * testString, 
    const char * validChars);
int GetBlackjackHandValue(
    const char *cards, int *ifail);
bool CheckValidCharacters(
    const char * testString, 
    const char * validChars){
// Check if the first argument string contains // only characters in the second argument 
// string. Returns true if it is the case,
// otherwise returns false.
  size_t testStringLen;
  testStringLen = strlen(testString);
  if(strspn(testString, validChars) 
          != testStringLen) return false;
  return true;
}
int GetBlackjackHandValue(
    const char *cards, int *ifail) {
// Returns the value of a blackjack hand. The 
// value returned is the sum of the value of 
// the cards held, if all the cards are valid 
// and the number of cards is within the 
// presecribed limits. In the event of the 
// hand being invalid, ifail is set to one of 
// the enumerated ifailValue values.
  int cardsLen;
  int handValue = 0;
  int aceCount = 0;
  *ifail = Success;
  cardsLen = strlen(cards);
// Handle simple error cases by setting the 
// ifail status value and returning a hand 
// value of zero.
  if(cardsLen < 2){
    *ifail = Error_Too_Few_Cards;
    return 0;
  }
  if(cardsLen > 5) {
    *ifail = Error_Too_Many_Cards;
    return 0;
  }
// Check that all the cards held are valid.
  bool validHand = CheckValidCharacters(cards,
                   "23456789tTjJqQkKaA");
  if(!validHand) {
    *ifail = Error_Invalid_Card;
    return 0;
  }
// Now that we've checked the pre-conditions,
// calculate the value of the cards held. For 
// numeric cards, we use the fact that the 
// characters representing the digits are 
// required to be contiguous and in the 
// expected collation order. For ten and 
// picture cards we add 10. For aces, we add 
// 11, but retain a count of the number of 
// aces, in case we need to use the 
// alternative lower value of 1, to attempt to 
// reduce a busted hand to an acceptable one.
  while(*cards)  {
    switch(*cards){
    case '1': case '2': case '3': case '4':
    case '5': case '6': case '7': case '8':
    case '9':
      handValue += (*cards-'0'); break;
    case 't': case 'T': case 'j': case 'J': 
case 'q': case 'Q': case 'k': case 'K':
      handValue += 10; break;
    case 'a': case 'A':
      handValue += 11; aceCount++; break;
    }
    cards++;
  }
// If the hand is busted and there are any 
// aces, then substitute the value 1 for 11, // while the value of the hand is over 21 and 
// there are still unconverted aces remaining.
  while((handValue > 21) && (aceCount>=1)) {
    aceCount--;
    handValue -= 10;
  }
// If despite the above, the value of the hand 
// is over 21, return the value but set the 
// ifail status value to reflect that.
  if(handValue > 21) *ifail = Error_Busted;
  return handValue;
}
int main() {
  char *testString[] = {"8", "ak2233",
               "aaaa2","kkaa4","2234f"};
  int ifail;
  int result;
  for(int i=0; i<5;++i) {
    result = GetBlackjackHandValue(testString[i], &ifail);
    cout << testString[i] << " " << result << " " << ifailMessages[ifail] << endl;
  }
  return 0;
}

At first, as in the past I want to apologize for my English, please be patient.

{At the author's request I have cleaned it up for him, but I can assure you that it was much better than my Hebrew <g> Francis}

At last I got the courage and time (new baby boy :) to do a critique. I am neither an authority nor a C++ guru, like the amazing people who write here, just a student, so writing this critique is based on my own personal experience.

After reading so many books, columns and articles about C++ it was very hard to start this critique. I have in my mind all, what you should do and what you shouldn't do, and writing for such audience like you its very, well, brrrr.

But then I thought (hell) why not, let me try it (and I would even appreciate a critique on my critique), let see if I can contribute a little.

Reading the code and trying to "fix" it, I've decided on some key guidelines that I will follow:

First of all the function prototype hand_value should be new_value.

In the main function the use of card_no for the for-loop would be better if it was defined in the for-statement and card_num would be better though I would have preferred simply i for a counter.

I think I would have preferred to put the prompt to continue in a separate function that returned a bool then you could have something like:

...
} while (yn_continue() );
...

that would tidy up the main function a bit.

Now onto the other functions. First the number function. having a argument num_card is not needed as it returns the value entered; it should be declared in the body of the function. The if-statement could have been better put as

if(num_card < 2 || num_card > 5 )

Though the original would work fine. there is no check to see if the user entered a digit or other invalid character or any flushing of '\n' characters which could course problems. And finally the name of the function could be better i.e. number_of_cards();

Now to card_val and the ace_count functions. As Francis pointed out, there's a problem with aces. Basically it needs to have an int pointer in the parameter list to access the aces variable and not define the variable again in the body of the function. In the switch-statement we have some cases that receive the same value so these could be allow to drop through :

//....
case 't':
case 'j':
case 'q':
case 'k': value = 10; break;
//....

Also you do not need the flag invalid_card as setting value to zero would do:

while( ! value );

The acs_count function has a reference to aces but I think in this case a pointer would be better though I wonder if we needed that to be put in a separate function.

Finally after thinking about it I decided that the output was a bit inadequate and was in need of improvement. This, for me, means displaying, in English, the cards entered and then the total of my hand. For the program this means storing what the user enters in an array. In my example below I use a map to keep and retrieve the words as needed. This map could than be used to validate the input and this also allowed me to get rid of the switch-statement to find the value of the card. I have not put in a check to make sure that you cannot enter five of a kind; It seems a bit unnecessary and with displaying the cards with the result a user should be able to see that mistake.

So here's my version, as you can see some functions have changed, gone and new ones added.

// blackjack.C
#include <iostream>
#include <cctype>
#include <map>
#include <iterator>
#include <string>
using namespace std;
#define MAX_VAL 21
#define MIN_CARDS 2
#define MAX_CARDS 5
int number_of_cards();
int value_of_hand(int, int *, char *,
               map<char, string> &);
int new_val(int, char *, int);
int card_value(map<char, string> &, char, 
                        int *);
bool yn_continue();
void display_hand(map<char, string>
      &card_kind, const char *hand, 
      int num_cards);
int main(){
  char *str[] = {"ace-low", "one", "two",
     "three", "four", "five", "six", "seven",
     "eight", "nine", "ten", "Jack", "Queen",
     "King", "ace-high" };
  char * ch = "A123456789tjqka";
  map<char, string> card_kind;
// build map to keep of each type
  for(char **s = str, *c = ch; *c; c++, s++ )
    card_kind.insert(map<char,
       string>::value_type(*c , string(*s)));
  cout << "Welcome to the Blackjack Counter."
      << endl;
  do {
    int total = 0, aces = 0;
    int num_cards = number_of_cards();
// storage for cards in hand
    char hand[MAX_CARDS];
    for(int i = 0; i < num_cards; i++ )
      total += value_of_hand(i + 1,
              &aces, hand + i, card_kind);
// if bust with aces in try making them ace-low
    if(total > MAX_VAL && aces > 0) 
      total=new_val(total, hand, num_cards);
    display_hand(card_kind, hand, num_cards);
    if( total <= MAX_VAL )
      cout << "Your hand is " << total 
          << "." << endl;
    else
      cout << "You Bust with " << total 
          << endl;
  } while ( yn_continue() );
  return 0;
}
int number_of_cards() {
  int num_cards;
  do {
    cout << "How many cards do you have? ";
    cin >> num_cards;
    if(!cin) {
      cout << "Invalid input: "
          << "Numeric input only!" << endl;
    cin.clear();
    while(cin.get() != '\n');
    }
    if(num_cards < MIN_CARDS || 
                  num_cards > MAX_CARDS )
      cout << "You must have between 2 and 5"
          << "cards (inclusive)." << endl;
  }while (num_cards < MIN_CARDS || 
                  num_cards > MAX_CARDS);
  return num_cards;
}
int value_of_hand(int card_num, 
          int *ace_count, char *hand, 
          map<char, string> &card_kind) {
  char card;
  int value = 0;
  do {
    cout << "Enter value of card number " 
        << card_num << ": " << endl;
    cin >> card;
    while (cin.get() != '\n');
    card = tolower(card);
    value = card_value(card_kind, 
                    card, ace_count);
  }while(!value);
  *hand = card;
  return value;
}
int card_value(map<char, string> &card_kind,
             char card, int *ace_count){
  map<char, string>::iterator it 
                = card_kind.find(card);
  if(it != card_kind.end() ) {
  if(card == 'a' ) {
    (*ace_count)++;
    return 11;
  }
  else if(card == 't' || card == 'j' ||
             card == 'q' || card == 'k' )
return 10;
  else
    return card - 48;
  }
  return 0;
}
int new_val(int total, char *hand, int num) {
  for(int i = 0; i<num && total>MAX_VAL; i++) {
    if(hand[i] == 'a' ) {
      hand[i] = 'A';
      total-= 10;
    }
  }
  return total;
}
bool yn_continue() {
  cout<<"Do you whish to enter another hand? ";
  char c = cin.get();
  while (cin.get() != '\n');
  return ( c == 'y' || c == 'Y' );
}
void display_hand(map<char, string> &card_kind, const char *hand, int num_cards){
  cout << "In your hand you have cards:" 
      << endl;
  for(int i = 0; i < num_cards; i++ ){
  map<char, string>::iterator it
            = card_kind.find(hand[i]);
    if(it != card_kind.end())
      cout << (*it).second << endl;
  }
}

I always feel guilty about not entering. Here is my approach to the problem. I hope it is seen as an attempt to make you think. The lecturing style should be considered as a literary device. That is assuming I am entering the (Student Code) Critique and not the Student (Code Critique) competition, although both are equally valid. It is actually the eleventh hour. For some reason, my magazine had been lying undetected and I read the code critique competition the day before the due date. Today is that date, and I write hurriedly before I turn into a pumpkin.

Whilst driving between A and B this morning, I was trying to crystallise the essence of what I might write. Note that I was neither reading on the motorway nor have I a photographic memory. My approach is going to be to get across to the student things that from my experience will benefit their rite of passage. I come from a background of little formal training in either C++ or the standard libraries. However, I have had the benefit of working for more than one modelling company and have been in positions of, albeit minor, influence within them. So, I'm not going to pick holes in things a student should know, but from the standpoint of a senior technical person for whom the student had produced the code (and who must deliver the code to another party).

Let's suppose my brief was, "Write me a program to workout the value of a blackjack hand, and demonstrate that it works". It would appear to me that the first thing this student did was to switch on a computer and create main.cpp (or whatever the listing is). Right we start with main()...

What are you doing? Switch it off. My god, don't you want to think about it? Take a drive for a few hours. Whatever. I would expect that the brief should first need to be clarified. What do you mean by a "program"? What use is it to be put to? What kind of test-harness would you like? My answer: I want a "class" I can include in my program to add up the values of blackjack hands. Maybe it's in a DOS window, maybe I use it in an activeX control. Furthermore, I would like a test harness "class", which is also reusable, so that I (or rather you) can test your evaluation class. So, we call these Blackjack Evaluator and Blackjack Test Harness. Your deliverable program will also have some controlling class, let's call it main. I don't know, but I may have already raised eyebrows. Have I insulted the student? I know you want to prove that you can write an algorithm, but that's not the deliverable. We'll get there in a minute. Given that we'd like to separate interface and implementation, we now have five files for the student - main.c, Blackjack Evaluator.h & c, Blackjack Test Harness.h & c. Note the judicious use of capital letters. Now, on our class diagram, we have drawn a relationship from the controller to the test harness and from the test harness to the evaluator. These are uni-directional, so we have one file including the other. In main.c, we have #include "Blackjack Test Harness.h" and in the test harness implementation file, we have #include "Blackjack Evaluator.h". Note that there is no backward inclusion going on i.e. that the user of the evaluator does not need the test harness.

So, what next? Well, the blackjack evaluator needs something to evaluate. So, we have a function "Evaluate Hand". This would seem to be one of the things main is doing. What parameters should it take? Using noun-analysis, we conclude it must be a "Hand". Another class, representing a hand of cards, that is a collection of somethings. What? "Card Values". I'm not up on stl, so I don't know the most appropriate container class to use. It might be nice to have it ordered so that busting could be given in more detail. A list? A vector? You tell me.

I now see a potential problem, we can't just ask for a hand and then evaluate it, as it might be the case that a part of that hand was already over the total allowable, so our test harness needs to keep calling the evaluate as it grows the hand. This means that each card is potentially counted more than once. Well, if this program is so abominably slow that this is unacceptable, then my brief was wrong (or rather the constraints were not documented). It's interesting to note here that although my algorithm is non-linear, I know the limits of it (5 cards maximum). We couldn't do this for a hand of undefined length without cause for concern.

Having expounded, I realise I am not criticising the code in the traditional sense. Line 31, missing semi-colon. Tut, tut. I won't continue the above train of thought to the bitter end, perhaps I can now apply some more specific comments, but still along the lines of standing back from the issue of being a human compiler.

The first thing I object to is a function called number. Meaningless. GetNumberOfCards is my best offering after a few pints of homebrew. Why does it take an in-parameter? No reason that I can see apart from not wanting to declare (and initialise) a local variable.

Next function, card_val. The name is better. GetCardValue would be better, but perhaps you are feeling sheepish by now, so we'll be kind (but can I recommend that you take a touch typing course instead of playing computer games?). But, again, why the in-parameter? So you can tell the user which card they are entering. Fair enough given this structure, but in my ideal structure, we'd have one lump looking after getting the card values from the user and another working out the value of the hand. There are uninitialised variables here. It makes me shiver. Yes, they are set up in this case, well, all apart from value when the user enters something that invokes the default case of the switch statement. I can quite accept that there are people who could look at the code and say, "In this case it doesn't matter". But, I don't think you should have had to get someone to go that far. There is a problem in assigning a sensible initial value. I usually go for zero on numerical variables unless the logic dictates something specific.

ace_count. An admirable function, provided it counts the aces in a hand, which it doesn't. This function as implemented should be called IncrementNumberAndReturnEleven. Enough said, I think.

new_value. Yes, it does get a new value, but so would a random generator. What is this function doing? It's working out a refactored value based on the fact that instead of busting, you can count an ace as 1 instead of 11. A good name for this function escapes me. GetMaxNonBustTotal would be a first stab. Not that long really. I typed it in a second or two.

I've now run out of code. It would seem I have been reduced to the level I wanted to avoid. I don't see how it would compile and link:

A general point, Mr/Ms Student: Your code should compile and link, and do so with no warnings. You might "know about that one", but when you leave for higher echelons, your successor might not. I might not, you might not in a year's time.

I sincerely hope that by thinking about the problem in the way I have eluded to above, a student would largely escape the nasties here. Name your functions well, and only give them the parameters they need. My class modelling would have applied to C, C++, in fact any language for which you could devise a suitable mapping between logical classes and code units. I may have missed the things that Francis was getting at, I may have picked on things that weren't a problem, but then, that's my point - it's difficult to analyse a mess.

After looking at the code I could see some visible syntax problems. I'm sure the code would not compile as is. After repeatedly looking at the code these are the things I have problems with or would like to improve on.

Glancing at this code, my brain says "C". Looking more closely, I notice the use of cout, and the reference parameter in ace_count, so this must be C++. Whilst I have no objection to using C++ as a better C, I do not think that this code is "better C": all variables are declared at the top of functions, rather than where they are first needed and the switch statement could so easily be replaced by a better C++ construct, e.g. using std::map.

I don't much like the use of "using namespace std" at the beginning of every file, but since my compiler effectively does this anyway, I have not means of finding places where I use a std:: item unqualified, so I shall leave this here.

Let us first tackle the internal design issue: how do we represent the cards in such a way that the value of the hand can be calculated when there are aces present. The code presented fails on this because the aces count inside card_val is not propagated up to the main program for passing to new_value - which with this code is always going to see zero aces. We can represent the value of a card or set of cards by a pair of integers for the value and the ace-count:

class card_value {
  int value; int aces;

N.B. If you are feeling paranoid about space, we can note that the value will never exceed 60, so these two members could be char rather than int. Such a choice does not affect the rest of the implementation.

An item of this sort can be initialised in these three ways:

We can easily make this using one constructor with default arguments:

card_value(int val=0, int ace=0)
      : value(val), aces(ace){}

however this would leak the implementation of aces, so I prefer to add a helper routine for case (c) and use the constructor for normal cards only:

public:
  card_value(int val=0): value(val), aces(0){}
  static card_value ace();

The other constructors and the destructor can be left to the compiler because the default ones will be correct. I shall return to the implementation of the ace() function.

We need to add a card to the total, so we need an operator +=:

  card_value &operator+=(const card_value &c){
    value += c.value;
    aces += c.aces;
    return *this;
  }

And now the only thing remaining is to calculate the value of the cards accumulated, so we need an evaluation function:

  int hand_value() const;
};

"Cheat" I hear you say, "you haven't defined this function." Well, yes and no. I can defer the implementation of the function to be external to the class, but also I have not yet defined how the value/ace split is made in order to be able to write this function anyway.

So how am I going to define the card_value member variables? I think that it is clear that non-ace cards have their value as specified in the original "switch" statement, with aces being zero. Also aces for an ace needs to be one, so the problem is the value. I can see three equally valid values of "value" for an ace card:

I shall choose (b) simply because I want to. Now we can write the ace and hand_value functions:

card_value card_value::ace() {
  card_value new_ace(1);
  new_ace.aces = 1;
  return new_ace;
}
int card_value::hand_value() const {
  int total = value;
  int ace_count = aces;
  while (total < 11 && ace_count > 0) {
// Use up an ace counting as eleven
    total += 10;
    ace_count--;
  }
  return total;
}

Now we need to look at how the card_value class is going to be used. Firstly, there must be a declaration of the total value:

card_value total;

This picks up the constructor with two zero arguments, and so conveniently initialised to zero. There is a loop, which finds the card and adds it to the hand.

for (...) total += read_card();

Here read_card will return a card_value, and so the operator+= adds it to the hand using the operator we defined. read_card is my replacement to card_val. This function needs defining.

I already suggested that the switch-statement could be replaced by a map, so let us look at this possibility. The map has to take a character representing the card and return the card_value for that card. To save RSI, I like to define a type for the map, which also allows possible future transformation into a class type, if required:

typedef std::map<char, card_value> card_map;

We are going to need to initialise one of these maps with all the valid values, so let us define this function:

void init_card_map(card_map &cards) {

Most of the cards are simple values, and so we can initialise them with integers, since this uses the (val, ace=0) constructor:

  cards['2'] = 2;
<similar lines snipped>
  cards['t'] = 10;
<similar lines, both upper and lower case snipped>
  cards['K'] = 10;
</pr

Notes: 
More fields may be available via dynamicdata ..