Should we protect the callback call against null?

[heckert]

One might protect the actions against having a null callback Action at the time the action is completed.

So for example when the spin is completed:

        if (_totalSpinAmount >= 360f)
        {
            _isActive = false;
            _onActionComplete();
        }

if the calling side did

           selectedUnit.GetSpinAction().Spin(null);

instead of

           selectedUnit.GetSpinAction().Spin(ClearBusy);

We would get an error when the action completed.

So instead one could do this:

        if (_totalSpinAmount >= 360f)
        {
            _isActive = false;
            _onActionComplete?.Invoke();
        }

But since we’re not just having a bunch of listeners for an event and if onActionComplete were null all that might happen is in some situation the game would be missing some animation, some sound effect, or similar polishing details, but this time it’s a crucial element of the code’s flow to have the action inform the action system about being done and clearing the “busy” state, here it should be a good thing to have an error pop up if the delegate invocation fails.

[Brian_Trotter]

@heckert

But since we’re not just having a bunch of listeners for an event and if onActionComplete were null all that might happen is in some situation the game would be missing some animation, some sound effect, or similar polishing details, but this time it’s a crucial element of the code’s flow to have the action inform the action system about being done and clearing the “busy” state, here it should be a good thing to have an error pop up if the delegate invocation fails.

This is some sound reasoning. Here’s why I would either let the callback fail with a null reference or log out the error. In the case of onActionComplete, if the Spin() method (this will later be a more generic method name) was passed in a null, then our code is already broken, because the UnitActionSystem will hang and we’ll basically be stuck. Letting the callback fail with an NRE means that we get a Log of the failure, which makes debugging much easier.

My personal preference is to log the fault at the beginning of the action… for example:

public void Spin(Action callback)
{
    if(callback==null) 
    {
         Debug.LogError("Spin called with a null delegate");
    }
    //rest of method
}

Because this gives us some very direct information about where the error might be when the game hangs. Even in a built player in Development mode, you’ll get a clear visual message of what happened.

Otherwise, it would be preferable to let the NRE get called than to null check it (unless you use a verbose error checking like

if(_onActionComplete!=null)
{
    _onActionComplete();
} else
{
     //Log out error
}

Simply using _onActionComplete?.Invoke(); would yield no errors or logs.

Note that this logic only applies in cases where a single callback delegate is intentionally being passed, not in the case of events that are part of the observer pattern. For example, for events like notifying that a unit has changed or an action has changed, etc, if there are no listeners, that’s not something the caller should be concerned with. You’ll still want to null-check, but if there aren’t listeners, this generally isn’t an error so a ?.Invoke() is sufficent.