Code Review - Event Use?

Posting here always seems to get my brain juices flowing a bit better and I've re-written my code to use events more extensively.

Explanation of my event call chain.

When the user makes a power request (by clicking on an increase subsystem power icon), the button notifies the appropriate subsystem via an event that it needs to start a power request. The subsystem then first checks to make sure its even capable of getting more power and if possible, notifies its parent power relay via calling a power request event. The appropriate relay receives this event and checks to make sure that the relay can handle more power flowing through it. If it cannot, it does a callback event to the subsystem denying the power request, if it can, it calls another power request event that the WarpCore is listening to. The warp core checks if it has any available power and either allocates power and does a successful power request callback, or if it cannot, it calls a refused power request callback. Finally if warpcore sends back a successful request then the relay will increase its power load and the subsystem will allocate more power to itself. If the call wasn't a success, the relay will notify the subsystem of the failure and neither will increase their power usage. ....Wow that sounds way more complicated than it was to code.

I've posted the classes below to get any potential feedback on my method! I love code reviews. The classes are in event call order as best as possible.

public class ControlButton : MonoBehaviour
{
    private EventButton eventButton = null;
    private Subsystem subsystem = null;

    public void Initialize(Subsystem subsystem)
    {
        eventButton = this.gameObject.ValidateComponent<EventButton>();
        eventButton.onLeftClick += subsystem.AllocatePowerRequest;
        eventButton.onRightClick += subsystem.DeallocatePowerRequest;
        this.subsystem = subsystem;
    }
}

public class Subsystem : MonoBehaviour
{
    public event EventHandler OnAllocationChange;
    public event EventHandler OnPotentialAllocationChange;
    public event EventHandler OnWantPower;

    public Console console = null;
    public Equipment equipment = null;
    public Relay relay = null;

    public virtual void InitializeNew(int currentHitPoints, int maximumHitPoints, int currentLevel, int maximumLevel, object[] abilities)
    {
        equipment.CurrentHitPoints = currentHitPoints;
        equipment.MaximumHitPoints = maximumHitPoints;
        IsActive = true;
        CurrentLevel = currentLevel;
        MaximumLevel = maximumLevel;
        Abilities = abilities;
        HookEvents();
    }

    private void HookEvents()
    {
        equipment.Damaged += EquipmentDamaged;
        console.Damaged += ConsoleDamaged;
        relay.OnApprovedRequestPower += AllocatePower;
        relay.OnRefuseRequestPower += NotGivenPower;
    }

    private void AllocatePower(object sender, EventArgs e)
    {
        PowerTransferInfoEventArgs powerTransferInfo = (PowerTransferInfoEventArgs)e;
        if (powerTransferInfo.Subsystem == this)
            DoAllocation();
    }

    private void NotGivenPower(object sender, EventArgs e)
    {
        // TODO: Do something showing we were not given power.
    }

    public virtual void AllocatePowerRequest(object sender, EventArgs e)
    {
        if (ActiveLevel < CurrentLevel)
        {
            if (OnWantPower != null)
            {
                PowerTransferInfoEventArgs powerTransferInfo = new PowerTransferInfoEventArgs();
                powerTransferInfo.Subsystem = this;
                OnWantPower(this, powerTransferInfo);
            }
        }
    }

    public virtual void DeallocatePowerRequest(object sender, EventArgs e)
    {
        DoDeallocation();
    }

    public virtual void DoAllocation()
    {
        Debug.Log("Increase Active Level...");
        ActiveLevel = Mathf.Clamp(ActiveLevel + 1, 0, CurrentLevel);
        AllocationChangeNotify();
    }

    public virtual void DoDeallocation()
    {
        ActiveLevel = Mathf.Clamp(ActiveLevel - 1, 0, CurrentLevel);
        AllocationChangeNotify();
    }

    public virtual void IncreaseAllocationPotential()
    {
        CurrentLevel = Mathf.Clamp(CurrentLevel + 1, 0, MaximumLevel);
        AllocationChangeNotify();
    }

    public virtual void DecreaseAllocationPotential()
    {
        CurrentLevel = Mathf.Clamp(CurrentLevel - 1, 0, MaximumLevel);
        AllocationChangeNotify();
    }

    private void AllocationChangeNotify()
    {
        Debug.Log("Active Level Notidy...");
        if (OnAllocationChange == null)
            return;

        SubsystemPowerLevelEventArgs info = new SubsystemPowerLevelEventArgs(CurrentLevel, ActiveLevel, MaximumLevel);
        OnAllocationChange(this, info);
    }

}

public class Relay : MonoBehaviour
{
    public event EventHandler OnRequestPower;
    public event EventHandler OnRefuseRequestPower;
    public event EventHandler OnApprovedRequestPower;

    private int currentLevel = 0;
    private int maxLevel = 0;

    public Relay()
    {
    }

    public void ListenForPowerRequestFrom(Subsystem subsystem)
    {
        subsystem.OnWantPower += RequestPowerFromWarpCore;
    }

    public void RequestPowerFromWarpCore(object sender, EventArgs e)
    {
        // Cancel the power request if the relay is at capacity.
        if (currentLevel == maxLevel)
        {
            RefusePowerRequest(this, e);
            return;
        }

        if (OnRequestPower != null)
        {
            PowerTransferInfoEventArgs powerTransferInfo = (PowerTransferInfoEventArgs)e;
            powerTransferInfo.Relay = this;
            OnRequestPower(this, powerTransferInfo);
        }
    }

    public void ApprovePowerRequest(object sender, EventArgs e)
    {
        PowerTransferInfoEventArgs powerTransferInfo = (PowerTransferInfoEventArgs)e;
        if (powerTransferInfo.Relay == this)
        {
            if (OnApprovedRequestPower != null)
                OnApprovedRequestPower(this, powerTransferInfo);

            IncreaseLevel();
        }
    }

    public void RefusePowerRequest(object sender, EventArgs e)
    {
        PowerTransferInfoEventArgs powerTransferInfo = (PowerTransferInfoEventArgs)e;
        if (powerTransferInfo.Relay == this)
        {
            if (OnRefuseRequestPower != null)
                OnRefuseRequestPower(this, powerTransferInfo);
        }
    }

    private bool IncreaseLevel()
    {
        if (currentLevel < maxLevel)
        {
            currentLevel++;
            return true;
        }
        return false;
    }

    private bool DecreaseLevel()
    {
        if (currentLevel > 0)
        {
            currentLevel--;
            return true;
        }
        return false;
    }
}

public class WarpCore
{
    public EventHandler OnSuccesfulAllocation;
    public EventHandler OnSuccessfulDeallocation;
    public EventHandler OnFailedAllocation;
    public EventHandler OnFailedDeallocation;

    private int absoluteMaximumAllocation = 31;
    private int maximumAllocators = 0;

    private Dictionary<PowerAllocator, Subsystem> allocators = new Dictionary<PowerAllocator, Subsystem>();

    public WarpCore(int maximumPowerCells, int currentUpgradeLevel, int maximumUpgradeLevel)
    {
        for (int i = 0; i < absoluteMaximumAllocation; i++)
        {
            PowerAllocator powerAllocator = new PowerAllocator(i, AllocatorStatus.Hidden);
            allocators.Add(powerAllocator, null);
        }
    }

    public void ListenForRelayPowerRequests(Relay relay)
    {
        relay.OnRequestPower += PowerAllocationRequest;

        //Setup callbacks to the relay
        OnSuccesfulAllocation += relay.ApprovePowerRequest;
        OnFailedAllocation += relay.RefusePowerRequest; 
    }

    public void PowerAllocationRequest(object sender, EventArgs e)
    {
        PowerTransferInfoEventArgs powerTransferInfo = (PowerTransferInfoEventArgs)e;

        if (GetAvailablePowerCell() != null)
        {
            AllocateTo(powerTransferInfo.Subsystem);
            if (OnSuccesfulAllocation != null)
                OnSuccesfulAllocation(this, powerTransferInfo);
        }
        else
        {
            if (OnFailedAllocation != null)
                OnFailedAllocation(this, powerTransferInfo);
        }
    }

    public void IncreaseWarpCoreAllocationPotential()
    {
        foreach (KeyValuePair<PowerAllocator, Subsystem> allocationSet in allocators)
        {
            if (allocationSet.Key.AllocatorStatus == AllocatorStatus.Hidden)
                allocationSet.Key.SetStatus(AllocatorStatus.Unused);
        }
        maximumAllocators++;
    }

    public void AllocateTo(Subsystem subsystem)
    {
        PowerAllocator allocation = GetAvailablePowerCell();
        allocators[allocation] = subsystem;
        allocation.SetStatus(AllocatorStatus.Used);
    }

    // TODO: Add deallocation methods
}

Please, please stop using events for logic. They are fragile, a complete pain to debug, a mess for concurrency, and make reading your code way more difficult.

Thanks for the reply, but it wasn't a very useful comment. Besides using events to notify my classes that things are happening, what would you suggest? I was taught that events should be used when you want to notify other classes of something happening and pass some data using a custom event arg if needed. The only other way I see writing this is if I pass instances of the relay, warp core, and subsystem to each other. Feels over kill when I just need to do a request notification and receive an answer and not needing access to all the members of the rest of the classes.

The only other way I see writing this is if I pass instances of the relay, warp core, and subsystem to each other.

Yes, that seems to be the obvious thing to do.

Feels over kill

Just passing things around is the simplest, most straightforward way to code. It should be your default approach until you have some requirement (there may or may not be something passed in, there may be N things passed in at any given time, there may be things in weird plugins we don't know about).

I don't know your requirements but I would approach it like this:

public interface Subsystem {
        bool IsPowered { get; }
        int MaxPower { get; }
        int PowerLevel { get; }
    }

    public interface PowerSource {
        int SuppliedPower { get; }
        IEnumerable<KeyValuePair<Subsystem, int>> PowerAllocation { get; }
        void AllocatePowerToSubsystem(Subsystem target);
    }

If you can't simply allocate any subsystem to any power source, the Allocate method may need to be in a higher level "Ship" object. Either way, the entire logic for that operation lives in the one method devoted to doing it - not spread across 6 event handlers which provide no benefit.

Thank you, that helped a lot. I adapted your approach to my current requirements and came up with this.

public interface Subsystem
{
    void IncreasePowerLevel();
    bool IsPowered { get; }
    bool IsPowerMaxed { get; }
    int MaximumPowerCapacity { get; }
    int CurrentPowerCapacity { get; }
    int PowerLevel { get; }
    PowerRelay PrimaryPowerRelay { get; }
    PowerRelay SecondaryPowerRelay { get; }
}

public interface PowerRelay
{
    bool IncreaseThroughput(Subsystem target);
    bool IsAtCapacity { get; }
    int MaximumPowerThroughput { get; }
    int CurrentThroughput { get; }
    IEnumerable<KeyValuePair<Subsystem, int>> PowerAllocation { get; }
}

public interface PowerSource
{
    void AllocatePowerToSubsystem(Subsystem target);
    int maximumPowerSupplyCapacity { get; }
    int currentPowerSupplyCapacity { get; }
    int currentPowerSupply { get; }
    IEnumerable<KeyValuePair<Subsystem, int>> PowerAllocation { get; }
}

Whenever the user increases a subsystems power it can only happen as long as it isn't maxed out, the primary/secondary relay feeding it isn't at capacity and the power source has an available block.

My only hangup now is when I call the AllocatePowerToSubsystem(..). Since the power source, relay, and subsystem need to have available power requirements before anything is assigned I need to do some simple checks.

I contemplated doing something like below. I know it would work fine, but in the interest of the single responsibility principle, would it be ok?

public AllocatePowerToSubsystem(Subsystem target)
{
     if(target.IsPowerMaxed == false)
     {
        if(target.PrimaryPowerRelay.IsAtCapacity == false)
        {
           target.IncreasePower();
           target.PrimaryPowerRelay.IncreaseThroughput(target);
           PowerAllocation[target]+= 1;
        }
        else if(target.SecondaryPowerRelay.IsAtCapacity == false)
        {
           target.IncreasePower();
           target.SecondaryPowerRelay.IncreaseThroughput(target);
           PowerAllocation[target]+= 1;
        } 
     }
}

I know it would work fine, but in the interest of the single responsibility principle, would it be ok?

But it has a single responsibility: to allocate power to a subsystem. It knows how to tie the various other single responsibilities together. Something has to do that after all.

It might be good to separate that responsibility from the power source, but I'm not sure the decoupling benefits (flexibility, testability) here are worth the downsides (more overhead, harder to implement/read/maintain).

Thank you Telastyn! Everything seems to work a lot better (not to mention a lot of less lines of code). I never thought about 'tying various other single responsibilities together' as a single responsibility itself. You've been a great help.