Title: Student Code Critique Competition

I always try to fit this code in, but doing so this time has unpleasant consequences like what else should I drop to make the page count. So this time you will either have to dig out your last issue or visit the ACCU web site.

Hello, I am a relatively new (6 months now…) C++ programmer and I thought I'd have a go at the critique competition. I am not especially confident so this should be a good learning process for me; to that end feel free to replace 'I like to…' with 'I think it's good practice but I'm worried that I'm wrong…' in my comments. ;-)

I appreciate that limitations dictate what you can print so I will refrain from 'comments' comments etc. except to say that the code really belongs in five files: Hand.h, Hand.cpp, Card.h, Card.cpp and Main.cpp.

My first comment on the code is the one made by Catriona O'Connell in the previous Critique: it exhibits "the intertwining of user input and calculations".

In class Hand there are four public member functions. Two relate to input and output (input_hand(), show_total()) and two to the calculation (add_card(), calculate_total()). This distinction is clarified by the observation that the latter two are only ever called by the former two, and not by any client of Hand.

I suggest removing input_hand() and show_total() from Hand and making them standalone functions, declared in a separate header file (named handIO.h say). They would need additional parameters, Hand& and Hand const& respectively. I would remove the existing stream parameters as they are promising more than they are delivering. The implementation of the functions is really only appropriate for cout and cin.

I think there is scope for improving the function names. add_card() could become simply add() since I think hand.add(aCard) reads better than hand.add_card(aCard). Likewise input_hand() could be replaced by input(). I would change calculate_total() to total() since the function is returning the value of an attribute of the class - whether we perform the calculation of that value in that particular function is an implementation detail. This detail is of no interest to clients, so it ought to be concealed from them. [I also think value() would be a more exact name than total(), but that is a rather more subjective opinion.]

Hand encapsulates a vector of Cards, which sounds good. However when we inspect class Card more closely we discover that it only encapsulates a card value without encapsulating the card's name. This is an important distinction in this application because Hand::calculate_total() sometimes needs to know whether there is an ace in the hand. It does this in the submitted code by making an assumption about the value assigned to an ace by Card. The algorithm for calculate_total() additionally depends upon the value assigned to an ace because that determines whether it should increment or decrement by 10 when adjusting the value of the hand. For these two reasons, the value assigned to an ace cannot be encapsulated internally within Card.

We could re-design so that the ace value is visible to both Card and Hand, but I think a cleaner design results from encapsulating that value internally within Hand (cleaner in the sense that there are fewer interdependencies). In this scheme, Card would encapsulate just the name of a card. An obvious choice would be to make it an enum. This may be good enough, but it must be borne in mind that in C/C++, unlike Pascal/Ada, an enum value is a thinly disguised integer which can be used in arithmetic operations without explicit casting. In the light of the exchange between Paul Whitehead and Francis Glassborow (C Vu 13.6, Dec. 2001) on ADTs, we may prefer to make it a class exporting static const values, with constructors disabled so that no other valid values can be created. (A default constructor creating an invalid value should be made available so that the class can be used with STL containers).

Again, I would separate the user input aspects of Card into a standalone function input(Card&). How should we map a character input by a user to a Card? The answer is in the question: a map. I really liked Colin Hersom's proposal to do this in the previous Critique, and I was only sorry that he didn't carry it through fully. His comment "See how it would have been neater if we used a class?" would more accurately have been stated "See how messy it is without a class?" If we make the map<char, Card> a private member variable of a wrapper class it can be populated in the constructor of the wrapper class. Then within input(Card&) all we need to do is declare a static local variable of the wrapper class.

The submitted code throws an exception when an invalid character is entered. I consider this unduly aggressive; miss-typing by a user should be expected by the program rather than treated as an exceptional occurrence. Instead the map-wrapper class could have a member function lookup(char, Card&) returning a bool to indicate whether it can map the character to a Card.

Turning then to the implementation of Hand, what container should we use? What should it contain? Do we want to make it a pointer so that we can use the pImpl/CheshireCat/Handle-Body/Bridge idiom/pattern (delete according to taste)? There are a number of valid options, depending upon the exact use we wish to make of our Hand class. We could keep these options open by making Hand a template, to be instantiated with the container implementation class. To describe how to do this is beyond the scope of a code critique; I will try to write a separate article.

If, as I have argued, Card encapsulates a name rather than a value a map is likely to be a more suitable container than a vector. We sometimes need to find aces in the hand, and find is optimised for maps (or should that be maps are optimised for find?). [For the paranoid, and it probably pays to be paranoid in relation to gambling card games, representing Card as a name rather than a value gives us the opportunity to check for five of the same card in a hand.]

How should calculate_total() accumulate the value of the hand? The answer is in the question (if we #include <numeric>):

unsigned int value =  std::accumulate(cards.begin(), cards.end(),
      0, AccumulateCardValue(cardValues()));

What is cardValues()? A function that returns a reference to another Colin Hersom style lookup map, wrapped in a class and declared as a static local variable within the function. The case for such a map rather than a switch-statement is stronger here: in the switch-statement submitted a calculation is performed every time a numerical value is looked up:

numerical_value = face_val - '0';

The syntactic tidiness of the switch fall through conceals the potential for inferior performance. Note that the common (in my experience) practice of declaring cardValues() as a private member function, in this case a static one, clutters the header file unnecessarily. Instead it can be declared within an unnamed namespace in the implementation file (equivalent to a static function in C).

What is AccumulateCardValue? A function object class more succinctly listed than described:

class AccumulateCardValue {
  public:
    AccumulateCardValue(
            const CardValues& cardValues)
      : values(cardValues){}
    unsigned int operator()(
          unsigned int accumulatedValue,
          Cards::value_type const& card) {
// Exact implementation depends upon
// Cards::value_type
// In pseudo code:
// card found = values.lookup(card name, card
// value) if not card found then throw
// exception return accumulatedValue + 
// (card value * number of cards)
    }
  private:
    const CardValues& values;
};

In this case it is an unexpected occurrence for the lookup to fail, so it is legitimate to throw an exception. I would simply derive my exception class from std::exception rather than build my own as done in the submitted code.

A final observation on calculate_total(): there is no need to check for aces during the accumulation loop, and indeed no need to check for them unless the hand value thus accumulated is less than 12.

I suspect some readers will remain uneasy about my proposal to strip the value attribute from the Card class. My solution is fine in the case that we only ever want to calculate the value of a hand once, when the hand is complete. Well, maybe that is all our application requires. It is, though, easy to imagine there might be a requirement to show the value of the hand after each card is added. In that case it is obviously sensible to cache the value of each card the first time it is looked up. This can easily be achieved within my design by changing the class of the objects held by the container, adding a value attribute to those objects. This value attribute could be initialised when the card is added to the hand (here we see the value of encapsulating card values in the cardValues() function), and the implementation of AccumulateCardValue is made simpler correspondingly. At this stage you may realise the usefulness of making the container implementation class a template parameter, and if you've used the pImpl idiom your clients won't even be affected by such a modification.

More often than not, critiques of source code focus on the code itself. I think that is often a mistake. We need to get to the fundamental design issue and when we have a hold of that the code often writes itself. With this in mind let us look at the source code Francis presented for critique this time.

A quick read of the code should start raising questions in your mind. If it is correct to make constructors explicit (which it generally is) what are those conversion operators doing there? What is the purpose of the three classes? More fundamentally, what is the code achieving? So back to the drawing board.

We want to input the denominations (though not suits) of between two and five cards and evaluate them as a blackjack hand. Apparently we need not consider special hands, just the best total that the hand will make. The first question I ask is why ask the user for the number of cards in the hand? Why not just count them in? We know we need two and we know that when we have five that is it. If we are going to count them in we need some way for the user to signal that the hand is complete. Let me use a common technique and simply use any invalid key to terminate input. Of course we will not allow termination until two cards have been put in.

Now let me make a list of the tools I need:

Let me write the driver first:

int main(){
  try{
    instructions();
    do{
      evalhand eh;
      eh.result();
    } while(again("Do you have another?")
  }  
  catch(...) {
    std::cout << "Stopping \n" << std::endl;
  }
  std::cout << "That is it." << endl;
  return 0;
}

I always put most of my main in a try block so that I will get clean up if some exception is thrown that I had not prepared for. It costs little so I can see little reason to do otherwise. Now let me write the various bits needed to allow that code to compile and link to produce an executable.

void instructions(){
  std::cout << "Ask me about it." << std::endl;
}

In real life you implement that function somewhat better, but you can refine that as you go and easily respond to user problems with understanding your instructions because you have isolated that task in its own function. I would rip out all the string literals and provide them through a file that could be customised for different human languages. The instructions function is a special case because it is all text and so could be provided in English, French, Spanish etc. versions.

Now what can we deduce from the evalhand eh; statement? Yes, evalhand must be a type. So we get (with a little thought):

class evalhand {
  int total;
  bool hasanace;
public:
  evalhand();
  void result();
};

What has that constructor for evalhand got to do? It has to read in data, card by card until finished keeping a running total, and a record of whether the hand contains an ace. Reading in a card is a service routine so we hide it away in the unnamed namespace of the implementation file for evalhand. I want to get a value back and a validation code. Something like this:

namespace{
  bool get(char & c) {
    std::cin >> c;
    return std::strchr("23456789tjqkaTJQKA", c)
  }

Note that that string literal used to show valid input characters may need adjusting for non-English speaking versions. While I remember, I can also use the following as another service function:

  void validinputs(){
    std::cout << "Type the first letter of"
          << " Ace, King, Queen, Jack, Ten"
          << " and 2 to 9 for the others."
          << std::endl;
}

I would polish the wording for my release version.".

It is about time to write the evalhand constructor (at file scope):

evalhand::evalhand():total(0), hasanace(false) {
  char card;
  std::cout << "First card please : ";
  while(!get(card)) {
    std::cout << "Input not recognised\n";
    validinputs();
  }
  total += value(card, hasanace);
  std::cout << "Second card please : ";
  while(!get(card)) {
    std::cout << "Input not recognised\n";
    validinputs();
  }
  total += value(card, hasanace);
  std::cout << "Any key other than one"
       "representing a card ends input.\n"
  for(int count=0; count<3; ++count){
    if(!get(card)) break;
    total += value(card, hasanace);
  }
}

I am leaving refactoring the common code in the above as an exercise for the reader. Note that I do not need exceptions. I avoid exceptions unless the problem detection and solution are necessarily remote from each other.

I slipped in another service function there so I will need to add that to my unnamed namespace:

  int value(char c, bool & isace){
    int val = 10;
    c = tolower(c);
    if(c=='a'){
      isace = true;
      val = 1;
    }
    if(c>'1' && c<='9') val = c-'0';
    return val;
}

Note that I can skip most of the tests by initialising val to the value of face cards . The get function has already validated the input.

Now to that member function:

void evalhand::result() {
  if(total<11 && hasanace) total += 10;
  if(total>21) std::cout << "You busted. "
  std::cout<<"Your total was: " << total << '\n';
}

Those who have tracked my code to here will realise that there is one more function to go. Well you should have a function such as again in stock. If you haven't it is time you wrote one. That gives me an idea. There are dozens of little utility functions like again. What about an ACCU library of them? They are tedious to write and many of them have little catches that need some thought. In this case even partial internationalisation needs a little care.

That is it. Please feel free to rip into my code and my design. However next time you do a code review think about the design issues first. That means you must focus on why the code is being written. You may not agree with the XP philosophy but there is a great deal to be said for leaving tomorrow's problems till tomorrow. Trying to solve them early often leads to undesirable complexity.