patterncppMinor
Enumerations for a game character's statistics
Viewed 0 times
enumerationscharacterstatisticsgamefor
Problem
I would like to improve my functions and work properly on enums without all of those IFs as well as make it more consistent (for example
The file I'm reading from looks like this: http://ideone.com/cQ4M3g
I know I'm missing few things when I'm reading from a file, but I'll fix it later. I'm a beginner so this is probably horrible, but it is working.
Backgrounds::displayQualifingBackgroundNames()). How should I go around it properly?The file I'm reading from looks like this: http://ideone.com/cQ4M3g
I know I'm missing few things when I'm reading from a file, but I'll fix it later. I'm a beginner so this is probably horrible, but it is working.
#include
#include
#include
#include
#include
using namespace std;
const string stats[] = {"Upkeep","ExperienceGain","MeleeSkill","RangeSkill","Resolve","MeleeDefense","RangeDefense","HitPoints","Initiative","ChanceHead","Fatigue"};
string stringLowCase(string s) {
for(int i = 0 ; i 0) cout backgrounds;
public:
Backgrounds() : backgrounds(0) {}
void loadBackgrounds() {
cout > values;
// cout > value >> name;
backgrounds[current].modify(stringToEnum(name), value);
}
string emptyLine;
getline(readfile,emptyLine);
getline(readfile,emptyLine);
if(emptyLine == "END") {
break;
}
// backgrounds[current].display();
isEmpty = true;
}
// int aaa;
// cin >> aaa;
}
readfile.close();
cout = 0) {
system("cls");
bg.displayBackgroundNames();
bg.displayAvailableTags();
bg.displayBackground(index);
}
else if(index == -2) {
system("cls");
bg.displayQualifingBackgroundNames(stringToEnum(text));
}
else {
system("cls");
bg.displayBackgroundNames();
bg.displayAvailableTags();
cout << "ERROR: Wrong background name" << endl;
}
}
}Solution
Here's a question for you. Why do you need an enum? As far as I can tell your logic goes something like this:
Notice steps 3, 4 there are very repetitive. What if we could get our value directly from the string? And make it more efficient than a bunch of equality comparisons? I posit to you that we can.
Use Maps for Lookup
Right now, when you read in
What can we do with that? So much! Right now you do:
Let's replace that with just:
Where
This is less code that is less error prone while also being more efficient.
One other comment I wanted to make is in regards to the...
Department of Redundancy Department
You have a helper function,
Just move that check inside of
so you can write:
This directly leads into:
Don't do it. Ever. Writing
Passing by const ref?
There is no reason to pass fundamental types like
- Read string
- Make it lower case
- Convert it to enumeration with a series of
ifcomparisons
- Convert it to a value to modify with a
switch
Notice steps 3, 4 there are very repetitive. What if we could get our value directly from the string? And make it more efficient than a bunch of equality comparisons? I posit to you that we can.
Use Maps for Lookup
Right now, when you read in
fatigue, you have to compare it against every other modifier. And then do a switch on that. Let's use a hashtable instead - it's a far better way of doing lookup. We'll use the lower-case string as our key. What should we use as our value? Let's pick the most direct thing for our use-case: the actual pointers to the values!// private member on Background
static std::unordered_map stats_map{
{"upkeep", &Background::upkeep},
{"experiencegain", &Background::experienceGain},
...
};What can we do with that? So much! Right now you do:
backgrounds[current].modify(stringToEnum(name), value);Let's replace that with just:
backgrounds[current].modify(name, value);Where
modify() can now be (both arguments by value):void modify(std::string statName, int value) {
// to lower case
for (char& c : statName) {
c = tolower(c);
}
// look it up
auto it = stats_map.find(statName);
if (it != stats_map.end()) {
// assign as appropriate
auto member = it->second;
(this->*member) = value;
}
else {
// error!
}
}This is less code that is less error prone while also being more efficient.
One other comment I wanted to make is in regards to the...
Department of Redundancy Department
You have a helper function,
displayHelper(), in which you factored out a bunch of common code. This is great. However, as far as I can tell, it is always called like this:if (x != 0) {
displayHelper("foo", x);
}Just move that check inside of
displayHelper:void displayHelper(const std::string& statName, int value, bool percentage = false) const {
if (value != 0) {
std::cout 0) std::cout << "+";
std::cout << value;
if(percentage) std::cout << "%";
std::cout << endl;
}
}so you can write:
displayHelper("Experience Gain", experienceGain);
displayHelper("Melee Skill", meleeSkill);
displayHelper("Ranged Skill", rangeSkill);
...This directly leads into:
using namespace std;Don't do it. Ever. Writing
std:: takes 5 characters. See also why is "using namespace std;" considered bad practice?Passing by const ref?
There is no reason to pass fundamental types like
int or bool by reference-to-const. It may actually be more expensive too. Just pass them by value.Code Snippets
// private member on Background
static std::unordered_map<std::string, int Background::*> stats_map{
{"upkeep", &Background::upkeep},
{"experiencegain", &Background::experienceGain},
...
};backgrounds[current].modify(stringToEnum(name), value);backgrounds[current].modify(name, value);void modify(std::string statName, int value) {
// to lower case
for (char& c : statName) {
c = tolower(c);
}
// look it up
auto it = stats_map.find(statName);
if (it != stats_map.end()) {
// assign as appropriate
auto member = it->second;
(this->*member) = value;
}
else {
// error!
}
}if (x != 0) {
displayHelper("foo", x);
}Context
StackExchange Code Review Q#105901, answer score: 5
Revisions (0)
No revisions yet.