How to make this C++ code more DRY?

Question

I have these two methods on a class that differ only in one method call. Obviously, this is very un-DRY, especially as both use the same formula.

int PlayerCharacter::getAttack() {
    int attack;
    attack = 1 + this->level;
    for(int i = 0; i < this->current_equipment; i++) {
        attack += this->equipment[i].getAttack();
    }
    attack *= sqrt(this->level);
    return attack;
}
int PlayerCharacter::getDefense() {
    int defense;
    defense = 1 + this->level;
    for(int i = 0; i < this->current_equipment; i++) {
        defense += this->equipment[i].getDefense();
    }
    defense *= sqrt(this->level);
    return defense;
}

How can I tidy this up in C++?

Answer 1

In my opinion, what you have is fine, as it will allow you to tweak attack/defense more than if you represented both of them with one function. Once you start testing your game, you'll begin balancing attack/defense formulas, so having separate functions for them is fine.

The whole concept of DRY [don't repeat yourself] is [hopefully] to prevent your code from becoming a huge copy & paste fest. In your situation, the defense/attack formulas will change over time [for example, what if characters have buffs/status-ailment? A specific status ailment might cut defense in half, while increasing attack by 2 (Berserk, FF reference, heh)]

Answer 2

One easy way is to represent all of a piece of equipment's attributes in an array, indexed by an enum.

enum Attributes {
  Attack, 
  Defense,
  AttributeMAX
};

class Equipment {
  std::vector<int> attributes;

  Equipment(int attack, int defense): attributes(AttributeMAX)
  {
    attributes[ATTACK] = attack;
    attributes[DEFENSE] = defense;
  }

};

Then you change your function to

int PlayerCharacter::getAttribute(int& value, Attribute attribute) {
    value = 1 + this->level;
    for(int i = 0; i <= current_equipment; i++) {
        value += this->equipment[i].attributes[attribute];
    }
    value *= sqrt(this->level);
    return value;
}

And you can call it like so

  player.getAttribute(player.attack, Attack);

Answer 3

well, I would at least consider extracting sqrt(this.level); as a separate function called getLevelModifier()

and

defense = 1 + this.level;

attack = 1 + this.level;

could be

defense = getBaseDefense();

attack= getBaseAttack();

Not only does this add flexibility, it also auto-documents your function.

Answer 4

From a strict refactoring point of view, you could do this:

int PlayerCharacter::getDefense() {
    return getAttribute(&EquipmentClass::getDefense);
}

int PlayerCharacter::getOffense() {
    return getAttribute(&EquipmentClass::getOffense);
}

int PlayerCharacter::getAttribute(int (EquipmentClass::*attributeFun)()) {
    int attribute = 0;
    attribute= 1 + this->level;
    for(int i = 0; i <= current_equipment; i++) {
        attribute += this->equipment[i].*attributeFun();
    }
    attribute *= sqrt(this->level);
    return attribute;
}

Answer 5

IMO, ItzWarty makes a reasonable point -- you may want to just leave the code alone. Assuming you decide that changing it is a good thing though, you could do something like this:

class equipment { 
public:
    int getAttack();
    int getDefense();
};

int PlayerCharacter::getBattleFactor(int (equipment::*get)()) { 
    int factor = level + 1;
    for (int i=0; i<current_equipment; ++i)
        factor += equipment[i].*get();
    return factor * sqrt(level + 1);
}

You'd call this like:

int attack = my_player.getBattleFactor(&equipment::getAttack);

or:

int defense = my_player.GetBattleFactor(&equipment::getDefense);

Edit:

Another obvious possibility would be to decree that any one piece of equipment can only be defensive or offensive. In this case, things become simpler still, to the point that it might even be questionable whether you really need a function at all:

class PlayerCharacter {
    std::vector<equipment> d_equip;
    std::vector<equipment> o_equip;

// ...

int d=level+1+std::accumulate(d_equip.begin(), d_equip.end(), 0)*sqrt(level+1);

int o=level+1+std::accumulate(o_equip.begin(), o_equip.end(), 0)*sqrt(level+1);

Answer 6

Apart from ltzWarty's answer I would recommend refactoring your loop into a function for better readability:

int PlayerCharacter::getEquipmentAttack() {
    int attack = 0;
    for(int i = 0; i <= current_equipment; i++) {
        attack += this.equipment[i].getAttack();
    }
    return attack;
}
int PlayerCharacter::getAttack() {
    int attack = 1 + this->level;
    attack += getEquipmentAttack();
    attack *= sqrt(this->level);
    return attack;
}

Also, when you declare your local variable attack you should initialize it immediately.

Answer 7

Depending on other code in the application it may or may not be worth it but an OOP approach would make defense and attack values objects of a class rather than a plain int. Then you could derive them from a common base class that has a get() method that calls a virtual getEquipmentRate() method defined by each of the subclasses as necessary.