Scorekeeper for a game

Question

The code is not for any project, I just want to know the best coding practices. The code is also on GitHub.

#include <iostream>
#include <new>
#include <typeinfo>

using namespace std;

/****************************************************************
 * Exceptions
 ****************************************************************/
class RuntimeException {
private:
  string errorMsg;
public:
  RuntimeException(const string& err) { errorMsg = err; }
  string getMessage() const { return errorMsg; }
};

class IndexOutOfBounds : public RuntimeException {
public:
  IndexOutOfBounds(const string& err = "Index out of bounds!")
    : RuntimeException(err) {}
};

/****************************************************************
 * Chapter 3.1 - Game Entries
 *
 ****************************************************************/
// Game entry data structure:
class GameEntry {       // Stores the game scores
public:
  GameEntry(const string& n = "", int s = 0); // Constructor
  string getName() const;             // Get player name
  int getScore() const;               // Get player score
private:
  string name;          // Player's name
  int score;            // Player's score
};
// Definitions for Game entry
GameEntry::GameEntry(const string& n, int s)
  : name(n), score(s) {}
string GameEntry::getName() const { return name; }
int GameEntry::getScore() const { return score; }

ostream& operator<<(ostream& out, const GameEntry& obj) {
  out << '{' << obj.getName() << ',' << obj.getScore() << '}';
  return out;
}

// Scores data structure
class Scores {
public:
  Scores(int maxEnt = 10);  // Constructor
  ~Scores();            // Destructor
  void add (const GameEntry& e); // add an entry
  GameEntry remove(int i)    // Remove a single entry
    throw (IndexOutOfBounds);
  // Operators:
  GameEntry& operator[](size_t i) {
    return entries[i];
  };    // Access to game entries
  friend ostream& operator<<(ostream& out, const Scores& obj);
private:
  int maxEntries;       // maximum number of entries
  int numEntries;       // actual number of entries
  GameEntry* entries;       // array of game entries
};
// Definitions for Scores
Scores::Scores(int maxEnt) {
  maxEntries = maxEnt;
  entries = new GameEntry[maxEnt];
  numEntries = 0;
}
Scores::~Scores() {     // Destructor
  delete [] entries;
}
void Scores::add(const GameEntry& e) { // Add a game entry
  int newScore = e.getScore();         // Score to add
  if (numEntries == maxEntries){       // The array is full
    if (newScore <= entries[maxEntries-1].getScore())
      return;           // not high enough
  }
  else
    numEntries++;

  int i = numEntries - 2;   // Start with the next to last
  while (i >= 0 && newScore > entries[i].getScore()) {
    entries[i+1] = entries[i];
    i--;
  }
  entries[i+1] = e;
}
GameEntry Scores::remove(int i) throw(IndexOutOfBounds) { // Remove
  try{    // Exception for outof bounds
    if ( (i < 0) || (i >= numEntries) )
      throw IndexOutOfBounds("Invalid index");
  } catch (IndexOutOfBounds& iob) {
    cout << iob.getMessage() << endl;
    return GameEntry();
  }
  GameEntry e = entries[i];     // Save the removed object
  for (int j = i+1; j < numEntries; j++)
      entries[j-1] = entries[j];    // Shift entries left
  numEntries--;         // one fewer entry
  return e;         // return the removed object
}

ostream& operator<<(ostream& out, const Scores& obj){
  for (int i = 0; i < obj.numEntries; i++) {
    out << obj.entries[i] << ' ';
  }
  return out;
}


int main() {
  GameEntry *e0 = new GameEntry("Zafar", 10);
  GameEntry *e1 = new GameEntry("Zafa", 11);

  // Scores *s = new Scores(5);
  Scores s(5);
  s.add(*e0);
  s.add(*e1);
  s.add(GameEntry("John", 9));
  s.add(GameEntry("Alice", 19));
  s.add(GameEntry("Bob", 15));
  s.add(GameEntry("Connor", 1));
  cout << s << endl;
  s.remove(0);
  s.remove(6);


  cout << s << endl;
}

Answer 1

Using namespace std

This is a common issue. Don't do it.

Parameter names

Your parameter names to the GameEntry constructor aren't very illuminating. I would name them name_ and score_ to clarify the intent and avoid using the same name as your private fields.

Pass by value if you will copy

The GameEntry constructor takes a const string& and copies it. If you have C++11 enabled, it can be more efficient if you take the string by value and then move from it:

GameEntry::GameEntry(string n, int s)
  : name(std::move(n)), score(s) {}

Exception types

You appear to be inventing your own exception types. This is unnecessary: the <stdexcept> header already includes a number of standard exception types, among which is std::out_of_range, which should be used in place of IndexOutOfBounds.

Exception specifications

Exception specifications are deprecated as of C++11, and should not be used. This StackOverflow post provides some reasons why.

Exception handling

The only place you throw an exception, you immediately catch it and print a log statement. A better solution would be to throw the exception without catching it. This is better for several reasons:

• It is clear that an error has occurred. Printing an error message and returning an empty object does signal something has gone wrong, but an exception is a much better indicator.
• It forces the caller to handle the error. Index out-of-bounds is a serious logical error in a program, and needs to be dealt with by the caller. Failing fast makes it easier to identify and diagnose bugs.
• It allows the caller to handle the error however they want. Printing the error message inside remove() means the caller has no control over when and where the error is handled. It is very annoying (as a user) to have a function which will always print errors to the console, when I really want error messages silenced.

Answer 2

You should design your class to be as convenient for the caller as possible. Some annoyances are:

• Scores can only keep a limited number of entries.
• The caller needs to instantiate a GameEntry to be added. Not only is it inconvenient, it also led, in this case, to a memory leak. (You instantiated e0 and e1 but never deallocated them.)
• It's not immediately obvious how .remove() behaves. What does the i parameter mean? What if the score is tied — which player would be removed?

I would expect a score-keeping class to work more like this:

int main() {
    Scores s;
    s.set("Zafar", 10);
    s.set("Zafa", 11);

    s.set("John", 9);
    s.add("John", 1);       // John's score would be 10

    s.add("Alice", 19);     // Alice's score would be 19

    s.add("Bob", 15);
    s.add("Connor", 1);
    cout << s << endl;

    s.remove("Alice");

    cout << s << endl;
}

I would suggest implementing it using a std::priority_queue<GameEntry>, so that you can easily keep the players sorted by their score.

Answer 3

With regards to allocation, 200_success went into this a bit, but I'd like to harp on it some more from a more technical perspective.

No dynamic allocation should be happening. entries should be a std::vector (or a more suited structure like 200_success suggested). Also, e0 and e1 shouldn't be dynamically allocated. Unless there's specific reasons for it (won't fit on the stack, need a pointer for nullability, non-scope lifetime etc), avoid non-automatic memory management as much as possible. And even when you do have a legitimate reason to use it, use smart pointers. They're leak-free, exception safe, and have much less confusing semantics.

It's worth noting that if you do go the manual resource management route there's a lot of things you need to be aware of (whether using smart pointers or not). For example, you're violating the rule of three (or 4 or 5 in C++11).

GameScores s1(3);
GameScores s2 = s1;

Since the default copy constructor does an element-wise copy, this will result in s2 having a pointer to the same entries array. This means that not only are the two now confusingly backed by the same data, the array will be double deleted when the objects get destructed. This can have catastrophic consequences.

The proper way to handle this is to provide a copy-constructor that actually copies the underlying data, along with an assignment operator that behaves as expected. The rule of thumb is, if you have a destructor, you almost certainly need a copy constructor and assignment operator as well (or really, if you have one of the three, you need all of the three).

---

One more thing jumped out at me that hasn't previously been mentioned. You're not using explicit constructors.

void f(const Scores& s) { ... }
f(3);

Would you expect that to compile? It will. The exact process of what's going on is fairly wordy, but the short of it is that one implicit conversion is allowed, and the constructor of Score is eligible. Basically the compiler sees 3, and int, and it sees a const Scores&, then it notices Scores::Scores(int) and goes "ooohhh, I can just drop the int in there and get what I need!"

There are legitimate uses of this, but in general, unless you've carefully thought it through and decided that you want this behavior, you should mark the constructor explicit so it won't happen (explicit Scores(int maxEnt);).

The most common situation that you have to be wary of this is single argument constructors, but it's actually possible with any situation that only involves one conversion. This means that your GameEntry constructor is prone to the same problem and should likewise be marked explicit.

It's worth noting (if you're interested in this sort of thing :p), that to get the same example as used with Scores to work, you would actually have to pass a std::string, not something from which a std::string can be implicitly constructed (perfect example of an implicit constructor being good by the way -- void f(const std::string& s) { ... } f("hello"); is quite natural).

void f(const GameEntry& entry) { ... }
f("compilation error"); // Compilation error
std::string example;
f(example); // Compiles
f(std::string("...")); // Compiles

Rule of thumb: If you have a constructor that takes only one parameter, or a constructor that takes more than one parameter and 1 or fewer of the parameters have defaults, you should mark it explicit. In potentially clearer words, if you can construct something like Type var(oneParameter); then it should be explicit (unless of course you want implicit construction).

---

Also, while I'm in paranoid about memory management, you should be aware of the risk of allocation in exceptions.

You're not only implicitly constructing a string if the default constructor is used, you're copying a std::string. Both of these operations can (and likely do) involve memory allocation. Memory allocation can fail, and when it does, it (usually) throws an exception. You can likely see where this is going, but what is relevent is that if an exception is thrown while trying to throw an exception, the program terminates (technically terminate() is called).

Whether this really matters depends on the use case of the code throwing your exception (and it usually doesn't), but it's something to be aware of, especially if you can get by with a good old literal C-string (i.e. having your exception class store a const char* and be constructed off of literal strings like SomeException("Some literal string")).