Как сделать этот код C ++ более сухим? - PullRequest
Новая
       17

Как сделать этот код C ++ более сухим?

7 голосов
/ 19 апреля 2010

У меня есть эти два метода в классе, которые отличаются только одним вызовом метода. Очевидно, это очень НЕ СУХО, особенно потому, что оба используют одну и ту же формулу. 

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 ++?

Ответы [ 7 ]

18 голосов
/ 19 апреля 2010

Один простой способ - представить все атрибуты оборудования в массиве, проиндексированном перечислением. 

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);
12 голосов
/ 19 апреля 2010

На мой взгляд, то, что у вас есть, хорошо, так как позволит вам настроить атаку / защиту больше, чем если бы вы представляли их обоих одной функцией. Как только вы начнете тестировать свою игру, вы начнете балансировать формулы атаки / защиты, так что наличие отдельных функций для них - это хорошо.

Вся концепция DRY [не повторяйся] состоит в [надеюсь], чтобы предотвратить превращение твоего кода в огромный фестиваль копирования и вставки. В вашей ситуации формулы защиты / атаки со временем меняются [например, что если у персонажей есть баффы / статус-недуг? Определенный статусный недуг может сократить защиту в два раза, в то же время увеличивая атаку на 2 (Берсерк, ссылка FF, хех)]

6 голосов
/ 19 апреля 2010

С точки зрения строгого рефакторинга, вы можете сделать это: 

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;
}
5 голосов
/ 19 апреля 2010

ну, я бы хотя бы рассмотрел извлечение sqrt(this.level); как отдельную функцию с именем getLevelModifier()

и 

defense = 1 + this.level;

attack = 1 + this.level;

может быть 

defense = getBaseDefense();

attack= getBaseAttack();

Это не только добавляет гибкости, но и автоматически документирует вашу функцию.

3 голосов
/ 19 апреля 2010

В зависимости от другого кода в приложении это может или не может стоить, но подход ООП сделал бы защиту и атаку объектами значений класса, а не обычным int. Затем вы можете получить их из общего базового класса, у которого есть метод get (), который вызывает виртуальный метод getEquipmentRate (), определенный каждым из подклассов, как необходимо.

2 голосов
/ 19 апреля 2010

Кроме ответа 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, вы должны немедленно ее инициализировать .

2 голосов
/ 19 апреля 2010

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

Edit:

Другой очевидной возможностью было бы постановление о том, что любая единица оборудования может только быть оборонительной или наступательной. В этом случае все становится еще проще, так что даже может быть сомнительно, нужна ли вам вообще функция: 

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