patterncppModerate
Scorekeeper for a game
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;
```
#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
Pass by value if you will copy
The
Exception types
You appear to be inventing your own exception types. This is unnecessary: the `
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.