HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppModerate

Scorekeeper for a game

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
gamescorekeeperfor

Problem

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

```
#include
#include
#include

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= 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 = 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;

Solution

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 ` 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.

Code Snippets

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

Context

StackExchange Code Review Q#60834, answer score: 10

Revisions (0)

No revisions yet.