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

Enumerations for a game character's statistics

Submitted by: @import:stackexchange-codereview··
0
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 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:

  • Read string



  • Make it lower case



  • Convert it to enumeration with a series of if comparisons



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