• Advertisement
Sign in to follow this  

When one object can be used multiple ways

Recommended Posts

I've asked this question 3 different times to varying results, and I'm not entirely sure why. I'll present my problem, solution, and hopefully someone can explain if it's or isn't code smell. 

First

Second

Third

I have the following Character Interface:

interface Character{
  void attack(Weapon toAttackwith, Enemy enemyObj);
  void reload(Reloadable weapon, Bullet bullets);
  void drinkHealthPotion(Potion health);
  void drinkManaPotion(Potion mana);
  void spendMoney(Money toSpend);
}
interface Reloadable {
    void reloadWeapon(Bullet bulletstoReload);
}

interface Weapon {
    void attack();
} 
public class ReloadableWeapon implements Weapon, Reloadable {
    @Override
    //logic to fire bullet
    public class attack();
    
    @Override
    //logic to reload Gun
    public class reload(Bullet bullet);
}
public class MainCharacter implements Character{
    //left out other method implementations
    void attack(Weapon to attackWith, Enemy enemy){
    }
    
    void Reload(ReloadableWeapon weapon, Bullet bullets){
        //call weapon.reloadWeapon(bullets);
    }
}

A character can do a lot of things. Here is why I placed Attack/Reload in the character interface:

A character can:

  • attack an enemy object with a Weapon.
  • reload a weapon with bullets.

To me it makes sense to place those two methods in the character interface, pass the required parameters, and call the necessary methods to complete the operations.

The first link suggests that I use the Observable pattern to listen for when the reload method has been called. To me this seems complex and unnecessary.  The suggested implementation just uses the weapon to attack. The provided code works, but seems like a lot for one action. For example, In Gun you have fire, but in MeleeWeapon you have hit, both are just attacks.

The second link, one of the commenters agreed that it should be part of the character, however another suggests a separate implementation for actions.

The third, considers it code smell, because I would have to keep updating to to accommodate any actions a character can take with an Object.

However, the problem I see is, regardless of where I place my actions, as I'm creating my game, I'll have to keep updating my interface/whatever container hold my actions. Now I understand, some game objects can only be used in one way, so rather than have a method called spend money I can have a method that takes the objects abstract type(in my game, GameItem), and call the use method, for example, spendMoney could be:

void useGameItem(GameItem item);

Drink potion can't be condensed into one drink method as both alter a character state in different ways(one will affect player health, the other the mana).

Some times an Object can be used in more than one way, for example my Gun. At this point you will have to determine who/what is using the object, and how.

For me placing attack/reload in the character interface makes sense, it's an action the character can do given certain situations/parameters.

If it's too much to read, my main idea is this. My character could use a GameItem, but certain GameItems could be used in more than one way. How can I handle this?

Can some explain why this is code smell/not ideal?

Share this post


Link to post
Share on other sites
Advertisement
All interface members should be part of a useful abstraction.
Is this:
character.attack(weapon, enemy)
Any different from this:
weapon.attack(enemy)
?
If not, then the character.attack function is useless and should be deleted.

I would imagine that part of the character abstraction though would be to hide the weapon interface from the character's user, so it would look more like:
character.attack(enemy) // this part of the code doesn't even know what a weapon is.

As well as interfaces providing meaningful abstraction that enforce invariants and present a simplified viee of the internal workings, they should also strive to reduce the dependencies between each software component.

As for your drinkHealth/ManaPotion functions - these seem inside out. If the character has a drink action with many polymorphic behaviours, those behaviours should be hidden away behind the abstract 'drink' interface function.

Maybe, for example, health/mana attributes range from 0-100, but can temporarily pushed up to 200 though they'll slowly self drain back down to 100.
You could implement both as some kind of CharacterAttribute class, with min/max/uberMax/selfDrainRate members, and Add/Subtract/Set/Update(deltaTime) functions.

A mana potion acting upon a character could call character.mana.Add(amount). The player just has to update its health/mana members each frame to implement the 200->100 "overcharge"/self-discharge feature. The Character class doesn't need to know about potions whatsoever, or how the mechanics of its health/mana attributes actually work.

Share this post


Link to post
Share on other sites

I would argue that the fundamental problem you have here is that you want to give your game entities an arbitrary number of mix-and-match units of functionality, which you implement with interfaces - but interfaces are a compile-time concept, designed to help you bind code together, not a run-time concept to help you examine objects for their capabilities. Therefore the place to mark a weapon as reloadable is not in the class definition - as they do not vary at run time - but in the object data somehow, including the potential choice of subclass.

In the weapon case, this could simply be done via changing the interface:

public interface Weapon {
    void attack();
    bool needsReloading();
    void replenish(); // does nothing if needsReloading returns false
}

But this does not necessarily extend to your general case, where you want individual items to be able to support a whole bunch of potential behaviours. The traditional way to do this would need to have all the various methods in the base GameItem object, and each subclass would implement them accordingly, including functions similar to needsReloading() to show the presence or absence of capabilities. It's a bit unwieldy, but it's been used in many shipping games.

A more adaptable approach comes in the form of components, where the presence or absence of a component tells you whether a certain interface is adhered to. For example, to represent a health potion, your GameItem might contain a CarryableComponent and a DrinkableComponent. A fountain might contain just the DrinkableComponent. A gun contains a AttackComponent, a ReloadableComponent, and a CarryableComponent; a turret only contains the first two, and an energy weapon or sword might not have the ReloadableComponent.

The downsides of components is that they can complicate the code - you need to check for component existence at run time (do you have something like 'GetComponent' or do you try to structure code so that you can be sure about which objects contain which components), you need to consider how to divide up the functionality (where does the ammo count live? which component checks for it?). Worse still, there are a lot of different philosophies on how components should be implemented, many coming from people who are concerned primarily about performance rather than ease of use.

In your case, I would recommend the chapter on components over at Game Programming Patterns.The subtitle there is "Allow a single entity to span multiple domains without coupling the domains to each other.", which is pretty much exactly what you want, and it doesn't get bogged down in some of the esoteric stuff that other component-aficionados sometimes get into.

Share this post


Link to post
Share on other sites

As for your drinkHealth/ManaPotion functions - these seem inside out. If the character has a drink action with many polymorphic behaviours, those behaviours should be hidden away behind the abstract 'drink' interface function.

Maybe, for example, health/mana attributes range from 0-100, but can temporarily pushed up to 200 though they'll slowly self drain back down to 100.
You could implement both as some kind of CharacterAttribute class, with min/max/uberMax/selfDrainRate members, and Add/Subtract/Set/Update(deltaTime) functions.

A mana potion acting upon a character could call character.mana.Add(amount). The player just has to update its health/mana members each frame to implement the 200->100 "overcharge"/self-discharge feature. The Character class doesn't need to know about potions whatsoever, or how the mechanics of its health/mana attributes actually work.

Bare with me here, cause I'm still learning :D

The problem I see is, the potions are updating two different attributes of the Character. If I hide the drinking of potions behind one method, how would I determine what attribute the potion was meant for? Health or Mana. Let's keep it simple, and say Health and Mana are int variables. How would I write my function to determine which attribute to update based on Potion, and then update it?

I would imagine that part of the character abstraction though would be to hide the weapon interface from the character's user, so it would look more like:
character.attack(enemy) // this part of the code doesn't even know what a weapon is.

Shouldn't the character be aware of what weapon it's using? and who it's using it on?

Edited by Nexusfactor

Share this post


Link to post
Share on other sites

In your case, I would recommend the chapter on components over at Game Programming Patterns.The subtitle there is "Allow a single entity to span multiple domains without coupling the domains to each other.", which is pretty much exactly what you want, and it doesn't get bogged down in some of the esoteric stuff that other component-aficionados sometimes get into.

If I understand the pattern correctly, it's composition? i.e Like Composition over Inheritance? Only in this case, instead of turning a "is-a" relationship into a "has-a", they are breaking down into smaller manageable pieces. it seems they are delegating certain tasks to other components.

I placed the reloadable in it's own interface because, not every Weapon is reloadable, for example Swords.

Share this post


Link to post
Share on other sites

Yes, it is composition. And I already explained why implementing reloadability via interfaces is arguably the wrong approach - it's not (practically) queryable at runtime.

Edit: let me try and word this another way. The purpose of an interface, in a typical programming language, is to say: "In this part of the program code, you are only allowed to use objects that conform to this interface". It ensures that objects are wired together, at compile time, with a predictable set of functionality. A function that requires an object implementing the Comparable interface always has, at compile time, the compareTo method. This is all decided before the program is ever run, to help you build maintainable software.

What you're looking for is a system that, at run time, can examine 2 existing objects and see what functionality they provide. That is a property of the objects, not of their classes. It is possible to return object properties via methods hard-coded into the classes - e.g. different return values from `needsReloading` depending on the object type - but that means all these objects must implement `needsReloading` which means they share the same (or at least one) interface in code. The interface(s) can do the work of telling you about the objects, but not the presence or absence of those interfaces.

Edited by Kylotan

Share this post


Link to post
Share on other sites

Yes, it is composition. And I already explained why implementing reloadability via interfaces is arguably the wrong approach - it's not (practically) queryable at runtime.

Let's say if in my PC Game, I hit the key to reload. On the technical side, what's happening?  I check if the Weapon contains a reloadable component? and if so, pass the reloadable, let the reloadable component to do it's job, and if not, pass null? isn't that bad practice?

Take for example a Sword class:

public class Sword implements Weapon {
    private int damage;
    //what should I return here, given that Sword isn't reloadable.
    private Reloadable reload;
}
Edited by Nexusfactor

Share this post


Link to post
Share on other sites

I updated my post above, so take a look at that. But no, I wouldn't implement Sword like that. Please re-read my first post, and especially the article that I linked as it would explain that (a) there would be no Sword class, and (b) there would be no Reloadable in that object as the whole point is that the absence of the component means it doesn't support whatever that component does. (You'd have to check for that either in the general Weapon class, or in the Player class when it uses a weapon, etc.)

Share this post


Link to post
Share on other sites

I hit the key to reload. On the technical side, what's happening?  I check if the Weapon contains a reloadable component? and if so, pass the reloadable, let the reloadable component to do it's job, and if not, pass null? isn't that bad practice?
Take for example a Sword class:

public class Sword implements Weapon {
    private int damage;
    //what should I return here, given that Sword isn't reloadable.
    private Reloadable reload;
}

All three (now all four) of your questions describe the same problem. This is yet another example of the problem.

Interfaces describe how a thing is to work, everything must be completely interchangeable. It IS-A thing. In all ways it is interchangeable with any other thing.  For example, a 13/16 spark plug must be interchangeable with all other 13/16 spark plugs, even though they may vary in their implementation details. Some may be iridium, some may have forked tips, but all of them can be used in an engine that accepts 13/16 spark plugs.

You need to be careful about building your interfaces.  Whatever it implements must be COMPLETELY replaceable with the parent.  If you have an interface that says an object has a function, perhaps a function named Reload(), then the function must behave interchangeably with all other objects based on the interface.

Even if the behavior is to do nothing and return true, the behavior must exist and must follow the same rules as its siblings.

Share this post


Link to post
Share on other sites

[Interfaces describe how a thing is to work, everything must be completely interchangeable. It IS-A thing. In all ways it is interchangeable with any other thing. For example, a 13/16 spark plug must be interchangeable with all other 13/16 spark plugs, even though they may vary in their implementation details.

This is a summary of the L in SOLID, the 5 core rules of OO.
Upon first reading these don't really give any practical advise as to how to fix your code... But it's worth keeping them in the back of your mind at all times so that eventually with experience you'll come to grok them.

If I hide the drinking of potions behind one method, how would I determine what attribute the potion was meant for? Health or Mana. Let's keep it simple, and say Health and Mana are int variables. How would I write my function to determine which attribute to update based on Potion, and then update it?

There's lots of ways. Perhaps Potions don't need to act upon the Character interface at all; instead they could act upon an Attributecollection interface which the Character implements. This is the D in SOLID - remove dependencies between components by creating a common interface instead.
The potion could have a name (enum, int, string, etc) specifying which attribute it wants to act on. It could ask the AttributeCollection to retrieve an attribute with that name, or null if it doesn't exist,and then the potion can then apply a change to that attribute. This would also allow potions to be used on any other game object that implements the AttributeCollection interface and contains a "mana" attribute.

Shouldn't the character be aware of what weapon it's using? and who it's using it on?

In your example, this is true but it's also true that the code that is controlling the character is also aware of these details... Which is a hint that the character abstraction is pretty weak.
If the character is fully aware of its weapon and its enemy, then the controlling code could just write:
character.attack(); // it knows which weapon and who

Share this post


Link to post
Share on other sites
There's lots of ways. Perhaps Potions don't need to act upon the Character interface at all; instead they could act upon an Attributecollection interface which the Character implements. This is the D in SOLID - remove dependencies between components by creating a common interface instead.
The potion could have a name (enum, int, string, etc) specifying which attribute it wants to act on. It could ask the AttributeCollection to retrieve an attribute with that name, or null if it doesn't exist,and then the potion can then apply a change to that attribute. This would also allow potions to be used on any other game object that implements the AttributeCollection interface and contains a "mana" attribute

Would something like this be adequate?

public class Potion {
    private final String potionName;
    private final int increaseBy;
    private final String potionType;
    
    public Potion(String name, int increase, String type){
        this.potionName = name;
        this.increaseBy = increase;
        this.potionType = type;
    }
    
    public int getincreaseBy(){
        return this.increaseBy;
    }
    
    public String getPotionType(){
        return this.potionType;
    }
   //Character class
   AbstractMap<String,Integer> characterAttribs = new HashMap<String,Integer>();
   characterAttribs.put("Health", 10);   
    public void Drink(Potion potion){
        if(potion.getPotionType() == "Health"){
            //grab the heath attribute by the key, increase it and put it back.
        }
    }

I've followed you're advice and rewrote my character interface as you described.

interface Character{
  void attack(Enemy enemyObj);
}

How then, would I allow my character to interact with the weapon so that it could reload if it wanted to? I understand the character knows the weapon, but it doesn't know the Weapon is reloadble. How would I let the Character know it's reloadable, and allow the character to do it?

Edited by Nexusfactor

Share this post


Link to post
Share on other sites

Your Drink implementation has an unnecessary step - instead of checking each name explicitly, just look up the name provided by the potion in your characterAttribs - if it exists, you have the attribute you need to modify.

 

I covered the reloading aspect above, though perhaps not clearly enough.

If the Character wants to know if a weapon is reloadable, it simply asks it. e.g. `bool Weapon::IsReloadable()`. (I called it 'needsReloading' in an earlier post.) This returns true or false depending on the type of the weapon.

Edited by Kylotan

Share this post


Link to post
Share on other sites

Your Drink implementation has an unnecessary step - instead of checking each name explicitly, just look up the name provided by the potion in your characterAttribs - if it exists, you have the attribute you need to modify.

Clarify this a little. How would I get the name without calling the getter?

Edited by Nexusfactor

Share this post


Link to post
Share on other sites

I didn't say "without calling the getter", I said without "checking the name explicitly".

public void Drink(Potion potion){
    if (characterAttribs.containsKey(potion.getPotionType()) {
        characterAttribs[potion.getPotionType()] += WhateverAmount;
    }
}

Share this post


Link to post
Share on other sites

 

I didn't say "without calling the getter", I said without "checking the name explicitly".

public void Drink(Potion potion){
    if (characterAttribs.containsKey(potion.getPotionType()) {
        characterAttribs[potion.getPotionType()] += WhateverAmount;
    }
}

I was just about to update my code the the same sample!

Thanks man! I mean it, it's been a rough few days attempting to figure all this out. At least on this forum, people are patient enough to help, and even provide sample code. Thanks for the links, and explanation. Here's a vote up :D

One more question, given that any Weapon that implements the Weapon interface has to implement all the methods, what should I do if a Weapon isn't reloadable, and therefore can't replenish, what should go in that method (Obviously, as you pointed out, it does nothing, but you didn't say "leave it empty", so do I throw an exception?

Edited by Nexusfactor

Share this post


Link to post
Share on other sites

You have to follow the rules set by the interface. If the interface allows calling "reloadWeapon", even if "canReload" returns false, then yes, expect that at some point it may get called, so generally I'd simply do nothing, as that's as good as it gets for a non-reloadable weapon.

 

If the interface forbids calling "reloadWeapon", if "canReload" returns false, then you can basically do anything you like in "reloadWeapon", since if it gets called, you are in undefined behavior territory, so anything you do or don't do is ok. This seems like a good point to kill the user randomly, or win the lottery, crash, or something else.

In the more sane behaviors however, depending on how paranoid you are, you either implement it as empty method (you shouldn't c<all it, but no harm done), or you assert "You shouldn't get here!". That's useful for development, so you notice you're doing things you're not supposed to do. In a release, asserts should do nothing, so a user will not see your assert message if he/she trigger a bug that ends up calling that method.

Edited by Alberth

Share this post


Link to post
Share on other sites

I'd be tempted to put an assert in there, because calling it would be an error that you need to fix as a programmer, rather than an exceptional condition that you need to deal with and recover from at runtime. (If it does happen in a release build, then doing nothing at all is reasonable.)

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this  

  • Advertisement