如何让这个C++代码更加DRY？

Question

我有一个类上的两个方法，它们只有一个方法调用不同。显然，这非常不符合DRY原则，尤其是两者都使用相同的公式。

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

我该如何在C++中整理这段代码？

- Macha

---

这是什么意思？请给我们提供一些真实的代码。 :) - GManNickG

GMan所说的话。另外，attack和 defense是全局变量还是你省略了它们的定义？ - sbi

你的循环中似乎存在一个偏移错误。 - anon

我必须努力去发现DRY的含义，这并没有帮助到我。能够提供答案的人数比理解问题的人数要多得多。你可能已经限制了回答的数量。 - Clifford

@sbi：不要重复自己是我最好的猜测 - Dennis Zickefoose

Answer 1

一种简单的方法是使用一个由枚举类型索引的数组，来表示设备属性的所有信息。

enum Attributes {
  Attack, 
  Defense,
  AttributeMAX
};

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

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

};

那么你需要修改你的函数为

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

你可以这样调用它

  player.getAttribute(player.attack, Attack);

- Bill Prin

---

想到这个了，但是懒得表达出来。=) - Nailer

这是一个很好的解决方案，因为它不会混淆代码的意图。然而，它确实倾向于混淆使用方法。但是，通过创建两个包装程序getAttack和getDefence来调用该程序，这个问题很容易解决。(编辑)但我要说的是，player.getAttribute(player.attack, Attack)本身就非常清晰明了。 - Torlack

+1. 当然，在构造函数中赋值之前，您需要将“attributes”初始化为属性的数量。 - Bill

谢谢，由于某种原因，我认为如果您尝试超出范围索引向量，则会动态扩展。 - Bill Prin

Answer 2

在我看来，你所做的很好，因为这样可以让你调整攻击/防御的方式，而不像用一个函数表示它们那样限制了。一旦你开始测试你的游戏，你就会开始平衡攻击/防御公式，所以为它们分别创建函数是没问题的。

DRY [don't repeat yourself] 的整个概念是为了防止你的代码变成大量复制粘贴的操作。在你的情况下，防御/攻击公式将会随时间而改变[例如，如果角色有增益效果/状态异常？某些状态异常可能会削减防御值一半，同时增加攻击力2倍（狂暴状态，最终幻想参考）]

- Warty

---

我同意。虽然在理论上有办法将你所做的底层数学操作进行DRY化（使用一些函数指针），但是长远来看，你很可能还是会为每个情况编写不同的包装方法，因为（就像@ItzWarty建议的那样）你开始以不同的方式修改这两个值。 - Chris Tonkinson

Answer 3

从严格的重构角度来看，你可以这样做：

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

- Eclipse

Answer 4

嗯，我至少会考虑将 sqrt(this.level); 提取为一个名为 getLevelModifier() 的单独函数。

以及

defense = 1 + this.level;

attack = 1 + this.level;

可以是

defense = getBaseDefense();

attack= getBaseAttack();

这不仅增加了灵活性，还自动记录了您的函数。

- Nailer

Answer 5

根据应用程序中的其他代码，这可能或可能不值得，但OOP方法将使防御和攻击值成为类的对象，而不是普通的int。然后，您可以从一个公共基类派生它们，该基类具有一个get()方法，该方法调用每个子类根据需要定义的虚拟getEquipmentRate()方法。

- Clifford

Answer 6

在我看来，ItzWarty提出了一个合理的观点——你可能只想让代码保持原样。不过，如果你决定改变它是一件好事，那么你可以这样做：

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);
}

您可以这样调用：

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

或者：

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

编辑：

另一个明显的可能性是规定任何一件装备只能是防御或攻击。在这种情况下，事情变得更加简单，甚至可以质疑是否真的需要一个函数：

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

- Jerry Coffin

---

+1 对于“BattleFactor”。这听起来很棒，让我想玩它（无论它是什么）。 - Björn Pollex

Answer 7

除了ltzWarty的答案，我建议将您的循环重构为一个函数，以提高可读性：

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

此外，当您声明本地变量attack时，应该立即进行初始化。

- Björn Pollex