Est-ce une odeur de code de stocker des objets génériques dans un conteneur, puis d’obtenir un objet et de décaler les objets depuis un conteneur?

34

Par exemple, j'ai un jeu qui a quelques outils pour augmenter la capacité du joueur:

Tool.h

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

Et quelques outils:

Épée.h

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

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

Et puis un joueur peut avoir quelques outils pour attaquer:

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

Je pense qu'il est difficile de remplacer if-else les méthodes virtuelles dans les outils, car chaque outil a des propriétés différentes, et chaque outil affecte l'attaque et la défense du joueur, pour lesquelles la mise à jour de l'attaque et de la défense du joueur doit être effectuée à l'intérieur de l'objet Player.

Mais je n’étais pas satisfait de cette conception, car elle contient un affaiblissement, avec une longue if-elsedéclaration. Cette conception doit-elle être "corrigée"? Si oui, que puis-je faire pour le corriger?

ggrr
la source
4
Une technique standard de programmation orientée objet permettant de supprimer des tests pour une sous-classe spécifique (et les retransmissions ultérieures) consiste à créer une ou deux méthodes virtuelles dans la classe de base à utiliser à la place de la chaîne if et des conversions. Cela peut être utilisé pour supprimer complètement les if et déléguer l'opération aux sous-classes à implémenter. Vous n'aurez pas non plus besoin de modifier les instructions if à chaque fois que vous ajoutez une nouvelle sous-classe.
Erik Eidt
2
Considérez également Double Dispatch.
Boris l'araignée
Pourquoi ne pas ajouter une propriété à votre classe Tool contenant un dictionnaire de types d'attributs (c.-à-d. Attaque, défense) et une valeur qui leur est assignée. L'attaque, la défense pourrait être des valeurs énumérées. Ensuite, vous pouvez simplement appeler la valeur de l'outil lui-même par la constante énumérée.
user1740075
8
Je vais juste laisser ça ici: ericlippert.com/2015/04/27/wizards-and-warriors-part-one
Vous
1
Voir aussi le modèle Visiteur.
JDługosz le

Réponses:

63

Oui, c'est une odeur de code (dans beaucoup de cas).

Je pense qu'il est difficile de remplacer if-else avec des méthodes virtuelles dans les outils

Dans votre exemple, il est assez simple de remplacer les méthodes if / else par des méthodes virtuelles:

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

Maintenant, votre ifbloc n'est plus nécessaire , l'appelant peut simplement l'utiliser comme

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

Bien sûr, pour des situations plus compliquées, une telle solution n’est pas toujours aussi évidente (mais néanmoins presque toujours possible). Mais si vous arrivez dans une situation où vous ne savez pas comment résoudre le cas avec des méthodes virtuelles, vous pouvez poser une nouvelle question ici sur "Programmeurs" (ou, si cela devient spécifique au langage ou à l'implémentation, à Stackoverflow).

Doc Brown
la source
4
ou, d'ailleurs, sur gamedev.stackexchange.com
Kromster dit de soutenir Monica
7
Vous n'auriez même pas besoin du concept de Swordcette manière dans votre base de code. Vous pourriez simplement, new Tool("sword", swordAttack, swordDefense)par exemple, utiliser un fichier JSON.
AmazingDreams
7
@AmazingDreams: c'est exact (pour les parties du code que nous voyons ici), mais je suppose que l'OP a simplifié son vrai code pour que sa question se concentre sur l'aspect qu'il voulait discuter.
Doc Brown
3
Ce n'est pas tellement mieux que le code original (enfin, c'est un peu). Aucun outil ayant des propriétés supplémentaires ne peut être créé sans ajouter de méthodes supplémentaires. Je pense que dans ce cas, il faut privilégier la composition par rapport à l'héritage. Oui, il n'y a actuellement que l'attaque et la défense, mais cela n'a pas besoin de rester comme ça.
Polygnome
1
@ DocBrown Oui, c'est vrai, bien que cela ressemble à un RPG dans lequel un personnage a des statistiques modifiées par des outils ou plutôt des objets équipés. Je créerais un générique Toolavec tous les modificateurs possibles, en remplirais quelques-uns vector<Tool*>avec des éléments lus dans un fichier de données, puis en bouclant dessus et en modifiant les statistiques comme vous le faites maintenant. Vous auriez des ennuis quand vous voudriez qu'un objet donne par exemple un bonus de 10% pour une attaque. Peut-être un tool->modify(playerStats)est une autre option.
AmazingDreams
23

Le principal problème de votre code est que, chaque fois que vous introduisez un nouvel élément, vous devez non seulement écrire et mettre à jour le code de l'élément, vous devez également modifier votre lecteur (ou le lieu où l'élément est utilisé), ce qui en fait beaucoup plus compliqué.

En règle générale, je pense que c'est toujours un peu louche, quand on ne peut pas compter sur des sous-classes / héritages normaux et que l'on doit faire le upcasting soi-même.

Je pourrais penser à deux approches possibles pour rendre le tout plus flexible:

  • Comme d'autres l'ont mentionné, déplacez les membres attacket defensedans la classe de base et initialisez-les simplement 0. Cela pourrait également servir à vérifier si vous êtes réellement en mesure de faire basculer l'objet pour une attaque ou de l'utiliser pour bloquer des attaques.

  • Créer une sorte de système de rappel / événement. Il existe différentes approches possibles pour cela.

    Que diriez-vous de garder les choses simples?

    • Vous pouvez créer des membres de la classe de base tels que virtual void onEquip(Owner*) {}et virtual void onUnequip(Owner*) {}.
    • Leurs surcharges seraient appelées et modifieraient les statistiques lors de (dés) équipement de l'élément, par exemple virtual void onEquip(Owner *o) { o->modifyStat("attack", attackValue); }et virtual void onUnequip(Owner *o) { o->modifyStat("attack", -attackValue); }.
    • Les statistiques peuvent être consultées de manière dynamique, par exemple en utilisant une chaîne courte ou une constante comme clé, de sorte que vous puissiez même introduire de nouvelles valeurs spécifiques à un équipement ou des bonus que vous n'avez pas nécessairement à gérer spécifiquement chez votre joueur ou votre "propriétaire".
    • Comparé au simple fait de demander les valeurs d'attaque / défense juste à temps, cela rend non seulement le tout plus dynamique, mais vous évite également les appels inutiles et vous permet même de créer des objets qui affecteront votre personnage de façon permanente.

      Par exemple, imaginez un anneau maudit qui ne fera que définir une statistique cachée une fois équipé, marquant votre personnage comme étant maudit de manière permanente.

Mario
la source
7

Bien que @DocBrown ait donné une bonne réponse, cela ne va pas assez loin, à mon humble avis. Avant de commencer à évaluer les réponses, vous devez évaluer vos besoins. De quoi avez-vous vraiment besoin ?

Ci-dessous, je montrerai deux solutions possibles offrant des avantages différents pour des besoins différents.

Le premier est très simpliste et adapté spécifiquement à ce que vous avez montré:

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

Cela permet très une sérialisation / désérialisation facile des outils (par exemple, pour la sauvegarde ou la mise en réseau), et n'exige pas du tout de répartition virtuelle. Si votre code est tout ce que vous avez montré, et que vous ne vous attendez pas à ce qu'il évolue beaucoup plus que d'avoir plus d'outils différents avec des noms différents et ces statistiques, mais dans des quantités différentes, alors c'est la voie à suivre.

@DocBrown a proposé une solution qui repose toujours sur la répartition virtuelle, ce qui peut constituer un avantage si vous spécialisez les outils pour des parties de votre code qui ne sont pas affichées. Cependant, si vous avez réellement besoin ou que vous souhaitez également modifier un autre comportement, je vous suggère la solution suivante:

Composition sur héritage

Et si vous voulez plus tard un outil qui modifie l’ agilité ? Ou courir la vitesse ? Pour moi, il semble que vous réalisiez un RPG. Une des choses importantes pour les RPG est d’être ouvert à la prolongation . Les solutions présentées jusqu'à présent ne proposent pas cela. Vous auriez à modifier leTool classe et y ajouter de nouvelles méthodes virtuelles chaque fois que vous avez besoin d'un nouvel attribut.

La deuxième solution que je présente est celle que j'ai évoquée précédemment dans un commentaire: elle utilise la composition au lieu de l'héritage et suit le principe "fermé pour modification, ouvert pour extension *. Si vous connaissez le fonctionnement des systèmes d'entités, certaines choses semblera familier (j'aime penser à la composition en tant que petit frère d'ES).

Notez que ce que je montre ci-dessous est nettement plus élégant dans les langages contenant des informations de type à l'exécution, telles que Java ou C #. Par conséquent, le code C ++ que je présente doit inclure une "comptabilité" qui est simplement nécessaire pour que la composition fonctionne correctement ici. Peut-être que quelqu'un avec plus d'expérience en C ++ est en mesure de suggérer une approche encore meilleure.

D'abord, nous regardons à nouveau du côté de l' appelant . Dans votre exemple, en tant qu'appelant dans la attackméthode, vous vous souciez peu des outils. Ce qui compte pour vous, ce sont deux propriétés: les points d’attaque et de défense. Vous ne vous souciez pas vraiment de la provenance de ceux-ci, ni des autres propriétés (par exemple, la vitesse de course, l'agilité).

Alors d'abord, nous introduisons une nouvelle classe

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

Et puis, nous créons nos deux premiers composants

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

Ensuite, nous faisons en sorte qu'un outil contienne un ensemble de propriétés et que ces propriétés puissent être interrogées par d'autres.

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

Notez que dans cet exemple, je ne supporte qu'un seul composant de chaque type - cela facilite les choses. En théorie, vous pouvez également autoriser plusieurs composants du même type, mais cela devient très rapidement très laid. Un aspect important: il Toolest maintenant fermé pour modification - nous ne toucherons plus jamais la source de Tool- mais ouvert pour extension - nous pouvons étendre le comportement d’un outil en modifiant d’autres éléments, et simplement en y passant d’autres composants.

Nous avons maintenant besoin d'un moyen de récupérer des outils par types de composants. Vous pouvez toujours utiliser un vecteur pour les outils, comme dans votre exemple de code:

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

Vous pouvez également le refactoriser dans votre propre Inventory classe et stocker des tables de consultation qui simplifient grandement l'extraction des outils par type de composant et évitent d'itérer encore et encore l'intégralité de la collection.

Quels avantages a cette approche? Dans attack, vous traitez des outils qui ont deux composants - vous ne vous souciez de rien d'autre.

Imaginons que vous ayez une walkTométhode, et maintenant vous décidez que c’est une bonne idée si un outil acquiert la capacité de modifier votre vitesse de marche. Aucun problème!

Tout d'abord, créez le nouveau Component:

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

Ensuite, vous ajoutez simplement une instance de ce composant à l'outil WalkTopour lequel vous souhaitez augmenter votre vitesse de réveil, puis vous modifiez la méthode pour traiter le composant que vous venez de créer:

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

Notez que nous avons ajouté un comportement à nos outils sans modifier la classe d’outils.

Vous pouvez (et devriez) déplacer les chaînes vers une macro ou une variable const statique, de sorte que vous n'ayez pas à les taper encore et encore.

Si vous allez plus loin dans cette approche, par exemple si vous créez des composants pouvant être ajoutés au joueur et créez un Combatcomposant signalant le joueur comme pouvant participer au combat, vous pouvez également vous débarrasser de la attackméthode et le gérer. par le composant ou être traité ailleurs.

L'avantage de faire en sorte que le joueur puisse obtenir des composants, ce serait aussi que vous n'auriez même pas besoin de changer de joueur pour lui donner un comportement différent. Dans mon exemple, vous pouvez créer un Movablecomposant. Ainsi, vous n'avez pas besoin d'implémenter la walkTométhode sur le lecteur pour le faire bouger. Vous devez simplement créer le composant, l'attacher au lecteur et laisser quelqu'un d'autre le traiter.

Vous pouvez trouver un exemple complet dans cet élément essentiel: https://gist.github.com/NetzwergX/3a29e1b106c6bb9c7308e89dd715ee20

Cette solution est évidemment un peu plus complexe que les autres qui ont été postées. Mais selon votre degré de flexibilité et votre volonté, cette approche peut s'avérer très efficace.

modifier

D'autres réponses proposent un héritage direct (Faire que les épées étendent Outil, faire que Bouclier étende Outil). Je ne pense pas que ce soit un scénario où l'héritage fonctionne très bien. Et si vous décidiez que bloquer avec un bouclier d'une certaine manière peut aussi endommager l'attaquant? Avec ma solution, vous pouvez simplement ajouter un composant Attack à un bouclier et vous en rendre compte sans aucune modification de votre code. Avec l'héritage, vous auriez un problème. Les éléments / outils dans les jeux de rôle sont des candidats de choix pour la composition ou même directement à l'aide de systèmes d'entités depuis le début.

Polygone
la source
1

De manière générale, si vous avez déjà besoin d’utiliser if (en même temps que de spécifier le type d’instance) dans n’importe quelle langue POO, c’est le signe que quelque chose ne va pas. Au moins, vous devriez regarder de plus près vos modèles.

Je modéliserais votre domaine différemment.

Pour votre cas d'utilisation, un Toola AttackBonuset un DefenseBonus- qui pourraient être 0au cas où il serait inutile de se battre comme des plumes ou quelque chose comme ça.

Pour une attaque, vous avez votre baserate+ bonusde l'arme utilisée. Il en va de même pour la défense baserate+ bonus.

En conséquence, vous Tooldevez disposer d'une virtualméthode de calcul du boni d'attaque / défense.

tl; dr

Avec un meilleur design, vous pourriez éviter les hacky if.

Thomas Junk
la source
Parfois, un si est nécessaire, par exemple lors de la comparaison de valeurs scalaires. Pour le changement de type d'objet, pas tellement.
Andy
Haha, si est un opérateur assez essentiel et vous ne pouvez pas simplement dire que l'utilisation est une odeur de code.
tymtam
1
@ Tymski, à un certain égard, vous avez raison. Je me suis fait plus clair. Je ne défends pas ifmoins de programmation. Principalement dans des combinaisons comme si instanceofou quelque chose comme ça. Mais il y a une position, qui prétend ifêtre une odeur de code et il y a des moyens de la contourner. Et vous avez raison, c’est un opérateur essentiel qui a ses propres droits.
Thomas
1

Comme écrit, ça "sent", mais ce ne sont peut-être que des exemples que vous avez donnés. Stocker des données dans des conteneurs d'objets génériques, puis les diffuser pour accéder aux données ne constitue pas automatiquement une odeur de code. Vous verrez qu'il est utilisé dans de nombreuses situations. Cependant, lorsque vous l'utilisez, vous devez savoir ce que vous faites, comment vous le faites et pourquoi. Quand je regarde l'exemple, l'utilisation de comparaisons basées sur des chaînes pour me dire quel objet est quelle est la chose qui déclenche mon odomètre. Cela suggère que vous n'êtes pas tout à fait sûr de ce que vous faites ici (ce qui est bien, car vous avez eu la sagesse de venir ici pour les programmeurs. SE et dire "hé, je ne pense pas que j'aime ce que je fais, aide moi dehors! ").

Le problème fondamental du modèle de diffusion de données à partir de conteneurs génériques, comme celui-ci, est que le producteur et le consommateur de données doivent travailler ensemble, mais il n'est peut-être pas évident qu'ils le fassent à première vue. Dans chaque exemple de ce modèle, malodorant ou non, c'est l'enjeu fondamental. Il est très possible que le prochain développeur ignore complètement que vous suivez ce modèle et le casse par accident. Par conséquent, si vous utilisez ce modèle, vous devez prendre soin d'aider le prochain développeur. Vous devez lui faciliter la tâche pour qu'il ne perde pas le code par inadvertance en raison de détails dont il ne sait peut-être pas qu'ils existaient.

Par exemple, si je voulais copier un lecteur? Si je ne regarde que le contenu de l'objet player, cela semble assez facile. Je viens de copier les attack, defenseet les toolsvariables. C'est de la tarte! Eh bien, je découvrirai rapidement que votre utilisation des pointeurs rend la tâche un peu plus difficile (à un moment donné, cela vaut la peine de regarder les pointeurs intelligents, mais c’est un autre sujet). C'est facilement résolu. Je vais simplement créer de nouvelles copies de chaque outil et les mettre dans ma nouvelle toolsliste. Après tout, Toolc'est un cours très simple avec un seul membre. Donc, je crée un tas de copies, y compris une copie de la Sword, mais je ne savais pas que c’était une épée, je n’ai donc que copié la name. Plus tard, la attack()fonction regarde le nom, voit que c'est une "épée", la lance, et de mauvaises choses arrivent!

Nous pouvons comparer ce cas à un autre cas en programmation de socket, qui utilise le même motif. Je peux configurer une fonction de socket UNIX comme ceci:

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

Pourquoi est-ce le même modèle? Parce que bindn'accepte pas un sockaddr_in*, il accepte un plus générique sockaddr*. Si vous regardez les définitions de ces classes, nous voyons qu’un sockaddrseul membre est la famille à laquelle nous avons attribué sin_family*. La famille dit à quel sous-type vous devez lancer le sockaddr. AF_INETvous dit que l'adresse est une structure sockaddr_in. Si c'était le cas AF_INET6, l'adresse serait a sockaddr_in6, avec des champs plus grands pour prendre en charge les adresses IPv6 plus grandes.

Ceci est identique à votre Toolexemple, sauf qu’il utilise un entier pour spécifier quelle famille plutôt que un std::string. Cependant, je vais prétendre que ça ne sent pas, et essayer de le faire pour des raisons autres que "c'est un moyen standard de faire des sockets, alors ça ne devrait pas" sentir ". pourquoi je prétends que le stockage de données dans des objets génériques et leur transposition ne constituent pas automatiquement une odeur de code, mais il existe quelques différences dans la manière dont ils le font qui le rendent plus sûr.

Lorsque vous utilisez ce modèle, les informations les plus importantes sont la capture des informations sur la sous-classe, du producteur au consommateur. C'est ce que vous faites avec le namechamp et les sockets UNIX avec leur sin_familychamp. Ce champ est l’information dont le consommateur a besoin pour comprendre ce que le producteur a réellement créé. Dans tous les cas de ce modèle, il devrait s'agir d'une énumération (ou à tout le moins, d'un entier agissant comme une énumération). Pourquoi? Pensez à ce que votre consommateur va faire avec l'information. Ils vont avoir besoin d'avoir écrit un grosif déclaration ou unswitchdéclaration, comme vous l'avez fait, où ils déterminent le bon sous-type, le transtypent et utilisent les données. Par définition, il ne peut y avoir qu'un petit nombre de ces types. Vous pouvez le stocker dans une chaîne, comme vous l'avez fait, mais cela présente de nombreux inconvénients:

  • Lent - il faut std::stringgénéralement faire de la mémoire dynamique pour conserver la chaîne. Vous devez également faire une comparaison de texte intégral pour faire correspondre le nom à chaque fois que vous souhaitez déterminer votre sous-classe.
  • Trop polyvalent - Il y a quelque chose à dire pour vous imposer des contraintes lorsque vous faites quelque chose d'extrêmement dangereux. J'ai eu des systèmes comme celui-ci qui cherchaient une sous - chaîne pour lui dire quel type d'objet il regardait. Cela a fonctionné jusqu'à ce que le nom d'un objet contienne accidentellement cette sous-chaîne et crée une erreur terriblement cryptique. Comme, comme nous l’avons indiqué ci-dessus, nous n’avons besoin que d’un petit nombre de cas, il n’ya aucune raison d’utiliser un outil extrêmement puissant comme les chaînes. Cela mène à...
  • Susceptible d'erreurs - Disons simplement que vous voudrez vous lancer dans un déchaînement meurtrier pour essayer de résoudre les problèmes qui surviennent lorsqu’un consommateur donne accidentellement le nom d’un vêtement magique MagicC1oth. Sérieusement, des insectes comme ceux-là peuvent prendre des jours avant que vous ne réalisiez ce qui s'est passé.

Une énumération fonctionne beaucoup mieux. C'est rapide, pas cher et beaucoup moins sujet aux erreurs:

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

Cet exemple montre également une switchdéclaration impliquant les enums, avec la partie la plus importante de ce modèle: un defaultcas qui se déclenche. Vous ne devriez jamais vous retrouver dans cette situation si vous faites les choses parfaitement. Toutefois, si quelqu'un ajoute un nouveau type d'outil et que vous oubliez de mettre à jour votre code pour le prendre en charge, vous souhaiterez que quelque chose corrige l'erreur. En fait, je les recommande tellement que vous devriez les ajouter même si vous n'en avez pas besoin.

L’autre grand avantage de ce logiciel enumréside dans le fait qu’il offre immédiatement au développeur suivant une liste complète des types d’outils valides. Il n'est pas nécessaire de parcourir le code pour trouver la classe de flûte spécialisée de Bob qu'il utilise dans son combat épique contre le boss.

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

Oui, j'ai inséré une instruction par défaut "vide", simplement pour que le prochain développeur sache clairement ce que j'attend si de nouveaux types inattendus se présentent à moi.

Si vous faites cela, le motif sentira moins. Toutefois, pour éliminer les odeurs, la dernière chose à faire est d’envisager les autres options. Ces moulages font partie des outils les plus puissants et les plus dangereux du répertoire C ++. Vous ne devriez pas les utiliser sauf si vous avez une bonne raison.

Une alternative très populaire est ce que j'appelle une "structure syndicale" ou "classe syndicale". Pour votre exemple, ce serait en fait un très bon ajustement. Pour en créer un, vous créez une Toolclasse, avec une énumération comme avant, mais au lieu de sous Tool- classer , nous mettons simplement tous les champs de chaque sous-type.

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

    int   attack;
    int   defense;
};

Maintenant, vous n'avez plus besoin de sous-classes. Il suffit de regarder le typechamp pour voir quels autres champs sont réellement valides. C'est beaucoup plus sûr et plus facile à comprendre. Cependant, il présente des inconvénients. Il y a des moments où vous ne voulez pas utiliser ceci:

  • Lorsque les objets sont trop différents, vous pouvez vous retrouver avec une liste exhaustive de champs, et il est difficile de savoir lesquels s'appliquent à chaque type d'objet.
  • Lorsque vous travaillez dans une situation critique en mémoire - Si vous devez créer 10 outils, vous pouvez être paresseux avec la mémoire. Lorsque vous devez créer 500 millions d'outils, vous allez commencer à vous soucier des bits et des octets. Les structures syndicales sont toujours plus grandes que nécessaire.

Cette solution n'est pas utilisée par les sockets UNIX en raison du problème de dissimilarité aggravé par le caractère ouvert de l'API. L'intention des sockets UNIX était de créer quelque chose avec lequel chaque version d'UNIX pourrait fonctionner. Chaque type pourrait définir la liste des familles qu’il soutient, par exemple AF_INET, et il y aurait une courte liste pour chacun. Toutefois, si un nouveau protocole se présente AF_INET6, vous devrez peut-être ajouter de nouveaux champs. Si vous faisiez cela avec une structure d'union, vous créeriez effectivement une nouvelle version de la structure avec le même nom, créant ainsi des problèmes d'incompatibilité sans fin. C'est pourquoi les sockets UNIX ont choisi d'utiliser le modèle de casting plutôt qu'une structure d'union. Je suis sûr qu'ils y ont pensé et le fait d'y avoir pensé explique en partie pourquoi ça ne sent pas quand ils l'utilisent.

Vous pouvez également utiliser un syndicat pour de vrai. Les syndicats économisent de la mémoire en étant aussi gros que le membre le plus important, mais ils ont leurs propres problèmes. Ce n'est probablement pas une option pour votre code, mais c'est toujours une option à considérer.

Une autre solution intéressante est boost::variant. Boost est une excellente bibliothèque contenant de nombreuses solutions multiplates-formes réutilisables. C'est probablement l'un des meilleurs codes C ++ jamais écrits. Boost.Variant est essentiellement la version C ++ des unions. C'est un conteneur qui peut contenir plusieurs types différents, mais un seul à la fois. Vous pouvez créer votre Sword, Shieldet vos MagicClothclasses, puis transformer l'outil en outil boost::variant<Sword, Shield, MagicCloth>, ce qui signifie qu'il contient l'un de ces trois types. Ceci a toujours le même problème avec la compatibilité future qui empêche les sockets UNIX de l’utiliser (sans parler des sockets UNIX en C,boostun peu!), mais ce modèle peut être incroyablement utile. La variante est souvent utilisée, par exemple, dans les arborescences d’analyse syntaxique, qui prennent une chaîne de texte et la divisent en utilisant une grammaire pour les règles.

La solution finale que je vous conseillerais de regarder avant de plonger et d'utiliser l'approche générique de casting d'objet est le modèle de conception Visitor . Visiteur est un modèle de conception puissant qui tire parti de l'observation selon laquelle l'appel d'une fonction virtuelle effectue effectivement le casting dont vous avez besoin, et il le fait pour vous. Parce que le compilateur le fait, cela ne peut jamais être faux. Ainsi, au lieu de stocker une énumération, Visitor utilise une classe de base abstraite, qui a une table virtuelle qui sait quel type d'objet est. Nous créons ensuite un joli petit appel double-indirection qui fait le travail:

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

Alors, quel est ce modèle dieu aweful? Eh bien, Toola une fonction virtuelle, accept. Si vous transmettez un visiteur, il est censé se retourner et appeler la visitfonction correcte sur ce visiteur pour le type. C'est ce que l'on visitor.visit(*this);fait sur chaque sous-type. Compliqué, mais nous pouvons le montrer avec votre exemple ci-dessus:

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
}

Alors qu'est-ce qui se passe ici? Nous créons un visiteur qui travaillera pour nous une fois qu'il aura identifié le type d'objet visité. Nous parcourons ensuite la liste des outils. Par argument, supposons que le premier objet soit un Shield, mais notre code ne le sait pas encore. Cela appelle t->accept(v)une fonction virtuelle. Comme le premier objet est un bouclier, il finit par appeler void Shield::accept(ToolVisitor& visitor), ce qui appelle visitor.visit(*this);. Maintenant, quand nous cherchons où visitappeler, nous savons déjà que nous avons un bouclier (parce que cette fonction a été appelée), donc nous finirons par appeler void ToolVisitor::visit(Shield* shield)notre AttackVisitor. Ceci exécute maintenant le code correct pour mettre à jour notre défense.

Le visiteur est volumineux. C'est tellement maladroit que je pense presque qu'il a une odeur qui lui est propre. Il est très facile d'écrire de mauvais modèles de visiteurs. Cependant, il possède un énorme avantage qu'aucun des autres n'a. Si nous ajoutons un nouveau type d’outil, nous devons lui ajouter une nouvelle ToolVisitor::visitfonction. Dès que nous faisons cela, chaque ToolVisitor programme refusera de compiler car il manque une fonction virtuelle. Cela rend très facile d'attraper tous les cas où nous avons manqué quelque chose. Il est beaucoup plus difficile de garantir que si vous utilisez ifou des switchdéclarations pour faire le travail. Ces avantages sont suffisants pour que Visit ait trouvé une belle petite niche dans les générateurs de scènes graphiques 3D. Ils ont besoin du type de comportement que propose Visiteur pour que cela fonctionne bien!

En tout, rappelez-vous que ces schémas rendent la tâche difficile au prochain développeur. Passez du temps à leur faciliter la tâche et le code ne sentira pas!

* Techniquement, si vous regardez les spécifications, sockaddr a un membre nommé sa_family. Au niveau C, nous faisons quelque chose de délicat qui importe peu pour nous. Vous pouvez regarder l’ implémentation réelle , mais pour cette réponse, je vais sa_family sin_familyutiliser celle qui est la plus intuitive pour la prose, de manière totalement interchangeable, en sachant que cette supercherie C s’occupe des détails sans importance.

Cort Ammon - Rétablir Monica
la source
Attaquer consécutivement rend le joueur infiniment fort dans votre exemple. Et vous ne pouvez pas étendre votre approche sans modifier la source de ToolVisitor. C'est une excellente solution, cependant.
Polygnome
@ Polygnome Vous avez raison à propos de l'exemple. Je pensais que le code avait l'air bizarre, mais en ratant toutes ces pages de texte, j'ai raté l'erreur. Le réparer maintenant. En ce qui concerne l'exigence de modification de la source de ToolVisitor, il s'agit d'une caractéristique de conception du modèle de visiteur. C'est une bénédiction (comme j'ai écrit) et une malédiction (comme vous l'avez écrit). Il est beaucoup plus difficile de gérer le cas dans lequel vous souhaitez une version extensible de manière arbitraire. Il commence à creuser en fonction de la signification des variables, plutôt que simplement de leur valeur, et ouvre d'autres modèles, tels que les variables faiblement typées, les dictionnaires et JSON.
Cort Ammon - Rétablir Monica
1
Oui, malheureusement, nous ne connaissons pas assez les préférences et les objectifs des PO pour prendre une décision vraiment éclairée. Et oui, une solution totalement flexible est plus difficile à mettre en œuvre, j'ai travaillé sur ma réponse pendant près de 3h, car mon C ++ est plutôt rouillé :(
Polygnome
0

En général, j'évite d'implémenter plusieurs classes / hériter si ce n'est que pour communiquer des données. Vous pouvez vous en tenir à une seule classe et tout implémenter à partir de là. Pour votre exemple, cela suffit

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

Vous pensez probablement que votre jeu implémentera plusieurs types d’épées, etc., mais vous aurez d’autres moyens de le faire. L'explosion de classe est rarement la meilleure architecture. Rester simple.

Arthur Havlicek
la source
0

Comme indiqué précédemment, ceci est une odeur de code sérieuse. Cependant, on pourrait considérer que la source de votre problème utilise l'héritage au lieu de la composition dans votre conception.

Par exemple, compte tenu de ce que vous nous avez montré, vous avez clairement 3 concepts:

  • Article
  • Objet pouvant avoir une attaque.
  • Objet pouvant avoir une défense.

Notez que votre quatrième cours est juste une combinaison des deux derniers concepts. Je suggère donc d'utiliser la composition pour cela.

Vous avez besoin d'une structure de données pour représenter les informations nécessaires à l'attaque. Et vous avez besoin d'une structure de données représentant les informations dont vous avez besoin pour la défense. Enfin, vous avez besoin d’une structure de données pour représenter des éléments pouvant avoir ou non l’une ou les deux propriétés suivantes:

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_;}
};
Nicol Bolas
la source
Aussi: n'utilisez pas une approche composée-toujours :)! Pourquoi utiliser la composition dans ce cas? Je conviens que c'est une solution alternative, mais créer une classe pour "encapsuler" un champ (remarquez le "") semble bizarre dans ce cas ...
AilurusFulgens
@AilurusFulgens: C'est "un champ" aujourd'hui. Que sera-ce demain? Cette conception permet Attacket Defensedevient plus compliquée sans changer l'interface de Tool.
Nicol Bolas
1
Vous ne pouvez toujours pas étendre très bien Tool avec ceci - bien sûr, l’attaque et la défense peuvent devenir plus complexes, mais c’est tout. Si vous utilisez la composition au maximum de sa puissance, vous pouvez effectuer Toolune fermeture complète pour modification tout en la laissant ouverte pour une extension.
Polygnome
@Polygnome: Si vous souhaitez créer un système de composants arbitraire complet pour un cas aussi trivial que celui-ci, c'est à vous de décider. Personnellement, je ne vois aucune raison pour laquelle je voudrais prolonger Toolsans modifier . Et si j'ai le droit de le modifier, je ne vois pas la nécessité de composants arbitraires.
Nicol Bolas
Tant que Tool est sous votre propre contrôle, vous pouvez le modifier. Mais le principe "fermé pour modification, ouvert pour extension" existe pour une bonne raison (trop long à élaborer ici). Je ne pense pas que ce soit si trivial, cependant. Si vous passez suffisamment de temps à planifier un système de composants flexible pour un RPG, vous obtiendrez des avantages considérables à long terme. Je ne vois pas l'avantage supplémentaire de ce type de composition par rapport à l'utilisation de champs ordinaires. Pouvoir spécialiser attaque et défense par la suite semble être un scénario très théorique. Mais comme je l'ai écrit, cela dépend des exigences exactes du PO.
Polygnome
0

Pourquoi ne pas créer des méthodes abstraites modifyAttacket modifyDefenseen Toolclasse? Ensuite, chaque enfant aurait sa propre implémentation, et vous appelez cette manière élégante:

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

Passer des valeurs comme référence économisera des ressources si vous pouvez:

for(Tool* tool : tools){
    tool->recalculateAttack(&currentAttack);
    tool->recalculateDefense(&currentDefense);
}
// proceed with new values for currentAttack and currentDefense
Paulo Amaral
la source
0

Si vous utilisez le polymorphisme, il est toujours préférable que tout le code qui se préoccupe de la classe utilisée soit à l'intérieur de la classe elle-même. Voici comment je le coderais:

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

Cela présente les avantages suivants:

  • plus facile d'ajouter de nouvelles classes: il suffit d'implémenter toutes les méthodes abstraites et le reste du code fonctionne
  • plus facile d'enlever des classes
  • plus facile d'ajouter de nouvelles statistiques (les classes qui ne se soucient pas de la statistique ne l'ignorent pas)
Siphor
la source
Vous devriez au moins également inclure une méthode unequip () qui supprime le bonus du joueur.
Polygnome
0

Je pense qu’une façon de reconnaître les défauts de cette approche est de développer votre idée jusqu’à sa conclusion logique.

Cela ressemble à un jeu. Par conséquent, à un moment donné, vous commencerez probablement à vous soucier de la performance et à échanger ces comparaisons de chaînes contre un intou enum. Au fur et à mesure que la liste d'éléments s'allonge, la tâche devient de plus en plus if-elsecompliquée, vous pouvez donc envisager de la refactoriser dans un fichier switch-case. Vous avez également tout un mur de texte à ce stade afin que vous puissiez vider l'action au sein de chaquecase dans une fonction distincte.

Une fois que vous avez atteint ce point, la structure de votre code commence à sembler familière - elle commence à ressembler à un homebrew vtable * roulé à la main - la structure de base sur laquelle les méthodes virtuelles sont généralement implémentées. Sauf qu'il s'agit d'une table virtuelle que vous devez mettre à jour et gérer vous-même manuellement, chaque fois que vous ajoutez ou modifiez un type d'élément.

En vous en tenant à de "vraies" fonctions virtuelles, vous êtes en mesure de conserver la mise en œuvre du comportement de chaque élément dans l'élément lui-même. Vous pouvez ajouter des éléments supplémentaires de manière plus cohérente et autonome. Et pendant que vous faites tout cela, c'est le compilateur qui s'occupera de l'implémentation de votre dispatch dynamique, plutôt que vous.

Pour résoudre votre problème spécifique: vous avez du mal à écrire une simple paire de fonctions virtuelles pour mettre à jour l'attaque et la défense, car certains éléments n'affectent que l'attaque et d'autres seulement la défense. Le truc dans un cas aussi simple que celui-ci consiste à implémenter les deux comportements de toute façon, mais sans effet dans certains cas. GetDefenseBonus()pourrait revenir 0ou ApplyDefenseBonus(int& defence)pourrait simplement rester defenceinchangé. Cela dépend de la façon dont vous voulez gérer les autres actions qui ont un effet. Dans les cas plus complexes, où le comportement est plus varié, vous pouvez simplement combiner l'activité en une seule méthode.

* (Bien que transposé par rapport à la mise en œuvre typique)

DeveloperInDevelopment
la source
0

Avoir un bloc de code qui connaît tous les "outils" possibles n’est pas un bon design (d’autant plus que vous finirez par avoir beaucoup de ces blocs dans votre code); mais ce n'est pas non plus avoir une base Toolavec des moignons pour toutes les propriétés d'outil possibles: la Toolclasse doit maintenant connaître toutes les utilisations possibles.

Ce que chaque outil sait, c'est ce qu’il peut apporter au personnage qui l’utilise. Donc , fournir une méthode pour tous les outils, giveto(*Character owner). Il ajustera les statistiques du joueur comme il convient sans savoir ce que les autres outils peuvent faire, et ce qui est préférable, il n'aura pas non plus besoin de connaître les propriétés non pertinentes du personnage. Par exemple, un bouclier n'a même pas besoin de connaître les attributs attack, invisibility, healthetc. Tout ce qui est nécessaire d'appliquer un outil est le caractère pour soutenir les attributs que l'objet a besoin. Si vous essayez de donner une épée à un âne et que celui-ci n'a aucune attackstatistique, vous obtiendrez une erreur.

Les outils doivent également avoir une remove()méthode qui inverse leur effet sur le propriétaire. C'est un peu délicat (il est possible de se retrouver avec des outils laissant un effet non nul lorsqu'ils sont donnés puis enlevés), mais au moins c'est localisé pour chaque outil.

alexis
la source
-4

Il n'y a pas de réponse qui dit que ça ne sent pas, alors je serai celui qui défend cet avis; ce code est totalement bon! Mon opinion est basée sur le fait qu’il est parfois plus facile de progresser et de laisser vos compétences augmenter progressivement à mesure que vous créez de nouvelles choses. Vous pouvez rester bloqué pendant des jours pour réaliser une architecture parfaite, mais personne ne la verra même jamais en action, car vous n'avez jamais terminé le projet. À votre santé!

Ostmeistro
la source
4
Améliorer les compétences avec l'expérience personnelle est bon, bien sûr. Mais améliorer les compétences en demandant aux personnes qui ont déjà cette expérience personnelle, pour ne pas avoir à tomber dans le trou, est plus intelligent. C'est sûrement la raison pour laquelle les gens posent des questions ici en premier lieu, n'est-ce pas?
Graham
Je ne suis pas d'accord Mais je comprends que ce site a tout pour aller en profondeur. Parfois, cela signifie être trop pédant. C'est pourquoi j'ai voulu publier cet avis, car il est ancré dans la réalité et si vous recherchez des astuces pour devenir meilleur et aider un débutant, vous manquez tout ce chapitre sur "assez bon" qui est assez utile pour un débutant.
Ostmeistro