34

For example, I have a game, which has some tools to increase the ability of the Player:

Tool.h

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

And some tools:

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

And then a player may hold some tools for attack:

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

I think it is difficult to replace if-else with virtual methods in the tools, because each tool has different properties, and each tool affects the player's attack and defense, for which the update of player attack and defense needs to be done inside the Player object.

But I was not satisfied with this design, since it contains downcasting, with a long if-else statement. Does this design need to be "corrected"? If so, what can I do to correct it?

  • 4
    A standard OOP technique to remove tests for a specific subclass (and the subsequent downcasts) is to create a, or in this case maybe two, virtual method(s) in the base class to use instead of the if chain and casts. This can be used remove the if's altogether and delegate the operation to the subclasses to implement. You also won't have to edit the if statements every time you add a new subclass. – Erik Eidt Jun 10 '16 at 4:55
  • 2
    Also consider Double Dispatch. – Boris the Spider Jun 10 '16 at 8:49
  • Why not add a property to your Tool class that holds a dictionary of attribute types (i.e. attack, defense) and a value assigned to it. The attack, defense could be enumerated values. Then you can just call the value from the Tool itself by the enumerated constant. – user1740075 Jun 10 '16 at 17:32
  • 8
    I'll just leave this here: ericlippert.com/2015/04/27/wizards-and-warriors-part-one – You Jun 10 '16 at 19:59
  • 1
    Also see the Visitor pattern. – JDługosz Jun 11 '16 at 14:04

12 Answers 12

63

Yes, it is a code smell (in lots of cases).

I think it is difficult to replace if-else with virtual methods in tools

In your example, it is quite simple to replace the if/else by virtual methods:

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

Now there is no need any more for your if block, the caller can just use it like

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

Of course, for more complicated situations, such a solution is not always so obvious (but nethertheless almost anytime possible). But if you come to a situation where you do not know how to resolve the case with virtual methods, you can ask a new question again here on "Programmers" (or, if it becomes language or implementation specific, on Stackoverflow).

  • 4
    or, for that matter, on gamedev.stackexchange.com – Kromster says support Monica Jun 10 '16 at 7:01
  • 7
    You would not even need the concept of Sword this way in your codebase. You could just new Tool("sword", swordAttack, swordDefense) from e.g. a JSON file. – AmazingDreams Jun 10 '16 at 11:38
  • 7
    @AmazingDreams: that is correct (for the parts of the code we see here), but I guess the OP has simplified his real code for his question to focus on the aspect he wanted to discuss. – Doc Brown Jun 10 '16 at 12:12
  • 3
    This isn't that much better then the original code (well, it is a bit). Any tool that has additional properties can not be created without adding additional methods. I think in this case one should favor composition over inheritance. Yeah, there is currently only attack and defense, but that doesn't need to stay that way. – Polygnome Jun 10 '16 at 12:35
  • 1
    @DocBrown Yes that is true, though it looks like an RPG where a character has some stats which are modified by tools or rather equipped items. I would make a generic Tool with all the possible modifiers, fill up some vector<Tool*> with stuff read from a data file, then just loop over them and modify the stats like you do now. You'd get in trouble when you'd want an item to give e.g. a 10% bonus for attack though. Perhaps a tool->modify(playerStats) is another option. – AmazingDreams Jun 10 '16 at 12:55
23

The major problem with your code is, that whenever you introduce any new item, you not only have to write and update the item's code, you also have to modify your player (or wherever the item is used), which makes the whole thing a lot more complicated.

As a general rule of thumb, I think it's always kinda fishy, when you can't rely on normal subclassing/inheritance and have to do the upcasting yourself.

I could think of two possible approaches making the whole thing more flexible:

  • As others mentioned, move the attack and defense members to the base class and simply initialize them to 0. This could also double as a check whether you're actually able to swing the item for an attack or use it to block attacks.

  • Create some kind of callback/event system. There are different possible approaches for this.

    How about keeping it simple?

    • You could create a base class members like virtual void onEquip(Owner*) {} and virtual void onUnequip(Owner*) {}.
    • Their overloads would be called and modify the stats when (un-)equipping the item, e.g. virtual void onEquip(Owner *o) { o->modifyStat("attack", attackValue); } and virtual void onUnequip(Owner *o) { o->modifyStat("attack", -attackValue); }.
    • The stats could be accessed in some dynamic way, e.g. using a short string or a constant as a key, so you could even introduce new gear specific values or bonuses you don't necessarily have to handle in your player or "owner" specifically.
    • Compared to just requesting the attack/defense values just in time this not only makes the whole thing more dynamic, it also saves you unnecessary calls and even allows you to create items that will affect your character permanently.

      For example, imagine a cursed ring that will just set some hidden stat once equipped, marking your character as cursed permanently.

7

While @DocBrown has given a good answer, it doesn't go far enough, imho. Before you start evaluating the answers, you should evaluate your needs. What do you really need?

Below I will show two possible solutions, which offer different advantages for different needs.

The first is very simplistic and tailored specifically to what you have shown:

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

This allows very easy serialization/deserialization of tools (e.g. for saving or networking), and doesn't need virtual dispatch at all. If your code is all you have shown, and you don't expect it to evolve much other then having more different tools with different names and those stats, only in different amounts, then this is the way to go.

@DocBrown has offered a solution that still relies on virtual dispatch, and that can be an advantage if you somehow specialize the tools for parts of your code that was not shown. However, if you really need or want to also change other behavior, then I would suggest the following solution:

Composition over inheritance

What if you later want a tool that modifies agility? Or run speed? To me, it seems you are making an RPG. One things that is important for RPGs is to be open for extension. The solutions shown until now don't offer that. You would have to alter the Tool class and add new virtual methods to it every time you need a new attribute.

The second solution I'm showing is the one I hinted at earlier in a comment - it uses composition instead of inheritance and follows the "closed for modification, open for extension* principle. If you are familiar with how entity systems work, some things will look familiar (I like to think of composition as the smaller brother of ES).

Note that what I am showing below is significantly more elegant in languages that have runtime type information, like Java or C#. Therefore, the C++ code I'm showing has to include some "bookkeeping" that is simply necessary to make composition work right here. Maybe someone with more C++ experience is able to suggest an even better approach.

First, we look again at the side of the caller. In your example, you as the caller inside the attack method don't care about tools at all. What you care about is two properties - attack and defense points. You don't really care where those come from, and you don't care about other properties (e.g. run speed, agility).

So first, we introduce a new class

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

And then, we create our first two components

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

Afterwards, we make a Tool hold a set of properties, and make the properties query-able by others.

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

Note that in this example, I only support having one component of each type - this makes things easier. You could in theory also allow multiple components of the same type, but that gets ugly very fast. One important aspect: Tool is now closed for modification - we will never ever touch the source of Tool again - but open for extension - we can extend the behavior of a Tool by modifiyng other things, and just by passing other Components into it.

Now we need a way to retrieve tools by component types. You could still use a vector for tools, just like in your code example:

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

You could also refactor this into your own Inventory class, and store lookup tables that greatly simplify retrieving tools by component type and avoid iterating over the whole collection again and again.

What advantages has this approach? In attack, you process Tools that have two components - you don't care about anything else.

Lets imagine you have a walkTo method, and now you decide that it is a good idea if some tool would gain the ability to modify your walking speed. No problem!

First, create the new Component:

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

Then you simply add an instance of this component to the tool you want to increase your waking speed, and change the WalkTo method to process the component you just created:

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

Note that we added some behavior to our Tools without modifying the Tools class at all.

You can (and should) move the strings to a macro or static const variable, so you don't have to type it again and again.

If you take this approach further - e.g. make components that can be added to the player, and make an Combat component that flags the player as being able to participate in combat, then you can get rid of the attack method as well, and let that be handled by the Component or be processed elsewhere.

The advantage of making the player be able to get Components, too, would be that then, you wouldn't even need to change the player to give him different behavior. In my example, you could create a Movable component, that way you don't need to implement the walkTo method on the player to make him move. You would just create the component, attach it to the player and let someone else process it.

You can find a complete example in this gist: https://gist.github.com/NetzwergX/3a29e1b106c6bb9c7308e89dd715ee20

This solution is obviously a bit more complex then the others that have been posted. But depending on how flexible you want to be, how far you want to take it, this can be a very powerful approach.

Edit

Some other answers propose straight out inheritance (Making swords extend Tool, making Shield extend Tool). I don't think this is a scenario where inheritance works very well. What if you decide that blocking with a shield in a certain way can also damage the attacker? With my solution, you could simply add an Attack component to a shield and realize that without any changes to your code. With inheritance, you'd have a problem. items / Tools in RPGs are prime candidates for composition or even straight out using entity systems from the start.

1

Generally speaking, if you ever have the need to use if (in combination with requiring the type of an instance) in any OOP language, that's a sign, that something smelly is going on. At least, you should have a closer look at your models.

I would model your Domain differently.

For your usecase a Tool has an AttackBonus and a DefenseBonus - which could both be 0 in case it is useless for fighting like feathers or something like that.

For an attack, you have your baserate + bonus from the weapon used. The same goes for defense baserate + bonus.

In consequence your Tool has to have a virtual method for calculating the attack/defense boni.

tl;dr

With a better design, you could avoid hacky ifs.

  • Sometimes an if is necessary, for example when comparing scalar values. For object type switching, not so much. – Andy Jun 10 '16 at 6:44
  • Haha, if is a pretty essential operator and you cannot just say that using is a code smell. – tymtam Jun 10 '16 at 7:03
  • 1
    @Tymski to some respect you are right. I made myself more clear. I am not defending ifless programming. Mostly in combinations like if instanceof or something like that. But there is a position, which claims ifs are a codesmell and there are ways to get around it. And you are right, that is an essential operator which has its own right. – Thomas Junk Jun 10 '16 at 8:06
1

As written, it "smells," but that might just be the examples you gave. Storing data in generic object containers, then casting it to get access to the data is not automatically code smell. You will see it used in many situations. However, when you use it, you should be aware of what you're doing, how you're doing it, and why. When I look at the example, the use of string based comparisons to tell me what object is what is the thing that trips my personal smell meter. It suggests you're not entirely sure what you're doing here (which is fine, since you had the wisdom to come here to programmers.SE and say "hey, I don't think I like what I'm doing, help me out!").

The fundamental issue with the pattern of casting data from generic containers like this is that the producer of the data and the consumer of the data must work together, but it may not be obvious that they do so at first glance. In every example of this pattern, smelly or not smelly, this is the fundamental issue. It is very possible for the next developer to be completely unaware that you are doing this pattern and break it by accident, so if you use this pattern you must take care to help the next developer out. You have to make it easier for him to not break the code unintentionally due to some detail he may not know existed.

For example, what if I wanted to copy a player? If I just look at the contents of the player object, it looks pretty easy. I just have to copy the attack, defense, and tools variables. Easy as pie! Well, I'll find out quickly that your use of pointers makes it a little harder (at some point, it's worth looking at smart pointers, but that's another topic). That is easily resolved. I'll just create new copies of each tool, and put those in my new tools list. After all, Tool is a really simple class with only one member. So I create a bunch of copies, including a copy of the Sword, but I didn't know it was a sword, so I only copied the name. Later, the attack() function looks at the name, sees that it is a "sword", casts it, and bad things happen!

We can compare this case to another case in socket programming, which uses the same pattern. I can set up a UNIX socket function like this:

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

Why is this the same pattern? Because bind doesn't accept a sockaddr_in*, it accepts a more generic sockaddr*. If you look at the definitions for those classes, we see that sockaddr has only one member the family that we assigned to sin_family*. The family says which subtype you should cast the sockaddr to. AF_INET tells you that the address struct is actually a sockaddr_in. If it was AF_INET6, the address would be a sockaddr_in6, which has larger fields to support the larger IPv6 addresses.

This is identical to your Tool example, except it uses an integer to specify which family rather than a std::string. However, I'm going to claim it doesn't smell, and try to do so for reasons other than "its a standard way to do sockets, so it shouldn't 'smell.'" Obviously its the same pattern, which is why I claim that storing data in generic objects and casting it is not automatically code smell, but there's some differences in how they do it which make it safer.

When using this pattern, the most important information is capturing the conveyance of information about the subclass from producer to consumer. This is what you're doing with the name field and UNIX sockets do with their sin_family field. That field is the information the consumer needs to understand what the producer had actually created. In all cases of this pattern, it should be an enumeration (or at the very least, an integer acting like an enumeration). Why? Think about what your consumer is going to do with the information. They're going to need to have written out some big if statement or a switch statement, as you did, where they determine the correct subtype, cast it, and use the data. By definition, there can only be a small number of these types. You can store it in a string, as you did, but that has numerous disadvantages:

  • Slow - std::string typically has to do some dynamic memory to keep the string. You also have to do a full text comparison to match the name every time you want to figure out what subclass you have.
  • Too versatile - There's something to be said for putting constraints on yourself when you're doing something exceedingly dangerous. I've had systems like this which looked for a substring to tell it what type of object it was looking at. This worked great until the name of an object accidentally contained that substring, and created a terribly cryptic error. Since, as we stated above, we only need a small number of cases, there's no reason to use a massively overpowered tool like strings. This leads to...
  • Error prone - Let's just say that you will want to go on a murderous rampage trying to debug why things aren't working when one consumer accidentally sets the name of a magic cloth to MagicC1oth. Seriously, bugs like that can take days of head-scratching before you realized what happened.

An enumeration works much better. It's fast, cheap, and much less error prone:

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

This example also shows off a switch statement involving the enums, with the single most important part of this pattern: a default case that throws. You should never get in that situation if you do things perfectly. However, if someone adds an new tool type, and you forget to update your code to support it, you'll want something to catch the error. In fact, I recommend them so much that you should add them even if you don't need them.

The other huge advantage of the enum is that it gives the next developer a complete list of valid tool types, right up front. There's no need to go wading through the code to find Bob's specialized Flute class that he uses in his epic boss battle.

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

Yes, I put in an "empty" default statement, just to make it explicit to the next developer what I expect to happen if some new unexpected type comes my way.

If you do this, the pattern will smell less. However, to be smell-free the last thing you need to do is consider the other options. These casts are some of the more powerful and dangerous tools you have in the C++ repertoire. You shouldn't use them unless you have a good reason.

One very popular alternative is what I call a "union struct" or "union class." For your example, this would actually be a very good fit. To make one of these, you create a Tool class, with an enumeration like before, but instead of subclassing Tool, we just put all the fields from every subtype on it.

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

    int   attack;
    int   defense;
};

Now you don't need subclasses at all. You just have to look at the type field to see which other fields are actually valid. This is much safer and easier to understand. However, it has drawbacks. There are times you don't want to use this:

  • When the objects are too dissimilar - You can end up with a laundry list of fields, and it can be unclear which ones apply to each object type.
  • When operating in a memory-critical situation - If you need to make 10 tools, you can be lazy with memory. When you need to make 500 million tools, you're going to start caring about bits and bytes. Union structs are always bigger than they need to be.

This solution is not used by UNIX sockets because of the dissimilarity issue compounded by the open endedness of the API. The intent with UNIX sockets was to create something which every flavor of UNIX could work with. Each flavor could define the list of families they support, like AF_INET, and there would be a short list for each. However, if a new protocol comes along, like AF_INET6 did, you might need to add new fields. If you did this with a union struct, you'd end up effectively creating a new version of the struct with the same name, creating endless incompatibility issues. This is why the UNIX sockets chose to use the casting pattern rather than a union struct. I'm sure they considered it, and the fact that they thought about it is part of why it doesn't smell when they use it.

You could also use a union for real. Unions save memory, by only being as larger as the largest member, but they come with their own set of issues. This probably is not an option for your code, but its always an option you should consider.

Another interesting solution is boost::variant. Boost is a great library full of reusable cross-platform solutions. Its probably some of the best C++ code ever written. Boost.Variant is basically the C++ version of unions. It is a container which can contain many different types, but only one at a time. You could make your Sword, Shield, and MagicCloth classes, then make tool be a boost::variant<Sword, Shield, MagicCloth>, meaning it contains one of those three types. This still suffers from the same issue with future compatibility that prevents UNIX sockets from using it (not to mention UNIX sockets are C, predating boost by quite a bit!), but this pattern can be incredibly useful. Variant is often used, for example, in parse trees, which take a string of text and break it up using a grammar for rules.

The final solution which I'd recommend looking at before taking a plunge and using the generic object casting approach is the Visitor design pattern. Visitor is a powerful design pattern that takes advantage of the observation that calling a virtual function effectively does the casting you need, and it does it for you. Because the compiler does it, it can never be wrong. Thus, instead of storing an enum, Visitor uses an abstract base class, which has a vtable that knows what type the object is. We then create a neat little double-indirection call which does the work:

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

So what's this god aweful pattern? Well, Tool has a virtual function, accept. If you pass it a visitor, it is expected to turn around and call the correct visit function on that visitor for the type. This is what the visitor.visit(*this); does on each subtype. Complicated, but we can show this with your example above:

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
}

So what happens here? We create a visitor which will do some work for us, once it knows what type of object it's visiting. We then iterate over the list of tools. For argument's sake, let's say the first object is a Shield, but our code doesn't know that yet. It calls t->accept(v), a virtual function. Because the first object is a shield, it ends up calling void Shield::accept(ToolVisitor& visitor), which calls visitor.visit(*this);. Now, when we're looking up which visit to call, we already know that we have a Shield (because this function got called), so we will end up calling void ToolVisitor::visit(Shield* shield) on our AttackVisitor. This now runs the correct code to update our defense.

Visitor is bulky. It's so clunky that I almost think it has a smell of its own. It's very easy to write bad visitor patterns. However, it has one huge advantage none of the others have. If we add a new tool type, we have to add a new ToolVisitor::visit function for it. The instant we do this, every ToolVisitor in the program will refuse to compile because it's missing a virtual function. This makes it very easy to catch all cases where we missed something. It's much harder to guarantee that if you use if or switch statements to do the work. These advantages are good enough that Visitor has found a nice little niche in 3d graphics scene generators. They happen to need exactly the kind of behavior Visitor offers so it works great!

In all, remember that these patterns make it hard on the next developer. Spend time making it easier for them, and the code won't smell!

* Technically, if you look at the spec, sockaddr has one member named sa_family. There's some tricky being done here at the C level that doesn't matter for us. You're welcome to look at the actual implementation, but for this answer I'm going to use sa_family sin_family and others completely interchangeably, using whichever one is most intuitive for the prose, trusting that that C trickery takes care of the unimportant details.

  • Attacking consecutively will make the player infinitely strong in your example. And you can't extend your approach without modifying the source of ToolVisitor. Its a great solution, though. – Polygnome Jun 10 '16 at 18:39
  • @Polygnome You're right about the example. I thought the code looked odd, but scrolling past all those pages of text I missed the error. Fixing it now. As for the requirement of modifying the source of ToolVisitor, that's a design charactaristic of the Visitor pattern. It is a blessing (as I wrote) and a curse (as you wrote). Handling the case where you want an arbitrarily extensible version of this is far harder, and starts digging at the meaning of variables, rather than just their value, and opens up other patterns, such as weakly typed variables and dictionaries and JSON. – Cort Ammon Jun 10 '16 at 20:29
  • 1
    Yeah, sadly we don't know enough about the OPs preferences and goals to make a really informed decision. And yeah, a fully flexible solution is harder to implement, I worked on my answer for almost 3h, since my C++ is pretty rusty :( – Polygnome Jun 10 '16 at 20:35
0

In general I avoid implementing several classes/inheriting if it's just for communicating data. You can stick to a single class and implement everything from there. For your example, this is enough

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

Probably, you are anticipating that your game will implement several kind of swords etc. but you will have other ways to implement this. Class explosion is rarely the best architecture. Keep it simple.

0

As previously stated, this is a serious code-smell. However, one could consider the source of your problem to be using inheritance instead of composition in your design.

For example, given what you've shown us, you clearly have 3 concepts:

  • Item
  • Item which can have attack.
  • Item which can have defense.

Note that your fourth class is just a combination of the last two concepts. So I would suggest using composition for this.

You need a data structure to represent the information needed for attack. And you need a data structure representing the information you need for defense. Lastly, you need a data structure to represent things that may or may not have either or both of these properties:

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_;}
};
  • Also: do not use a compose-always approach :) ! Why using composition in this case ? I agree that it's an alternative solution, but creating a class for "encapsulating" a field (note the " ") seems weird in this case... – AilurusFulgens Jun 10 '16 at 14:12
  • @AilurusFulgens: It's "a field" today. What will it be tomorrow? This design allows Attack and Defense to become more complicated without changing the interface of Tool. – Nicol Bolas Jun 10 '16 at 14:15
  • 1
    You still can't extend Tool very well with this - sure, attack and defense might get more complex, but thats's it. If you use composition to its full power, you could make Tool complete closed for modification while still letting it open for extension. – Polygnome Jun 10 '16 at 18:20
  • @Polygnome: If you want to go through the trouble of making an entire arbitrary component system for a trivial case like this, that's up to you. I personally see no reason why I would want to extend Tool without modifying it. And if I have the right to modify it, then I don't see the need for arbitrary components. – Nicol Bolas Jun 10 '16 at 19:08
  • As long as Tool is under your own control, you can modify it. But the "closed for modification, open for extension" principle exists for good reason (too long to elaborate here). I don't think it's that trivial, though. If you spend the proper time planning a flexible component system for an RPG, you gain tremendous rewards in the long run. I don't see the added benefit in this type of composition over just using plain fields. Being able to specialize attack and defense further seems to be a very theoretical scenario. But as I wrote, it depends on the exact requirements of the OP. – Polygnome Jun 10 '16 at 19:16
0

Why not creating abstract methods modifyAttack and modifyDefense in the Tool class? Then each child would have their own implementation, and you call this elegant way:

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

Passing values as reference will save resources if you can:

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

If one uses polymorphism it is always best if all code that cares about which class is used is inside the class itself. This is how I would code it:

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

This has following advantages:

  • easier to add new classes: You only have to implement all abstract methods and the rest of the code just works
  • easier to remove classes
  • easier to add new stats(classes that dont care about the stat just ignore it)
  • You should at least also include an unequip() method that removes the bonus from the player. – Polygnome Jun 10 '16 at 18:14
0

I think one way to recognise the flaws in this approach is to develop your idea to its logical conclusion.

This looks like a game, so at some stage you will probably start to worry about performance and swap those string comparisons for an int or enum. As the list of items gets longer, that if-else starts to get pretty unwieldy, so you may consider refactoring it into a switch-case. You've also got quite a wall of text at this point so you may calve off the action within each case into a separate function.

Once you reach this point the structure of your code starts to look familiar - its beginning to look like a homebrew, hand-rolled vtable* - the basic structure upon which virtual methods are typically implemented. Except, this is a vtable that you must manually update and maintain yourself, each time you add or modify an item type.

By sticking to "real" virtual functions you are able to keep the implementation of each item's behaviour within the item itself. You are able to add additional items in a more self-contained and consistent way. And as you do all this, it is the compiler that will be taking care of the implementation of your dynamic dispatch, rather than you.

To solving your specific problem: you are struggling to write a simple pair of virtual functions for updating attack and defence because some items only affect attack and some items only affect defence. The trick in a simple case like this to implement both behaviours anyway, but with no effect in certain cases. GetDefenseBonus() might return 0 or ApplyDefenseBonus(int& defence) might just leave defence unchanged. How you go about it will depend on how you want to handle the other actions that do have an effect. In more complex cases, where there is more varied behaviour, you can simply combine the activity into a single method.

*(Albeit, transposed relative to the typical implementation)

0

Having a block of code that knows about all possible "tools" is not great design (especially since you'll end up with many such blocks in your code); but neither is having a basic Tool with stubs for all possible tool properties: now the Tool class must know about all possible uses.

What each tool knows is what it can contribute to the character that uses it. So provide one method for all tools, giveto(*Character owner). It will adjust the player's stats as appropriate without knowing what other tools can do, and what's best, it doesn't need to know about irrelevant properties of the character either. For example, a shield doesn't even need to know about the attributes attack, invisibility, health etc. All that's needed to apply a tool is for the character to support the attributes that the object requires. If you try to give a sword to a donkey, and the donkey has no attack stats, you'll get an error.

The tools should also have a remove() method, which reverses their effect on the owner. This is a little tricky (it's possible to end up with tools that leave a non-zero effect when given and then taken away), but at least it's localized to each tool.

-4

There's no answer that says that it doesn't smell so I'll be the one that stands for that opinion; this code is totally fine! My opinion is based on the fact that sometimes it's easier to move on and allow your skills to gradually increase as you create more new stuff. You can get stuck for days on making a perfect architecture, but probably nobody will ever even see it in action, because you never finished the project. Cheers!

  • 4
    Improving skills with personal experience is good, sure. But improving skills by asking people who already have that personal experience, so you don't need to fall into the hole yourself, is smarter. Surely that's the reason people ask questions here in the first place, isn't it? – Graham Jun 10 '16 at 14:01
  • I don't agree. But I understand that this site is all about going deep. Sometimes that means being overly pedantic. This is why I wanted to post this opinion, because it's anchored in reality and if you are searching for tips to become better and help for a beginner you miss this entire chapter about "good enough" that is pretty useful for a beginner to know. – Ostmeistro Jun 13 '16 at 9:07

protected by Community Jun 10 '16 at 17:32

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.