Является ли это запахом кода, чтобы хранить общие объекты в контейнере, а затем получать объекты и выгружать объекты из контейнера?

34

Например, у меня есть игра, в которой есть несколько инструментов для повышения способности игрока:

Tool.h

class Tool{
public:
    std::string name;
};

И некоторые инструменты:

Sword.h

class Sword : public Tool{
public:
    Sword(){
        this->name="Sword";
    }
    int attack;
};

Shield.h

class Shield : public Tool{
public:
    Shield(){
        this->name="Shield";
    }
    int defense;
};

MagicCloth.h

class MagicCloth : public Tool{
public:
    MagicCloth(){
        this->name="MagicCloth";
    }
    int attack;
    int defense;
};

И тогда игрок может держать некоторые инструменты для атаки:

class Player{
public:
    int attack;
    int defense;
    vector<Tool*> tools;
    void attack(){
        //original attack and defense
        int currentAttack=this->attack;
        int currentDefense=this->defense;
        //calculate attack and defense affected by tools
        for(Tool* tool : tools){
            if(tool->name=="Sword"){
                Sword* sword=(Sword*)tool;
                currentAttack+=sword->attack;
            }else if(tool->name=="Shield"){
                Shield* shield=(Shield*)tool;
                currentDefense+=shield->defense;
            }else if(tool->name=="MagicCloth"){
                MagicCloth* magicCloth=(MagicCloth*)tool;
                currentAttack+=magicCloth->attack;
                currentDefense+=magicCloth->shield;
            }
        }
        //some other functions to start attack
    }
};

Я думаю, что это трудно заменить if-elseвиртуальными методами в инструментах, потому что каждый инструмент имеет разные свойства, и каждый инструмент влияет на атаку и защиту игрока, для чего необходимо обновить обновление атаки и защиты игрока внутри объекта Player.

Но я не был удовлетворен этим дизайном, так как он содержит уныние, с длинным if-elseзаявлением. Нужно ли «исправлять» этот дизайн? Если так, что я могу сделать, чтобы исправить это?

ggrr
источник
4
Стандартный метод ООП для удаления тестов для определенного подкласса (и последующих понижений) заключается в создании или в этом случае, возможно, двух виртуальных методов в базовом классе для использования вместо цепочки if и приведений. Это можно использовать, чтобы полностью удалить if и делегировать эту операцию подклассам для реализации. Вам также не придется редактировать операторы if каждый раз, когда вы добавляете новый подкласс.
Эрик Эйдт
2
Также рассмотрите Двойную Отправку.
Борис Паук
Почему бы не добавить в свой класс Tool свойство, содержащее словарь типов атрибутов (например, атака, защита) и присвоенное ему значение. Атака, защита могут быть перечислены значения. Затем вы можете просто вызвать значение из самого инструмента по перечисляемой константе.
user1740075 10.06.16
8
Я просто оставлю это здесь: ericlippert.com/2015/04/27/wizards-and-warriors-part-one
Ты,
1
Также см. Шаблон посетителя.
JDługosz

Ответы:

63

Да, это кодовый запах (во многих случаях).

Я думаю, что трудно заменить if-else виртуальными методами в инструментах

В вашем примере довольно просто заменить if / else виртуальными методами:

class Tool{
 public:
   virtual int GetAttack() const=0;
   virtual int GetDefense() const=0;
};

class Sword : public Tool{
    // ...
 public:
   virtual int GetAttack() const {return attack;}
   virtual int GetDefense() const{return 0;}
};

Теперь больше нет необходимости в вашем ifблоке, вызывающий может просто использовать его как

       currentAttack+=tool->GetAttack();
       currentDefense+=tool->GetDefense();

Конечно, для более сложных ситуаций такое решение не всегда столь очевидно (но тем не менее почти всегда возможно). Но если вы попадаете в ситуацию, когда вы не знаете, как решить проблему с помощью виртуальных методов, вы можете снова задать новый вопрос здесь, в разделе «Программисты» (или, если это становится специфичным для языка или реализации, в Stackoverflow).

Док Браун
источник
4
или, в этом отношении, на gamedev.stackexchange.com
Кромстер говорит, что поддерживает Монику
7
Вам даже не понадобится концепция Swordэтого пути в вашей кодовой базе. Вы можете просто new Tool("sword", swordAttack, swordDefense)из, например, файла JSON.
AmazingDreams
7
@AmazingDreams: это правильно (для частей кода, которые мы видим здесь), но я предполагаю, что ОП упростил свой реальный код для своего вопроса, чтобы сосредоточиться на аспекте, который он хотел обсудить.
Док Браун
3
Это не намного лучше, чем оригинальный код (ну, это немного). Любой инструмент, имеющий дополнительные свойства, не может быть создан без добавления дополнительных методов. Я думаю, что в этом случае следует отдавать предпочтение композиции, а не наследованию. Да, в настоящее время есть только атака и защита, но это не должно оставаться таким.
Полигном
1
@DocBrown Да, это правда, хотя это выглядит как RPG, где у персонажа есть некоторые характеристики, которые модифицируются инструментами или скорее экипированными предметами. Я хотел бы сделать обобщение Toolсо всеми возможными модификаторами, заполнить некоторые vector<Tool*>материалами, считанными из файла данных, а затем просто зациклить их и изменить статистику, как вы делаете сейчас. Однако у вас могут возникнуть проблемы, если вы хотите, чтобы предмет давал, например, 10% бонус за атаку. Возможно, это tool->modify(playerStats)другой вариант.
AmazingDreams
23

Основная проблема с вашим кодом заключается в том, что всякий раз, когда вы вводите какой-либо новый элемент, вам нужно не только писать и обновлять код элемента, но и модифицировать проигрыватель (или везде, где используется элемент), что делает все это намного сложнее.

Как общее практическое правило, я думаю, что это всегда немного подозрительно, когда вы не можете полагаться на нормальное наследование / наследование и вынуждены выполнять пересмотр самостоятельно.

Я мог бы подумать о двух возможных подходах, делающих все это более гибким:

  • Как уже упоминалось другие, перемещения attackи defenseчленов базового класса и просто инициализировать их 0. Это также может быть двойной проверкой, действительно ли вы можете отбросить предмет для атаки или использовать его для блокирования атак.

  • Создать какую-то систему обратного вызова / событий. Есть разные возможные подходы к этому.

    Как насчет простоты?

    • Вы можете создать членов базового класса, таких как virtual void onEquip(Owner*) {}и virtual void onUnequip(Owner*) {}.
    • Их перегрузки будут вызываться и изменять статистику при (не) оснащении предмета, например, virtual void onEquip(Owner *o) { o->modifyStat("attack", attackValue); }и virtual void onUnequip(Owner *o) { o->modifyStat("attack", -attackValue); }.
    • Доступ к статистике можно получить некоторым динамическим способом, например, используя короткую строку или константу в качестве ключа, так что вы можете даже ввести новые значения или бонусы, связанные с экипировкой, которые не обязательно должны обрабатываться конкретно вашим игроком или «владельцем».
    • По сравнению с простым запросом значений атаки / защиты как раз вовремя, это не только делает все более динамичным, но и экономит ненужные вызовы и даже позволяет создавать предметы, которые будут постоянно влиять на вашего персонажа.

      Например, представьте проклятое кольцо, которое будет устанавливать скрытую стат, когда он будет экипирован, помечая вашего персонажа как проклятого навсегда.

Марио
источник
7

Хотя @DocBrown дал хороший ответ, он не зашел достаточно далеко, имхо. Прежде чем вы начнете оценивать ответы, вы должны оценить свои потребности. Что тебе действительно нужно ?

Ниже я покажу два возможных решения, которые предлагают разные преимущества для разных нужд.

Первый очень упрощен и специально разработан для того, что вы показали:

class Tool {
    public:
        std::string name;
        int attack;
        int defense;
}

public void attack() {
    int attack = this->attack;
    int defense = this->defense;
    for (Tool* tool : tools){
        attack += tool->attack;
        defense += tool->defense;
    }
}

Это позволяет очень легко сериализовать / десериализовывать инструменты (например, для сохранения или работы в сети) и вообще не требует виртуальной отправки. Если ваш код - это все, что вы показали, и вы не ожидаете, что он будет развиваться совсем иначе, чем использование более разных инструментов с разными именами и той статистикой, только в разных количествах, тогда это путь.

@DocBrown предлагает решение, которое все еще основано на виртуальной диспетчеризации, и это может быть преимуществом, если вы каким-то образом специализируете инструменты для частей вашего кода, которые не были показаны. Однако, если вам действительно нужно или вы хотите изменить другое поведение, я бы предложил следующее решение:

Состав по наследству

Что если вам позже понадобится инструмент, который изменяет ловкость ? Или скорость бега ? Мне кажется, вы делаете RPG. Одна вещь, которая важна для RPG - это быть открытым для расширения. . Решения, показанные до сих пор, не предлагают этого. Вам нужно будет изменить Toolкласс и добавить в него новые виртуальные методы каждый раз, когда вам понадобится новый атрибут.

Второе решение, которое я показываю, это то, на которое я намекал ранее в комментарии - оно использует композицию вместо наследования и следует принципу «закрыто для модификации, открыто для расширения *». Если вы знакомы с работой систем сущностей, некоторые вещи будет выглядеть знакомо (мне нравится думать о композиции как о младшем брате ES).

Обратите внимание, что то, что я показываю ниже, значительно более элегантно в языках с информацией о типах во время выполнения, таких как Java или C #. Поэтому код C ++, который я показываю, должен включать в себя некоторую «бухгалтерию», которая просто необходима, чтобы заставить композицию работать прямо здесь. Может быть, кто-то с большим опытом C ++ может предложить еще лучший подход.

Сначала мы снова смотрим на сторону звонящего . В вашем примере вы, как вызывающая сторона внутри attackметода, вообще не заботитесь об инструментах. Что вас волнует, так это два свойства - точки атаки и защиты. Ты не очень волнует , где те пришли, и вы не заботитесь о других свойствах (например , скорости бега, аджилити).

Итак, сначала мы вводим новый класс

class Component {
    public:
        // we need this, in Java we'd simply use getClass()
        virtual std::string type() const = 0;
};

И затем мы создаем наши первые два компонента

class Attack : public Component {
    public:
        std::string type() const override { return std::string("mygame::components::Attack"); }
        int attackValue = 0;
};

class Defense : public Component {
    public:
      std::string type() const override { return std::string("mygame::components::Defense"); }
      int defenseValue = 0;
};

После этого мы заставляем инструмент содержать набор свойств и делаем свойства доступными для запроса другими.

class Tool {
private:
    std::map<std::string, Component*> components;

public:
    /** Adds a component to the tool */
    void addComponent(Component* component) { 
        components[component->type()] = component;
    };
    /** Removes a component from the tool */
    void removeComponent(Component* component) { components.erase(component->type()); };
    /** Return the component with the given type */
    Component* getComponentByType(std::string type) { 
        std::map<std::string, Component*>::iterator it = components.find(type);
        if (it != components.end()) { return it->second; }
        return nullptr;
    };
    /** Check wether a tol has a given component */
    bool hasComponent(std::string type) {
        std::map<std::string, Component*>::iterator it = components.find(type);
        return it != components.end();
    }
};

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

Теперь нам нужен способ извлечения инструментов по типам компонентов. Вы все еще можете использовать вектор для инструментов, как в вашем примере кода:

class Player {
    private:
        int attack = 0; 
        int defense = 0;
        int walkSpeed;
    public:
        std::vector<Tool*> tools;
        std::vector<Tool*> getToolsByComponentType(std::string type) {
            std::vector<Tool*> retVal;
            for (Tool* tool : tools) {
                if (tool->hasComponent(type)) { 
                    retVal.push_back(tool); 
                }
            }
            return retVal;
        }

        void doAttack() {
            int attackValue = this->attack;
            int defenseValue = this->defense;

            for (Tool* tool : this->getToolsByComponentType(std::string("mygame::components::Attack"))) {
                Attack* component = (Attack*) tool->getComponentByType(std::string("mygame::components::Attack"));
                attackValue += component->attackValue;
            }
            for (Tool* tool : this->getToolsByComponentType(std::string("mygame::components::Defense"))) {
                Defense* component = (Defense*)tool->getComponentByType(std::string("mygame::components::Defense"));
                defenseValue += component->defenseValue;
            }
            std::cout << "Attack with strength " << attackValue << "! Defend with strenght " << defenseValue << "!";
        }
};

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

Какие преимущества имеет этот подход? В attack, вы обрабатываете инструменты, которые имеют два компонента - вам больше ничего не нужно.

Давайте представим, что у вас есть walkToметод, и теперь вы решили, что было бы неплохо, если бы какой-нибудь инструмент мог изменять вашу скорость ходьбы. Нет проблем!

Сначала создайте новое Component:

class WalkSpeed : public Component {
public:
    std::string type() const override { return std::string("mygame::components::WalkSpeed"); }
    int speedBonus;
};

Затем вы просто добавляете экземпляр этого компонента в инструмент, который хотите увеличить скорость пробуждения, и меняете WalkToметод для обработки только что созданного компонента:

void walkTo() {
    int walkSpeed = this->walkSpeed;

    for (Tool* tool : this->getToolsByComponentType(std::string("mygame::components:WalkSpeed"))) {
        WalkSpeed* component = (WalkSpeed*)tool->getComponentByType(std::string("mygame::components::Defense"));
        walkSpeed += component->speedBonus;
        std::cout << "Walk with " << walkSpeed << std::endl;
    }
}

Обратите внимание, что мы добавили некоторое поведение в наши инструменты без изменения класса инструментов вообще.

Вы можете (и должны) переместить строки в макро или статическую переменную const, чтобы вам не приходилось вводить их снова и снова.

Если вы продолжите этот подход - например, создадите компоненты, которые могут быть добавлены к игроку, и создадите Combatкомпонент, который помечает игрока как способного участвовать в бою, то вы также можете избавиться от attackметода и позволить этому быть обработанным. Компонентом или обрабатываться в другом месте.

Преимущество в том, что игрок сможет получать Компоненты, также заключается в том, что вам даже не нужно будет менять игрока, чтобы он мог по-разному вести себя. В моем примере вы могли бы создать Movableкомпонент, так что вам не нужно реализовыватьwalkTo метод на игроке, чтобы заставить его двигаться. Вы просто создадите компонент, подключите его к плееру и позволите кому-то другому обработать его.

Вы можете найти полный пример в этой сути: https://gist.github.com/NetzwergX/3a29e1b106c6bb9c7308e89dd715ee20

Это решение, очевидно, немного сложнее, чем другие, которые были опубликованы. Но в зависимости от того, насколько вы хотите быть гибким, как далеко вы хотите пройти, это может быть очень мощный подход.

редактировать

Некоторые другие ответы предлагают прямое наследование (Создание мечей расширяет Инструмент, делает Щит расширяет Инструмент). Я не думаю, что это сценарий, где наследование работает очень хорошо. Что если вы решите, что блокировка щитом определенным образом также может повредить атакующему? С моим решением вы можете просто добавить компонент Attack к щиту и понять это без каких-либо изменений в вашем коде. С наследованием у тебя будут проблемы. Предметы / инструменты в RPG - главные кандидаты на составление или даже прямое использование систем сущностей с самого начала.

Polygnome
источник
1

Вообще говоря, если вам когда-либо понадобится использовать if(в сочетании с требованием указать тип экземпляра) на любом языке ООП, это признак того, что происходит что-то вонючее. По крайней мере, вы должны присмотреться к своим моделям.

Я бы смоделировал ваш домен по-другому.

Для вашего варианта использования a Toolимеет a AttackBonusи a DefenseBonus- и то, и другое может быть 0в случае, если оно бесполезно для борьбы, как перья или что-то в этом роде.

Для атаки у вас есть baserate+ bonusот используемого оружия. То же самое касается обороны baserate+ bonus.

Следовательно, у вас Toolдолжен быть virtualметод расчета бонусов атаки / защиты.

ТЛ; др

С лучшим дизайном вы можете избежать хакерских ifс.

Томас Джанк
источник
Иногда, если необходимо, например, при сравнении скалярных значений. Для переключения типа объекта не так много.
Энди
Ха-ха, если это довольно важный оператор, и вы не можете просто сказать, что использование - это запах кода.
тымтам
1
@ Тымски в каком-то смысле ты прав. Я сделал себя более ясным. Я не защищаю ifменьше программирования. Главным образом в комбинациях, как будто instanceofили что-то подобное. Но есть позиция, которая утверждает if, что это кодовая ячейка, и есть способы обойти это. И вы правы, это важный оператор, который имеет свое право.
Томас Джанк
1

Как написано, это «пахнет», но это могут быть только те примеры, которые вы привели. Хранение данных в контейнерах универсальных объектов, а затем их преобразование для получения доступа к данным не является запахом кода. Вы увидите, что он используется во многих ситуациях. Однако, когда вы используете его, вы должны знать, что вы делаете, как вы это делаете и почему. Когда я смотрю на пример, использование сравнения на основе строк говорит мне, какой объект является тем, что является предметом, который срабатывает в моем личном измерителе запаха. Это говорит о том, что вы не совсем уверены в том, что вы здесь делаете (и это хорошо, поскольку у вас хватило мудрости прийти сюда к программистам. SE и сказать: «Эй, мне не нравится то, что я делаю, помогите меня нет! ").

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

Например, что если я захочу скопировать плеер? Если я просто смотрю на содержимое объекта проигрывателя, это выглядит довольно просто. Мне просто нужно скопировать attack, defenseи toolsпеременные. Проще простого! Что ж, я быстро выясню, что использование указателей делает его немного сложнее (в какой-то момент стоит взглянуть на умные указатели, но это уже другая тема). Это легко решается. Я просто создаю новые копии каждого инструмента и помещаю их в свой новый toolsсписок. В конце концов, Toolэто действительно простой класс с одним членом. Поэтому я создал кучу копий, включая копию Sword, но я не знал, что это меч, поэтому я только скопировал name. Позже, attack()функция смотрит на имя, видит, что это «меч», забрасывает его, и случаются плохие вещи!

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

int sockfd = socket(AF_INET, SOCK_STREAM, 0);
sockaddr_in serv_addr;
serv_addr.sin_family = AF_INET;
serv_addr.sin_port = htons(portno);
serv_addr.sin_addr.s_addr = INADDR_ANY;
bind(sockfd, (struct sockaddr *) &serv_addr, sizeof(serv_addr));

Почему это тот же шаблон? Потому bindчто не принимает sockaddr_in*, он принимает более общий sockaddr*. Если вы посмотрите на определения для этих классов, мы увидим, что у нас sockaddrесть только один член семьи, который мы присвоили sin_family*. Семья говорит, к какому подтипу следует применить sockaddr. AF_INETговорит вам , что адрес структура на самом деле sockaddr_in. Если бы это было AF_INET6, адрес был бы sockaddr_in6, который имеет большие поля для поддержки больших адресов IPv6.

Это идентично вашему Toolпримеру, за исключением того, что оно использует целое число, чтобы указать, какое семейство, а не std::string. Тем не менее, я собираюсь утверждать, что он не пахнет, и постараюсь сделать это по причинам, отличным от «это стандартный способ сделать сокеты, поэтому он не должен« пахнуть ». Очевидно, это тот же шаблон, который почему я утверждаю, что хранение данных в универсальных объектах и ​​их преобразование не являются автоматически запахом кода, но есть некоторые различия в том, как они это делают, что делает их более безопасными.

При использовании этого шаблона наиболее важной информацией является передача информации о подклассе от производителя к потребителю. Это то, что вы делаете с nameполем, а сокеты UNIX делают с их sin_familyполем. Это поле - информация, которая нужна потребителю, чтобы понять, что на самом деле создал производитель. Во всех случаях этого шаблона это должно быть перечисление (или, по крайней мере, целое число, действующее как перечисление). Зачем? Подумайте, что ваш потребитель собирается делать с информацией. Им нужно будет выписать некоторые большиеif заявление илиswitchоператор, как вы сделали, где они определяют правильный подтип, приводят его и используют данные. По определению, может быть только небольшое количество этих типов. Вы можете хранить его в виде строки, как и раньше, но у этого есть множество недостатков:

  • Медленно - std::stringобычно требуется некоторая динамическая память, чтобы сохранить строку. Вы также должны делать полнотекстовое сравнение, чтобы соответствовать имени каждый раз, когда вы хотите выяснить, какой у вас подкласс.
  • Слишком разносторонний - нужно сказать, что нужно ограничивать себя, когда вы делаете что-то чрезвычайно опасное. У меня были такие системы, которые искали подстроку, чтобы сказать, на какой тип объекта он смотрит. Это прекрасно работало до тех пор, пока имя объекта случайно не содержало эту подстроку, и не создало ужасно загадочную ошибку. Поскольку, как мы указывали выше, нам нужно лишь небольшое количество случаев, нет никаких причин использовать инструмент с огромным количеством мощностей, такой как строки. Это ведет к...
  • Склонность к ошибкам. Давайте просто скажем, что вы захотите совершить убийственное неистовство, пытаясь выяснить, почему ничего не работает, когда один потребитель случайно установил имя волшебной ткани MagicC1oth. Серьезно, на такие ошибки могут уйти дни, пока они не поняли, что произошло.

Перечисление работает намного лучше. Это быстро, дешево и намного менее подвержено ошибкам:

class Tool {
public:
    enum TypeE {
        kSword,
        kShield,
        kMagicCloth
    };
    TypeE type;

    std::string typeName() const {
        switch(type) {
            case kSword:      return "Sword";
            case kSheild:     return "Sheild";
            case kMagicCloth: return "Magic Cloth";

            default:
                throw std::runtime_error("Invalid enum!");
        }
   }
};

Этот пример также демонстрирует switchоператор, включающий перечисления, с единственной наиболее важной частью этого шаблона: defaultрегистр, который выбрасывает. Вы никогда не должны попадать в такую ​​ситуацию, если вы все делаете идеально. Однако, если кто-то добавит новый тип инструмента, и вы забудете обновить свой код для его поддержки, вы захотите, чтобы что-то перехватило ошибку. На самом деле, я так рекомендую их, что вы должны добавить их, даже если они вам не нужны.

Другим огромным преимуществом enumявляется то, что он дает следующему разработчику полный список допустимых типов инструментов, сразу же. Нет необходимости изучать код, чтобы найти специализированный класс Флейты Боба, который он использует в своей эпической битве с боссом.

void damageWargear(Tool* tool)
{
    switch(tool->type)
    {
        case Tool::kSword:
            static_cast<Sword*>(tool)->damageSword();
            break;
        case Tool::kShield:
            static_cast<Sword*>(tool)->damageShield();
            break;
        default:
            break; // Ignore all other objects
    }
}

Да, я вставил «пустую» инструкцию по умолчанию, просто чтобы ясно указать следующему разработчику, что я ожидаю, если какой-то новый неожиданный тип появится у меня на пути.

Если вы сделаете это, шаблон будет меньше пахнуть. Однако, чтобы быть без запаха, последнее, что вам нужно сделать, это рассмотреть другие варианты. Эти приведения являются одними из наиболее мощных и опасных инструментов, которые есть в репертуаре C ++. Вы не должны использовать их, если у вас нет веских причин.

Одна очень популярная альтернатива - это то, что я называю «объединенная структура» или «объединенный класс». Для вашего примера это на самом деле очень хорошо подходит. Чтобы создать один из них, вы создаете Toolкласс с перечислением, как раньше, но вместо того , чтобы создавать подклассы Tool, мы просто помещаем в него все поля из каждого подтипа.

class Tool {
    public:
        enum TypeE {
            kSword,
            kShield,
            kMagicCloth
        };
    TypeE type;

    int   attack;
    int   defense;
};

Теперь вам не нужны подклассы вообще. Вам просто нужно посмотреть на typeполе, чтобы увидеть, какие другие поля действительно действительны. Это намного безопаснее и проще для понимания. Однако у него есть недостатки. Есть моменты, когда вы не хотите использовать это:

  • Когда объекты слишком разные - вы можете получить список полей, и может быть неясно, какие из них применимы к каждому типу объектов.
  • При работе в критической ситуации с памятью - если вам нужно сделать 10 инструментов, вы можете лениться с памятью. Когда вам нужно сделать 500 миллионов инструментов, вы начнете заботиться о битах и ​​байтах. Профсоюзные структуры всегда больше, чем они должны быть.

Это решение не используется сокетами UNIX из-за различий, усугубляемых открытостью API. Цель сокетов UNIX состояла в том, чтобы создать что-то, с чем мог бы работать любой вариант UNIX. Каждый вариант может определять список поддерживаемых семейств, например AF_INET, и для каждого будет краткий список. Однако, если появляется новый протокол, как, например, AF_INET6сделал, вам может понадобиться добавить новые поля. Если бы вы сделали это с объединенной структурой, вы бы в итоге эффективно создали новую версию структуры с тем же именем, создавая бесконечные проблемы несовместимости. Вот почему сокеты UNIX решили использовать шаблон приведения, а не структуру объединения. Я уверен, что они рассмотрели это, и факт, что они думали об этом, является частью того, почему это не пахнет, когда они используют это.

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

Еще одно интересное решение boost::variant. Boost - это отличная библиотека, полная многоразовых кроссплатформенных решений. Это, вероятно, один из лучших кодов C ++, когда-либо написанных. Boost.Variant - это, в основном, версия союзов на C ++. Это контейнер, который может содержать много разных типов, но только по одному за раз. Вы можете сделать свои Sword, Shieldи MagicClothклассы, а затем сделать инструмент совсем немного!), Но этот шаблон может быть невероятно полезным. Вариант часто используется, например, в деревьях разбора, которые берут строку текста и разбивают ее, используя грамматику для правил.boost::variant<Sword, Shield, MagicCloth> , то есть он содержит один из этих трех типов. Это все еще страдает от той же самой проблемы с будущей совместимостью, которая препятствует тому, чтобы сокеты UNIX использовали это (не говоря уже о сокетах UNIX, C, предшествующийboost

Окончательное решение, которое я бы посоветовал рассмотреть, прежде чем окунуться и использовать универсальный подход приведения объектов, - это шаблон проектирования Visitor . Visitor - это мощный шаблон проектирования, использующий преимущество наблюдения, заключающегося в том, что вызов виртуальной функции эффективно выполняет кастинг, который вам нужен, и делает это за вас. Поскольку компилятор делает это, он никогда не может ошибаться. Таким образом, вместо хранения перечисления Visitor использует абстрактный базовый класс, который имеет виртуальную таблицу, которая знает, какой тип объекта. Затем мы создаем аккуратный маленький вызов двойного косвенного действия, который выполняет работу:

class Tool;
class Sword;
class Shield;
class MagicCloth;

class ToolVisitor {
public:
    virtual void visit(Sword* sword) = 0;
    virtual void visit(Shield* shield) = 0;
    virtual void visit(MagicCloth* cloth) = 0;
};

class Tool {
public:
    virtual void accept(ToolVisitor& visitor) = 0;
};

lass Sword : public Tool{
public:
    virtual void accept(ToolVisitor& visitor) { visitor.visit(*this); }
    int attack;
};
class Shield : public Tool{
public:
    virtual void accept(ToolVisitor& visitor) { visitor.visit(*this); }
    int defense;
};
class MagicCloth : public Tool{
public:
    virtual void accept(ToolVisitor& visitor) { visitor.visit(*this); }
    int attack;
    int defense;
};

Так что же это за ужасная картина? Ну, Toolесть виртуальная функция accept. Если вы передадите ему посетителя, ожидается, что он развернется и вызовет правильную visitфункцию для этого посетителя для типа. Это то, что visitor.visit(*this);делает для каждого подтипа. Сложно, но мы можем показать это на вашем примере выше:

class AttackVisitor : public ToolVisitor
{
public:
    int& currentAttack;
    int& currentDefense;

    AttackVisitor(int& currentAttack_, int& currentDefense_)
    : currentAttack(currentAttack_)
    , currentDefense(currentDefense_)
    { }

    virtual void visit(Sword* sword)
    {
        currentAttack += sword->attack;
    }

    virtual void visit(Shield* shield)
    {
        currentDefense += shield->defense;
    }

    virtual void visit(MagicCloth* cloth)
    {
        currentAttack += cloth->attack;
        currentDefense += cloth->defense;
    }
};

void Player::attack()
{
    int currentAttack = this->attack;
    int currentDefense = this->defense;
    AttackVisitor v(currentAttack, currentDefense);
    for (Tool* t: tools) {
        t->accept(v);
    }
    //some other functions to start attack
}

Так что здесь происходит? Мы создаем посетителя, который будет выполнять некоторую работу за нас, как только он узнает, какой тип объекта он посещает. Затем мы перебираем список инструментов. Ради аргумента, скажем, первый объект - это a Shield, но наш код этого еще не знает. Это звонки t->accept(v), виртуальная функция. Поскольку первый объект - это щит, он в конечном итоге вызывает void Shield::accept(ToolVisitor& visitor), что вызывает visitor.visit(*this);. Теперь, когда мы ищем, что visitвызывать, мы уже знаем, что у нас есть Shield (потому что эта функция была вызвана), поэтому мы в конечном итоге вызовем void ToolVisitor::visit(Shield* shield)нашу AttackVisitor. Теперь выполняется правильный код для обновления нашей защиты.

Посетитель громоздкий. Это настолько неуклюже, что я почти думаю, что у него есть собственный запах. Писать шаблоны плохих посетителей очень легко. Однако у него есть одно огромное преимущество, которого нет у других. Если мы добавим новый тип инструмента, мы должны добавить новую ToolVisitor::visitфункцию для него. В тот момент, когда мы делаем это, каждый ToolVisitor в программе откажется компилировать, потому что отсутствует виртуальная функция. Это позволяет очень легко поймать все случаи, когда мы что-то пропустили. Это гораздо сложнее гарантировать, если вы используете ifили switchзаявления, чтобы сделать работу. Эти преимущества достаточно хороши, чтобы Visitor нашел небольшую нишу в генераторах 3D-сцен. Им, скорее всего, нужно именно то поведение, которое предлагает посетитель, поэтому он прекрасно работает!

В общем, помните, что эти шаблоны затрудняют работу следующего разработчика. Потратьте время, чтобы им было легче, и код не пахнет!

* Технически, если вы посмотрите на спецификацию, у sockaddr есть один член sa_family. Здесь, на уровне Си, делаются некоторые хитрости, которые не имеют значения для нас. Вы можете взглянуть на фактическую реализацию, но для этого ответа я собираюсь использовать sa_family sin_familyи другие полностью взаимозаменяемо, используя тот, который наиболее интуитивно понятен для прозы, полагая, что этот трюк на Си заботится о неважных деталях.

Корт Аммон - Восстановить Монику
источник
Последовательная атака сделает игрока бесконечно сильным в вашем примере. И вы не можете расширить свой подход без изменения источника ToolVisitor. Это отличное решение, хотя.
Полигном
@Polygnome Вы правы по поводу примера. Я думал, что код выглядит странно, но прокручивая все те страницы текста, я пропустил ошибку. Исправляем это сейчас. Что касается требования модификации источника ToolVisitor, то это характерная черта модели Visitor. Это благословение (как я написал) и проклятие (как вы написали). Обрабатывать случай, когда вам нужна произвольно расширяемая версия, гораздо сложнее, и он начинает копаться в смысле переменных, а не только в их значении, и открывает другие шаблоны, такие как слабо типизированные переменные и словари и JSON.
Cort Ammon - Восстановить Монику
1
Да, к сожалению, мы недостаточно знаем о предпочтениях и целях ОП, чтобы принять действительно обоснованное решение. И да, полностью гибкое решение сложнее реализовать, я работал над своим ответом почти 3 часа, так как мой C ++ довольно ржавый :(
Polygnome
0

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

class Tool{
    public:
    //constructor, name etc.
    int GetAttack() { return attack }; //Endpoints for your Player
    int GetDefense() { return defense };
    protected:
         int attack;
         int defense;
};

Возможно, вы ожидаете, что ваша игра будет реализовывать несколько видов мечей и т. Д., Но у вас будут другие способы реализовать это. Класс взрыва редко бывает лучшей архитектурой. Будь проще.

Артур Гавличек
источник
0

Как уже говорилось ранее, это серьезный кодовый запах. Однако можно подумать, что источником вашей проблемы является использование наследования вместо композиции в вашем дизайне.

Например, учитывая то, что вы нам показали, у вас есть три понятия:

  • Предмет
  • Предмет, который может иметь атаку.
  • Предмет, который может иметь защиту.

Обратите внимание, что ваш четвертый класс - это просто сочетание двух последних понятий. Поэтому я бы предложил использовать композицию для этого.

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

class Attack
{
private:
  int attack_;

public:
  int AttackValue() const;
};

class Defense
{
private:
  int defense_

public:
  int DefenseValue() const;
};

class Tool
{
private:
  std::optional<Attack> atk_;
  std::optional<Defense> def_;

public:
  const std::optional<Attack> &GetAttack() const {return atk_;}
  const std::optional<Defense> &GetDefense() const {return def_;}
};
Николь Болас
источник
Также: не используйте композитный подход всегда :)! Зачем использовать композицию в этом случае? Я согласен, что это альтернативное решение, но создание класса для «инкапсуляции» поля (обратите внимание на «») кажется странным в этом случае ...
AilurusFulgens
@AilurusFulgens: сегодня это «поле». Что будет завтра? Такая конструкция позволяет Attackи Defenseусложняться без изменения интерфейса Tool.
Никол Болас
1
Вы все еще не можете расширить инструмент с этим - конечно, атака и защита могут быть более сложными, но это все. Если вы используете композицию на полную мощность, вы можете сделать ее Toolполностью закрытой для модификации, но при этом оставить ее открытой для расширения.
Полигном
@Polygnome: Если вы хотите решить проблему создания целой системы произвольных компонентов для такого тривиального случая, как этот, это ваше дело. Я лично не вижу причин, по которым я хотел бы расширить, Toolне изменяя его. И если у меня есть право изменить его, то я не вижу необходимости в произвольных компонентах.
Николь Болас
Пока Инструмент находится под вашим собственным контролем, вы можете изменять его. Но принцип «закрыто для модификации, открыт для расширения» существует по уважительной причине (слишком долго, чтобы уточнить здесь). Я не думаю, что это так банально. Если вы потратите надлежащее время на планирование гибкой системы компонентов для RPG, вы получите огромное вознаграждение в долгосрочной перспективе. Я не вижу дополнительного преимущества в этом типе композиции по сравнению с использованием простых полей. Возможность специализироваться на атаке и защите в дальнейшем представляется очень теоретическим сценарием. Но, как я уже писал, это зависит от точных требований ОП.
Полигном
0

Почему бы не создавать абстрактные методы modifyAttack и modifyDefenseв Toolклассе? Тогда у каждого ребенка будет своя реализация, и вы называете этот элегантный способ:

for(Tool* tool : tools){
    currentAttack = tool->recalculateAttack(currentAttack);
    currentDefense = tool->recalculateDefense(currentDefense);
}
// proceed with new values for currentAttack and currentDefense

Передача значений в качестве ссылки сэкономит ресурсы, если вы сможете:

for(Tool* tool : tools){
    tool->recalculateAttack(&currentAttack);
    tool->recalculateDefense(&currentDefense);
}
// proceed with new values for currentAttack and currentDefense
Пауло Амарал
источник
0

Если кто-то использует полиморфизм, то всегда лучше, если весь код, который заботится о том, какой класс используется, находится внутри самого класса. Вот как я мог бы написать это:

class Tool{
 public:
   virtual void equipTo(Player* player) =0;
   virtual void unequipFrom(Player* player) =0;
};

class Sword : public Tool{
  public:
    int attack;
    virtual void equipTo(Player* player) {
      player->attackBonus+=this->attack;
    };
    //unequipFrom = reverse equip
};
class Shield : public Tool{
  public:
    int defense;
    virtual void equipTo(Player* player) {
      player->defenseBonus+=this->defense;
    };
    //unequipFrom = reverse equip
};
//other tools
class Player{
  public:
    int baseAttack;
    int baseDefense;
    int attackBonus;
    int defenseBonus;

    virtual void equip(Tool* tool) {
      tool->equipTo(this);
      this->tools.push_back(tool)
    };

    //unequip = reverse equip

    void attack(){
      //modified attack and defense
      int modifiedAttack = baseAttack + this->attackBonus;
      int modifiedDefense = baseDefense+ this->defenseBonus;
      //some other functions to start attack
    }
  private:
    vector<Tool*> tools;
};

Это имеет следующие преимущества:

  • проще добавлять новые классы: вам нужно только реализовать все абстрактные методы, а остальная часть кода просто работает
  • легче удалить классы
  • проще добавлять новую статистику (классы, которые не заботятся о статистике, просто игнорируют ее)
Siphor
источник
Вы должны по крайней мере также включить метод unequip (), который удаляет бонус у игрока.
Полигном
0

Я думаю, что один из способов признать недостатки этого подхода - развить вашу идею до ее логического завершения.

Это похоже на игру, поэтому на каком-то этапе вы, вероятно, начнете беспокоиться о производительности и поменяете местами сравнения строк на intили enum. По мере того, как список предметов становится длиннее, он if-elseстановится довольно громоздким, поэтому вы можете рассмотреть возможность его реорганизации вswitch-case . На этом этапе у вас также есть достаточно текста, так что вы можете включить действие внутри каждого caseв отдельную функцию.

Как только вы достигнете этой точки, структура вашего кода начинает выглядеть знакомой - она ​​начинает выглядеть как доморощенный, свернутый вручную vtable * - базовая структура, на которой обычно реализуются виртуальные методы. Кроме того, это vtable, который вы должны вручную обновлять и обслуживать каждый раз, когда добавляете или изменяете тип элемента.

Придерживаясь «реальных» виртуальных функций, вы можете сохранить реализацию поведения каждого элемента в самом элементе. Вы можете добавлять дополнительные элементы более автономным и последовательным способом. И когда вы делаете все это, компилятор будет заботиться о реализации вашей динамической диспетчеризации, а не вы.

Для решения вашей конкретной проблемы: вы изо всех сил пытаетесь написать простую пару виртуальных функций для обновления атаки и защиты, потому что некоторые элементы влияют только на атаку, а некоторые - только на защиту. Хитрость в простом случае, подобном этому, заключается в том, чтобы реализовать оба поведения в любом случае, но без эффекта в определенных случаях. GetDefenseBonus()может вернуться 0или ApplyDefenseBonus(int& defence)может просто уйтиdefence без изменений. То, как вы поступите, будет зависеть от того, как вы хотите справиться с другими действиями, которые действительно оказывают влияние. В более сложных случаях, когда существует более разнообразное поведение, вы можете просто объединить действие в один метод.

* (Хотя и транспонированный относительно типичной реализации)

DeveloperInDevelopment
источник
0

Наличие блока кода, который знает обо всех возможных «инструментах», не очень хороший дизайн (тем более, что в вашем коде будет много таких блоков); но ни у одного из них нет базовых Toolзаглушек для всех возможных свойств инструмента: теперь Toolкласс должен знать обо всех возможных применениях.

То, что знает каждый инструмент, это то, что он может внести в персонажа, который его использует. Так что предоставьте один метод для всех инструментов giveto(*Character owner). Он будет корректировать статистику игрока соответствующим образом, не зная, что могут делать другие инструменты, и, что лучше, ему также не нужно знать о нерелевантных свойствах персонажа. Например, щит не нужно даже знать об атрибутах attack, invisibility, и healthт.д. Все , что нужно применять инструмент для персонажа , чтобы поддержать атрибуты , что объект требует. Если вы попытаетесь дать меч ослу, а у осла нет attackхарактеристик, вы получите ошибку.

У инструментов также должен быть remove()метод, который обращает их влияние на владельца. Это немного сложно (возможно, в конечном итоге с инструментами, которые оставляют ненулевой эффект, когда они даны, а затем убраны), но, по крайней мере, он локализован для каждого инструмента.

Alexis
источник
-4

Там нет ответа, который говорит, что это не пахнет, поэтому я буду тем, кто поддерживает это мнение; этот код полностью в порядке! Мое мнение основано на том факте, что иногда легче двигаться дальше и постепенно увеличивать свои навыки по мере создания новых вещей. Вы можете на несколько дней застрять в создании идеальной архитектуры, но, вероятно, никто даже не увидит ее в действии, потому что вы никогда не заканчивали проект. Ура!

Ostmeistro
источник
4
Улучшение навыков с личным опытом это хорошо, конечно. Но умение совершенствовать, спрашивая людей, у которых уже есть этот личный опыт, чтобы вам не нужно было проваливаться в дыру, умнее. Конечно, именно поэтому люди задают вопросы здесь, в первую очередь, не так ли?
Грэм
Я не согласна Но я понимаю, что этот сайт посвящен углублению. Иногда это означает быть чрезмерно педантичным. Вот почему я хотел опубликовать это мнение, потому что оно основано на реальности, и если вы ищете советы, чтобы стать лучше и помочь новичку, вы пропустите всю эту главу о «достаточно хорошо», которая довольно полезна для новичка.
Остмеистро