Start an EventHandler in different Thread I just stumbeled accross some code and I would do it in a different way. Since I am quite new to programming, I just wanted to know if I understand it right.

Original Code:


protected void FireStateChange(LightCommunicationState OldState, LightCommunicationState NewState)

    {
        if (StateChanged != null &amp;&amp; OldState != NewState)
        {
            Thread t = new Thread(StateChangeThread);
            uint u = ((uint)OldState * 0x10000) + (uint)NewState;
            t.Start(u);
        }
    }

    protected void StateChangeThread(object o)
    {
        uint u = (uint)o;
        ushort Old = (ushort)((u &amp; 0xFFFF0000) &gt;&gt; 16);
        ushort New = (ushort)(u &amp; 0xFFFF);

        StateChanged(this, (LightCommunicationState)Old, (LightCommunicationState)New);

    }



My personal solution would be:

protected void FireStateChange (LightCommunicationState OldState, LightCommunicationState NewState)
    {
        if (StateChanged != null &amp;&amp; OldState != NewState)
        {
            new ThreadStart(
                delegate
                {
                    StateChanged(this, OldState, NewState);
                }).Invoke();
        }
    }


It seems a bitshift and cast an object to a uInt back an forth looks freaky but is unnecessary. Maybe I am missing some benefits on the original solution?

The LightCommunicationState enum is defined as follows:

public enum LightCommunicationState: ushort
{
    notChoosen, Waiting, Communicating
}


And the StateChanged event like this:

public delegate void LightCommunicationStateChanged(LightCommunicationStateMachine Sender, LightCommunicationState OldState, LightCommunicationState NewState);

public event LightCommunicationStateChanged StateChanged;
Don't reinvent the wheel: use EventArgs

The StateChanged event isn't compliant with established C# conventions for creating events.


public delegate void LightCommunicationStateChanged(LightCommunicationStateMachine Sender, LightCommunicationState OldState, LightCommunicationState NewState);
public event LightCommunicationStateChanged StateChanged;



That is non-standard. C# conventions recommend using an EventHandler&lt;T&gt; delegate, where T is a type derived from System.EventArgs - the type could be as simple as this:

public class StateChangedEventArgs : EventArgs
{
    public StateChangedEventArgs(LightCommunicationState oldState, LightCommunicationState newState)
    {
        _oldState = oldState;
        _newState = newState;
    }

    private readonly LightCommunicationState _oldState;
    public LightCommunicationState OldState { get { return _oldState; } }

    private readonly LightCommunicationState _newState;
    public LightCommunicationState NewState { get { return _newState; } }
}


In C# 6 it's even simpler - you can implement an actual immutable type with auto-properties:

public class StateChangedEventArgs : EventArgs
{
    public StateChangedEventArgs(LightCommunicationState oldState, LightCommunicationState newState)
    {
        OldState = oldState;
        NewState = newState;
    }

    public LightCommunicationState OldState { get; }
    public LightCommunicationState NewState { get; }
}


By encapsulating the event data inside an EventArgs class, you avoid needing to change the delegate signature (and all the cascading modifications it entails), should the event data ever need to change:

public event EventHandler&lt;StateChangedEventArgs&gt; StateChanged;


Using such a EventHandler&lt;StateChangedEventArgs&gt; delegate for the StateChanged event makes all handler signatures look like this:

private void HandleStateChanged(object sender, StateChangedEventArgs e)


The day business requirements change and you need to add some DateTime Timestamp property to StateChangedEventArgs, none of the handlers' signature needs to change to keep the code compilable.

Asynchronous?

Note that using a custom delegate for events does have its use: generics cannot be exposed through COM interop, therefore a public EventHandler&lt;T&gt; member can't be exposed if the event needs to be handled on the COM side, so a custom delegate is required. But then, invoking a handler on a different thread than the thread it was registered with, will consistently result in a null handler on the .net side.

Hence, I suspect what you really wanted was async handlers - something like this:

private async void HandleStateChanged(object sender, StateChangedEventArgs e)
{
    await DoSomething(e.OldState, e.NewState);
}


Possibly the code was written before .net 4.5 when async/await wasn't available; consider re-targeting the project to a more recent .net version and replacing that threading code with async/await.



Non-Standard Enum

The LightCommunicationState enum isn't standard either:


public enum LightCommunicationState: ushort
{
    notChoosen, Waiting, Communicating
}



Enum values are public members, and should be named as such, in PascalCase. It's also easier to follow and eventually decorate the members with XML-docs and attributes when the members are on their own line, for example:

public enum LightCommunicationState
{
    /// &lt;summary&gt;
    /// Indicates that the state machine is idle.
    /// &lt;/summary&gt;
    NotChoosen,
    /// &lt;summary&gt;
    /// Indicates that the state machine is waiting for a signal.
    /// &lt;/summary&gt;
    Waiting,
    /// &lt;summary&gt;
    /// Indicates that the state machine is communicating with a client.
    /// &lt;/summary&gt;
    Communicating
}


There's seldom a valid reason to change the underlying type of an enum. Seems the ushort underlying type was chosen only to play nicely with the bit-shifting, but then the enum values aren't explicitly assigned, which I believe means adding a 4th value would have broken the logic - the 3 values are implicitly 0, 1 and 2 which happen to match the first few powers of 2, but the next value would be 3 when the next power of 2 would be 4... but that's all pointless: the whole bit-shifting logic was apparently only there to cram both oldState and newState into a single value, boxed into an object, to match the ParameterizedThreadStart delegate signature - which is ugly at best, and irrelevant given a proper EventHandler&lt;T&gt; event.



ThreadStart.Invoke

I don't think your solution is equivalent. ThreadStart is just a delegate - invoking it won't automagically run it on a separate thread, you need a Thread object somewhere!

Per the ParameterizedThreadStart topic on MSDN, the original code had it right. I believe your version simply runs your inline delegate (which merely invokes the handler) on the very same thread FireStateChanged is running on.



Thread Safety

An event reference is null whenever it has no registered handlers, and handlers can be added and removed at any time, by any thread. In both the original code and your own version, the event invocation isn't thread-safe:

if (StateChanged != null &amp;&amp; OldState != NewState)
{
    // here, another thread could remove the last handler for StateChanged
    StateChanged(...);


The work-around is simple:

var handler = StateChanged;
if (handler != null &amp;&amp; oldState != newState)
{
    // here, the handler reference is thread-local and can't ever be null
    handler.Invoke(...);


In C# 6 you can use the null-conditional operator directly on the event, instead of making a thread-local copy and explicitly checking for handler != null:

if (oldState != newState)
{
    StateChanged?.Invoke(...);


Notice I changed OldState and NewState to oldState and newState, respectively: parameters and local variables should be camelCase, not PascalCase (notice even the syntax highlighting picks it up!).

Also - and that's completely a personal preference - I prefer seeing an explicit .Invoke call on the event, rather than writing it to look like a standard method call - IMO the .Invoke makes a clear visual cue that we're invoking an event, not just another method.



My solution might have looked something like this:

protected void OnStateChanged(LightCommunicationState oldState, LightCommunicationState newState)
{
    var handler = StateChanged;
    if (handler == null || oldState == newState)
    {
        return;
    }

    var args = new StateChangedEventArgs(oldState, newState);
    var thread = new Thread(e =&gt; handler.Invoke(this, (StateChangedEventArgs)e));
    thread.Start(args);
}


Notice I named the method On[EventName] instead of Fire[EventName] - that's also a C# naming convention, see C# Naming Guidelines on MSDN. Note the cast from object to StateChangedEventArgs incurs no boxing, since only reference types are involved.

Also notice I'm making use of the compiler's type inference capabilities: you don't need to explicitly new up a ThreadStart delegate, using the =&gt; lambda operator instead: the compiler is able to infer the correct delegate type from the constructor overload.