Inheritance, need advices

Question

Let's say I have an ai or player, I want him to be able to use different weapons. My design with weapons:

public class Weapon() { public virtual void FireWeapon(){} // this is useless for melee weapons public virtual void SwingMelee(){} // this is useless for guns public virtual void Reload(){} // this is also useless for melee weapons }

Then in the ai controller class I simply call the function I want him to do. This is where the ugly part is (I think)... Controller class have a list containing some different weapons of ai and a weapon which is being used.

public class WeaponController { private List<Weapon> someWeapons; private Weapon aWeapon; public void Main() { if(/*"Some action or a button click" &&*/ aWeapon.CanSwingMelee() ) aWeapon.SwingMelee(); if(/*"Some action or a button click" &&*/ aWeapon.CanReload() ) aWeapon.Reload(); } }

What is the better way to implement this? do you have any advices? Seems that for every different action in a new weapon, I need to implement a function in the most parent Weapon class and I don't think it's a good idea...

Why not just make the virtual method Use, and whether it's a swing or a shot, it's just used... why be so specific? It's going to do damage at the end of the day, right, that should be what counts — Grant Thomas
– Grant Thomas, Commented Jul 12, 2017 at 0:19
I'd create an interface with a UseWeapon method and then implement classes that fire or swing based on what they are. Basically you want the weapon to know how it's used. — juharr
– juharr, Commented Jul 12, 2017 at 0:20
the functions are just for explaining my problem with this design. for example Reloading weapon is also useless for melee weapons... — 31thUser
– 31thUser, Commented Jul 12, 2017 at 0:22
You'd better contact a lawyer to help you draft and notarize a will--oh. — Prime
– Prime, Commented Jul 16, 2017 at 4:08

John Wu · Accepted Answer · 2017-07-12 00:48:11Z

The capability of an in-game object can be represented by an interface; you can check if a capability is present by attempting to cast to the interface. What's more, these interfaces can overlap, e.g. both melee and ranged weapons might both have an Attack method.

So for example:

public interface IWeapon { void Attack(); } public interface IRangedWeapon { bool IsInRange(ITargetable target); } public interface IRequiresAmmunition { void Reload(); int AmmoRemaining { get; set; } } public class Sword : IWeapon { public virtual void Attack() { //code } } public class Rifle : IWeapon, IRequiresAmmunition, IRangedWeapon { public virtual void Attack() { //code } public virtual void Reload() { //code } public virtual int AmmoRemaining { get { } set { } } public virtual bool IsInrange (ITargetable target) { //code } } public class LaserGun: IWeapon, IRangedWeapon { public virtual void Attack() { //code } public virtual bool IsInrange (ITargetable target) { //code } } public class WeaponController { private List<IWeapon> someWeapons; private IWeapon aWeapon; private ITargetable currentTarget; public void Weapon_OnUse() { if (!currentTarget.IsHostile) return; if (this.IsInMeleeRange(currentTarget)) { aWeapon.Attack(); return; } var w = aWeapon as IRangedWeapon; if (w != null && w.IsInRange(currentTarget) { aWeapon.Attack(); return; } context.HUD.Warn("Out of range"); } public void Weapon_OnReload() { var w = aWeapon as IRequiresAmmunition; if (w != null) { w.Reload(); context.HUD.DisplayAmmo(w.AmmoRemaining); } } }

thank you, this looks much better :) also nice interface example
I can see that Weapon_OnUse method getting long and out of control. It would be difficult to test.
Yep... this happens on Stackoverflow a lot... there is a limit to how much I will refactor a poster's sample code when all I'm doing is demonstrating a principle in action. Could make for an interesting (other) question though.

NetMage · Accepted Answer · 2017-07-12 00:22:59Z

This seems like what abstract classes and inheritance is for:

public abstract class Weapon { public abstract void Attack(); public abstract void Reload(); } public class MeleeWeapon : Weapon { public override void Attack() { // swing sword } public override void Reload() { // ignore reload } } public class GunWeapon : Weapon { public override void Attack() { // fire gun } public override void Reload() { // load weapon from inventory } } public class WeaponController { private List<Weapon> someWeapons; private Weapon aWeapon; public void Main() { if (/*"Some action or a button click" */) aWeapon.Attack(); else if (/* some other button click */) aWeapon.Reload(); } }

thanks for the answer but the only issue is not the Attack. it is all kinds of different behaviours according to some other actions like a button click
@31thUser your examples only give attack and reload, give a concrete example not covered by this answer
Yes, I've seen it. So you're saying that this is not that ugly at all? okayy.. thanks
@31thUser if you have actions specific to one weapon (or subclass) (Polish perhaps) then add it to that specifc class or subclass.

Scott Hannen · Accepted Answer · 2017-07-12 01:18:51Z

I don't recommend an approach that requires you to create new interfaces for every new behavior and check the type of the weapon. What about something like this:

(This is a very rough draft.)

public abstract class Weapon { protected Weapon(WeaponCommandStrategy[] commandStrategies) { CommandStrategies = commandStrategies; } protected IEnumerable<WeaponCommandStrategy> CommandStrategies { get; } public void Use(WeaponCommand command) { var strategy = CommandStrategies.FirstOrDefault(c => c.Command == command); strategy?.Execute(); } } public enum WeaponCommand { Fire, Swing, Reload } public abstract class WeaponCommandStrategy { public WeaponCommand Command { get; private set; } protected WeaponCommandStrategy(WeaponCommand command) { Command = command; } public abstract void Execute(); }

Now you can give a weapon whatever behaviors you want it to have in the form of various instances of WeaponCommandStrategy. If a command is sent to a weapon, it executes it. If it doesn't support a command it ignores it. You could add a property to a weapon exposing the available commands so that you could display a list of available commands.

public class Sword : Weapon { // Perhaps use dependency injection here public Sword() : base(new WeaponCommandStrategy[] { new SwordSwingStrategy() }) { } } public class SwordSwingStrategy : WeaponCommandStrategy { public SwordSwingStrategy() : base(WeaponCommand.Swing) { } public override void Execute() { // Do whatever it does } }

This essentially makes a Weapon a composition of various things that a weapon can do. If several weapons behave similarly they can share strategies vs. having code duplicated between various weapons.

Yes it can work. I'll upvote, however when I compare the john wu's answer, all we get would be deleting some downcast code for every command which is quite good, only disadvantage I think is passing all the needed data to WeaponCommandStrategy to execute a command. Still not sure which one to use :)
After I think a while Your suggestion is way better. And To get rid of useless command searchs, weapon can also tell controller which commands it can use .
Depending on how complex your system is gonna be a I would personally do a mix of both methods (interface and this one). Interface used to define what weapon can and cannot do, and strategy for specifics on how they do things.
In most cases I would rather not inspect an object to see what its type is. It violates the open/closed principle on the class that's doing the inspection. Every time we create a new behavior we'll need to create a new interface and then check for that interface. Better to know what the type is and tell it what to do. Then that type decides how to handle that action.
It seems like Execute implementations would have to have a switch/case handling each WeaponCommand they implement. I don't think that feels good either.

Collectives™ on Stack Overflow

Inheritance, need advices

3 Answers 3

3 Comments

10 Comments

9 Comments

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

3 Comments

10 Comments

9 Comments

Related