'Checking delegates for null

I was reading the Essential C# 3.0 book and am wondering if this is a good way to check delegates for null?:

class Thermostat
{
    public delegate void TemperatureChangeHandler ( float newTemperature );

    public TemperatureChangeHandler OnTemperatureChange { get; set; }

    float currentTemperature;

    public float CurrentTemperature
    {
        get { return this.currentTemperature; }
        set
        {
            if ( currentTemperature != value )
            {
                currentTemperature = value;

                TemperatureChangeHandler handler = OnTemperatureChange;

                if ( handler != null )
                {
                    handler ( value );
                }
            }
        }
    }
}

Does the solution changes if the type is immutable? I figured maybe with immutability you wouldn't run into this threading problem.

c++sharp.netdelegates


Solution 1:[1]

Use a question mark for a conditional access:

OnTemperatureChange?.Invoke();

Solution 2:[2]

There is a reason the code you've given is recommended over C. Ross's version. However, John is also right that there is still another problem if an event is unregistered in the meanwhile. The blog I linked recommends that the handler ensure they can be called even after being unregistered.

Solution 3:[3]

First, you aren't actually publishing an event - so at the moment, your code is "at risk" of people messing it up completely. It should be:

public event TemperatureChangeHandler CurrentTemperatureChanged;

The name "CurrentTemperatureChanged" is important for data-binding (there is a convention that the runtime uses - given a property Foo, it will look for FooChanged). However, IMO this should just be regular EventHandler. Data-binding will look for EventHandler, but more importantly: you aren't actually giving any information in the event that the subscriber can't already get just by looking at obj.CurrentTemperature.

I'll give the rest of the answer in terms of TemperatureChangeHandler, but I would encourage you (again) to switch to EventHandler:

public event EventHandler CurrentTemperatureChanged;

The approach:

TemperatureChangeHandler handler = CurrentTemperatureChanged;
if(handler != null) handler(value);

is reasonable, but (as per other replies) there is a slim risk of callers that think they disconnected getting the event. Unlikely in reality.

Another approach is an extension method:

public static class TemperatureChangeExt {
    public static void SafeInvoke(this TemperatureChangeHandler handler,
             float newTemperature) {
        if (handler != null) handler(newTemperature);
    }
}

Then in your class you can just use:

        if (currentTemperature != value) {
            currentTemperature = value;
            CurrentTemperatureChanged.SafeInvoke(value);
        }

Solution 4:[4]

If the Thermostat class doesn't need to be thread safe then yes the above code is fine - as long as there is only one thread accessing that instance of Thermostat there is no way for OnTemperatureChange to become unregistered between the test for null and the call to the event.

If you need to make Thermostat thread safe then you might want to take a look at the following article (new to me, looks like a good read):

http://www.yoda.arachsys.com/csharp/events.html

For the record, the recommendation is that you develop your classes not to be thread safe unless thread safety is explicitly needed as it can significantly increase the complexity of your code.

Solution 5:[5]

I just see a bit of refactoring that could be done but otherwise it looks good...

class Thermostat
{
    public delegate void TemperatureChangeHandler ( float newTemperature );

    public TemperatureChangeHandler OnTemperatureChange { get; set; }

    float currentTemperature;

    public float CurrentTemperature
    {
        get { return this.currentTemperature; }
        set
        {
                if (currentTemperature != value)
                {
                        currentTemperature = value;

                        if (this.OnTemperatureChange != null )
                        {
                                this.OnTemperatureChange.Invoke( value );
                        }
                }
        }
    }
}

Sources

This article follows the attribution requirements of Stack Overflow and is licensed under CC BY-SA 3.0.

Source: Stack Overflow

Solution Source
Solution 1 General Grievance
Solution 2 Matthew Flaschen
Solution 3 Marc Gravell
Solution 4
Solution 5 bytebender

Related Questions