How should I implement this to avoid downcasting (or is it ok if I downcast in this case)?-CodePudding

Im making a multiplayer game about space battles

There is a class called Craft which can contain many different types of components (Like thrusters, weapons, shields). Each component type is a child of an abstract CraftComponent class. The game is played in intervals of a few seconds with players giving instructions to their spacecrafts (and to the server) in between. The types of instructions a craft needs depend on the components it has.

Using a UI I havent implemented yet, the players need to be able to create an instance of a CraftInstruction that contains an instance of ComponentInstrucction for each component of the spacecraft. Each component type has an associated instruction type that inherits from ComponentInstrucction.

In order to not have to change the mechanism for sending instructions everytime I add a new type of component, the instructions are send as ComponentInstrucction, which means that when it reaches the component in question it first has to be downcast it to the specific component instruction type.

I know downcasting should generally be avoided and its usually a sign of a not-so-clean implementation, so, is there a better way I should try to do this?

CodePudding user response：

Its a common practice to aggregate objects of different types into an array of a base type or interface type. It might be tempting to have code that checks if each element is of some type and cast to do an action like

foreach (var item in componentInstrucction){
    if( item is CraftInstruction ci){
       ci.Craft() // do stuff

but you could have that Craft method in the base class as abstract or virtual and just call it:

foreach (var item in componentInstrucction){
       item.Craft()


abstract class ComponentInstrucction{
    public abstract void Craft();
}

CodePudding user response：

You can use the visitor pattern plus dynamic keyword to make this quite nicely.

Example script below. I had to use .NET 4.x mode for this.

using System.Collections.Generic;
using UnityEngine;

public class test : MonoBehaviour
{
    void Start()
    {
        List<ComponentInstruction> instructionList = new List<ComponentInstruction>();
        List<CraftComponent> componentList = new List<CraftComponent>();
        
        componentList.Add(new NoseConeComponent());
        componentList.Add(new EngineComponent());
        componentList.Add(new EngineComponent());
        
        instructionList.Add(new NoseConeInstruction());
        instructionList.Add(new EngineInstruction());

        // What happens when there is a mismatch?
        instructionList.Add(new NoseConeInstruction());
                            
        for (int i = 0; i < componentList.Count; i  )
        {
            ComponentInstruction inst = instructionList[i];
            CraftComponent comp = componentList[i];
            
            inst.Visit(comp);
        }
    }
}

abstract class CraftComponent {}

abstract class ComponentInstruction
{
    public abstract void Visit(CraftComponent c);

    // default behavior for component
    // we will just throw an exception because this should never happen
    public void DynamicVisit(CraftComponent c)
    {
        throw new System.InvalidOperationException();
    }
}

class EngineComponent : CraftComponent {}
class NoseConeComponent : CraftComponent {}

class EngineInstruction : ComponentInstruction
{
    public override void Visit(CraftComponent c)
    {
        DynamicVisit((dynamic) c);
    }
    
    public void DynamicVisit(EngineComponent c)
    {
       Debug.Log("Engine instructed");
    }
}

class NoseConeInstruction : ComponentInstruction
{
    public override void Visit(CraftComponent c)
    {
        DynamicVisit((dynamic) c);
    }
    
    public void DynamicVisit(NoseConeComponent c)
    {
       Debug.Log("NoseCone instructed");
    }
}

Based on code by Iaian Galloway

Which results in:

NoseCone instructed
Engine instructed

InvalidOperationException: Operation is not valid due to the current state of the object.
(wrapper dynamic-method) System.Object.CallSite.Target(System.Runtime.CompilerServices.Closure,System.Runtime.CompilerServices.CallSite,NoseConeInstruction,object)
System.Dynamic.UpdateDelegates.UpdateAndExecuteVoid2[T0,T1] (System.Runtime.CompilerServices.CallSite site, T0 arg0, T1 arg1) (at <351e49e2a5bf4fd6beabb458ce2255f3>:0)
(wrapper dynamic-method) System.Object.CallSite.Target(System.Runtime.CompilerServices.Closure,System.Runtime.CompilerServices.CallSite,NoseConeInstruction,object)
NoseConeInstruction.Visit (CraftComponent c) (at Assets/test.cs:65)
test.Start () (at Assets/test.cs:26)

Use of the dynamic keyword means this approach uses late binding. In other words, it 1. is not as performant as if you wrote a method for every concrete CraftComponent in each ComponentInstruction and 2. does not offer certain compile-time type guarantees e.g., you won't get a compile error if you somehow end up calling a NoseConeInstruction with a EngineComponent. These may be a deal breaker for you.

On the plus side, each instruction only needs to know about its corresponding component, and vice versa, and the base classes only need to be aware of defaults. If you expect to have many concrete component types, this is worth it in my opinion.