Is it fine to typecast from one implementation type to another while using an object?

Question

I've a simple code base with few 'weapon' concrete classes which implements different contracts in order to be used by its clients.

My contracts:

public interface IWeaponPrimaryAttack
{
    void DoAttack();
}
public interface IWeaponSecondaryAttack
{
    void DoSecondaryAttack();
}
public interface IReloadable
{
    void Reload();
}

Concrete implementation or the actual weapons:

public class Katana : IWeaponPrimaryAttack, IWeaponSecondaryAttack
{
    public void DoAttack(){Console.WriteLine ("Swing");}
    public void DoSecondaryAttack() {Console.WriteLine ("Stab");}
}
public class ShotGun : IWeaponPrimaryAttack, IReloadable
{
    public void DoAttack(){Console.WriteLine ("Swing");}
    public void Reload() {//reload it}
}

Clients that uses these concrete classes:

public class PrimaryAttack
{
    private IWeaponPrimaryAttack _attack;
    public PrimaryAttack(IWeaponPrimaryAttack attack)
    {
        _attack = attack;
    }
    public void DoAttack()
    {
        _attack.DoAttack();
    }
}
public class SecondaryAttack
{
    private IWeaponSecondaryAttack _attack;
    public SecondaryAttack(IWeaponSecondaryAttack attack)
    {
        _attack = attack;
    }
    public void DoSecondaryAttack()
    {
        _attack.DoSecondaryAttack();
    }
}
public class WeaponReload
{
    private IReloadable _reloader;
    public WeaponReload(IReloadable reloader)
    {
        _reloader = reloader;
    }
    public void Reload()
    {
        _reloader.Reload();
    }
}

New of course the instantiation of concrete class only be known when the user selects one among many weapons(one among ShotGun, Katana, etc).

Let say the use has selected ShotGun as the weapon, based on the selection it might go like this:

IWeaponPrimaryAttack weapon = new ShotGun(); // get it from a factory
PrimaryAttack primaryAttack = new PrimaryAttack(weapon);
primaryAttack.DoAttack();

Now for the WeaponReload have to do a typecast here in order to use it.

WeaponReload reloader = new WeaponReload ((IReloadable)weapon);
reloader.Reload();

I have questions around it,

• Should I really have to end up typecasting eventually?
• Or there is a better way to handle this object creation part.
• Or there is entirely a better design that I can come up with which does not necessarily ends up in casting.
• Or casting it like this is totally fine.

Answer 1

Not so sure why you need all these extra delegating wrappers? Anyhow, there's a few things at play here.

Maximizing composition

You've used a concrete type per weapon type here, but you could also put more emphasis on composition and have a single all-encompassing Weapon class that delegates all it's inner workings to strategies.

e.g.

Rather than Weapon shotgun = new Shotgun(); you could have Weapon shotgun = Weapons.shotgun() where the factory method may look like:

return new Weapon.Builder()
    .withPrimaryAttack(...)
    .withoutSecondaryAttack()
    .withSlowReload().build();

Maximizing composition makes the design very flexible and could allow you to introduce new weapon types dynamically if needed or even change certain aspects of a weapon at runtime (e.g. shotgun now fires knifes b/c of a picked-up powerup).

Following the ISP

In the composition-based approach described above, you may notice that the Weapon's interface will become bloated with all sort of things weapons can do. Clients depending on the Weapon class will indirectly depend on all the various implicit interface methods they may never call.

In order to reduce clients coupling you could very well make sure that Weapon features are segregated into many interfaces such as IReloadable, etc. The Weapon class would implement them all, but client code interested in only a subset of weapon features could still depend on these interfaces rather than Weapon.

e.g.

reload(weapon);

void reload(IReloadable reloadable) {
    if (stamina < ...) throw ...;

    reloadable.reload();
}

ISP & feature-detection

Considering your original design, I don't think there's anything fundamentally wrong with using instanceof as a feature detection mechanism.

Using instanceof to match concrete types is certainly wrong, but matching interfaces is most likely fine.

weapon instanceof Shotgun //bad
weapon instanceof IReloadable //ok

Note that you should always check with instanceof before casting. Also note that you need to think of an approach to make weapon implementers know the set of potential weapon-feature interfaces they could implement.