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

Lets first respond to your initial question:

is it fine to typecast from one implementation to another?

Absolutely! When using interfaces to declare behaviours like you have shown, not only is it fine but it becomes important that you DO typecast, otherwise what was the point of declaring the behavior through interfaces in the first place.

The use of interfaces implies that not all classes will implement the behaviour, so you can ONLY access the behaviour if your code knows the exact type of the concrete class or if you know the interface of the behaviour you want to execute, perhaps more importantly you may need to handle the case where the concrete class does NOT implement the expected interface.

Should you have to Type Cast?
If you are using interfaces, then yes you should have to typecast.

A better code implementation for WeaponReload would be to check first that the class does in fact implement IReloadable before reloading, since c# 7.0 we can use the is operator to test an expression against a pattern and we can capture the output in a single line of code:

if (weapon is IReloadable reloader)
    reloader.Reload();

Notice we didn't even need to use the Wrapper Client class WeaponReload to access the IReloadable.Reload() behaviour, I think this is what you are referring to with:

Is there a better way to handle this object creation part

Yes, the better way is to skip the Clients altogether.

If you are set on your current implementation, then that is just as easy to incorporate, however you should still handle the cases where the passed in object does not implement your interface.

WeaponReload reloader = null;
if (weapon is IReloadable r)
    reloader = new WeaponReload(r);
...
if (reloader != null)
    reloader.Reload();

Is there an entirely better design that I can come up with which does not necessarily ends up in casting.

There is nothing wrong with casting, it can lead to clear and concise code as you have created clear structural boundaries that can help you separate implementation from definitions.

If you are concerned about performance, then we have a solution for that too, but don't optimise your code for the sake of it, the performance losses due to boxing values into objects, though comparitively expensive to alternatives, wil not generally be noticied by the user unless you are performing millions of operations in quick succession.

To avoid performance issues caused by Boxing and Unboxing in C# we can use Generics. Interfaces play a huge part in C# generics, a generic type argument on a class or method defines all the interfaces or conditions that an object MUST implement for it to be passed into the methods as a parameter or stored in a variable.

Because these conditions are validated and enforced by the compiler, the storage and referencing to the underlying object stored in a generic typed variable is a lot more efficient at runtime compared to simply boxing objects into an object typed variable.

a Better design

Your interfaces are on the right track, interfaces allow your concrete classes to define explicit behaviors without having to incorporate those behaviours into their base class.

What you have missed here is a base class that all your weapons can inherit from, it's the first logical assumption that one makes when rationalising a domain that includes different types of weapons like shotguns and swords... Common properties might include Name, Description, Cost, BasePower... I would argue that all weapons have a Primary attack function, so really a Weapon base class would be replacing IWeaponPrimaryAttack:

public abstract class Weapon ()
{
    public string Name { get;set; }
    ... 
    public abstract void DoAttack();
}
public interface IWeaponSecondaryAttack
{
    void DoSecondaryAttack();
}
public interface IReloadable
{
    void Reload();
}

Now for the concrete class implementations, I would classify the weapons a bit differently again, I like to write down a breif story style description about the classes before I write the code, using the following phrasiology to help validate that my classes and interfaces make sense and can be utilised effectivly:

A Katana IS a Weapon, it has a Name of 'Katana' but it does NOT have a Reload feature, it's primary attack is a 'Slash' and it has a SecondaryAttack that is a 'Stab'.

A Shotgun IS a Weapon, it has a Name of 'Shotgun' and it DOES have a Reload feature, it only has a capacity of 2 shells before it must be reloaded, it does not have a SecondaryAttack.

This will work with your current interfaces, but notice already we have identified an element of redundancy, the Name is always the same as the type. This isn't a problem, but it is an indicator that there is potential room for improvement. Instead lets create classes that categorise the weapons :

In real life and in many games, you will find weapons categorised more granularly, for instance Swords vs Guns, Two handed vs One Handed, or Melee vs Ranged... Swords might include daggers and daggers can easily be Ranged as well as Melee...

Instead if we change our concrete classes to this:

public class Sword : Weapon, IWeaponSecondaryAttack
{
    public override void DoAttack() => Console.WriteLine("Slash");
    public void DoSecondaryAttack() => Console.WriteLine("Stab");
}
public class Gun : Weapon, IReloadable
{
    public override void DoAttack() => Console.WriteLine("Shoot");
    public void Reload() => Console.WriteLine("Reload");
}

We haven't yet handled the capacity of attacks before reload, should be easy to add this into the IReloadable interface, so we'll leave this out for.

Now we can create an array of weapons that the user might have access to that includes a 'Katana' as an instance of a 'Sword' as well as a 'Dagger' and of course our 'Shotgun' as an instance of Gun:

even though 'Dagger' and 'Katana' have a similar attack style, one would clearly deal more damage than the other, so Damage now becomes another property that we could add...

List<Weapon> weapons = new List<Weapon>
                           {
                               new Sword { Name = "Katana" },
                               new Sword { Name = "Dagger" },
                               new Gun { Name = "Shotgun" }
                           }; 

Although it doesn't make sense in a real-world situation to talk about the reload function on a Katana, you could for the purposes of simplifying the code say that all Weapon instances do have a Reload function, but the Reload function on a Katana does nothing. The same could be said for SecondaryAttack, in that case these become properties on the Weapon base class. The argument about if it should or shouldn't be an interface could swing either way depending on how you implement it in the rest of your code.

Another scenario to consider is What is an Attack? By making the attack method a void response type, we're really saying that anything and everything could occur when the method is executed, we have not feedback and cannot determine if the attack succeeded, or if it even ocurred at all.

If Attack returned a value or better yet an object that defined the parameters of the attack, then we can start to tell some interesting stories! Take

On the third attack, user with Shotgun equipped was out of ammo and had to use it as a 'blunt force' melee weapon, that had less damage, but required the user to be within melee range...

Write out user stories that explain the values and states of the objects over time, this will help you to identify the structures that you need, then we can talk about better implementations ;)

Answer 2

Composition over inheritance and Strategy Pattern

I would, instead of having differents weapons implementing differents interfaces defining their behaviour, create classes that holds the logic of the behaviour

Let's consider a weapon can have a primary attack, a secondary one and reload.

The class could be defined as such :

abstract class Weapon
{
    public IWeaponPrimaryAttack WeaponPrimaryAttack { get; set; }
    public IWeaponSecondaryAttack WeaponSecondaryAttack { get; set; }
    public IReloadable Reloadable { get; set; }
}

Now, we can define classes that holds differents behaviours of the interfaces.

In example :

public class SlashAttack : IWeaponPrimaryAttack
{
    public void DoAttack()
    {
        Console.WriteLine("Performing a slash attack");
    }
}

public class StabAttack : IWeaponSecondaryAttack
{
    public void DoSecondaryAttack()
    {
        Console.WriteLine("Performing a stab attack");
    }
}

public class Shooting12Caliber : IWeaponPrimaryAttack
{
    public void DoAttack()
    {
        Console.WriteLine("Bam !");
    }
}

public class Caliber12Reloader : IReloadable
{
    public void Reload()
    {
        Console.WriteLine("Reloading caliber 12 !");
    }
}

public class ShootingWith556 : IWeaponPrimaryAttack
{
    public void DoAttack()
    {
        Console.WriteLine("Bam (version 5.56) !");
    }
}

public class Caliber556Reloader : IReloadable
{
    public void Reload()
    {
        Console.WriteLine("Reloading caliber 5.56 !");
    }
}

And now, you can use theses class as properties in differents weapon classes.

You'll implement once the methods and can use the same for differents weapons :

public class Katana : Weapon
{
    public Katana()
    {
         WeaponPrimaryAttack = new SlashAttack();
         WeaponSecondaryAttack = new StabAttack();
    }
}

public class Machete : Weapon
{
    public Machete()
    {
         WeaponPrimaryAttack = new SlashAttack();
    }
}

public class Shotgun : Weapon
{
    public Shotgun()
    {
         WeaponPrimaryAttack = new Shooting12Caliber();
         Reloadable = new Caliber12Reloader();
    }
}

public class AssaultRifleWithBayonet : Weapon
{
    public AssaultRifleWithBayonet()
    {
         WeaponPrimaryAttack = new ShootingWith556();
         WeaponSecondaryAttack = new StabAttack();
         Reloadable = new Caliber556Reloader();
    }
}

Now, your methods implementations are refactored within classes and are reusable. You don't need to typecast to check if this or that interface is implemented.

if (weapon.Reloadable != null)
{
    weapon.Reloadable.Reload();
}
else
{
    Console.WriteLine("This weapon isn't meant to be reloaded");
}

/*
This works too if you want to omit the Reloadable != null

public class CannotReload : IReloadable
{
    public void Reload()
    {
        Console.WriteLine("This weapon isn't meant to be reloaded");
    }
}
*/

Answer 3

Since Weapon and Attack are partially related, you may need to implement each separately in order to make them flexible for future changes, and since Attack can be defined in multiple classes (not just for weapons), we can either implement an interface for it, or an abstract class, (or both if needed) and use it as definition of the attack logic.

If you want to define a concrete class for each weapon, and just inherit the Weapon class, you can do that, though, it won't be easy to handle nor to memorized; because more weapons, means more classes to handle.

For your current code, you could start with something like these interfaces :

public interface IWeapon
{
    WeaponType Type {  get; set;  }
}

public interface IReloadable 
{
    double IsReloadable {  get; set;  }     
    void Reload();    
}

public interface IAttack
{
    AttackType Type { get; set; }
}

Then, using enum to define some types :

public enum WeaponType { Katana, Shutgun, Knife }
public enum AttackType { Primary, Secondary }

now, we will use them in our classes

public class Weapon : IWeapon, IReloadable
{
    public WeaponType Type { get; set; }

    public bool IsReloadable  { get; set; } 

    public void Reload()
    {
        if(IsReloadable)
        {
            .....
        }
    }
    ...
}

public class Attack : IAttack
{
    public AttackType Type { get; set; }    
    ....    
}

public class WeaponAttack
{
    private readonly Weapon _weapon; 

    private readonly Attack _attack;

    public WeaponAttack(Weapon weapon, Attack attack)
    {
        _weapon = weapon; 
        _attack = attack;
    }

    public void DoAttack()
    {
        if(_attack.Type == AttackType.Primary)
        {
            // do something..
        }       
    }   

}

With this, we know if we use Weapon or Attack we are defining types, and setting their properties, logic for each type to be ready for use. WeaponAttack will use these prepared objects to do some actions on them.

so in your example of shutgun, we'll be revised to this

//define your weapon
var shutgun = new Weapon
{
        Type = WeaponType.Shutgun, 
        IsReloadable = true 
};
//define your attack 
var attack = new Attack
{
    Type = AttackType.Primary, 
    DamagePoints = 2.5
}; 

// take the action 
var attackAction = new WeaponAttack(shutgun, attack); 
attackAction.DoAttack();

This is the simplest form that came to my mind. It'll be easy to extend, for instance, say you want to add a new interface IDamage for storing damage points per attack :

public interface IDamage
{
    double DamagePoints { get; set; }
}

Then, you just implement it on Attack class :

public class Attack : IAttack, IDamage
{
    public AttackType Type { get; set; }    

    public double DamagePoints { get; set; }
}

Same thing applies to the Weapon class, if you want to add new features, it would be easy to add with minimal modifications to the current code. Also, I would prefer to change the constructor on WeaponAttack to accept the interfaces instead of the concrete class, it would be more fixable this way. But I used the concrete classes just for demonstration purpose.