8
\$\begingroup\$

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 && 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 & 0xFFFF0000) >> 16);
        ushort New = (ushort)(u & 0xFFFF);

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

    }

My personal solution would be:

protected void FireStateChange (LightCommunicationState OldState, LightCommunicationState NewState)
    {
        if (StateChanged != null && 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;
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Raising events on separate threads can become sketchy very quickly. By nature, an event is asynchronous and by nature expects something to be listening when the event finishes propagation. The "fire-and-forget" provided by a new thread is not really in the spirit of an event. What is common is having a long-running process initiated by an event to run on a separate thread so the UI doesn't become unresponsive. What is the reasoning behind this decision? \$\endgroup\$
    – xDaevax
    Commented Aug 12, 2016 at 19:29
  • \$\begingroup\$ @xDaevax except event handlers for a given event run sequentially and synchronously, in an order that cannot really be predetermined. I've edited my answer to add a mention of async/await, but I suspect OP's original code was written before async/await was made available to C#. \$\endgroup\$ Commented Aug 12, 2016 at 19:33

1 Answer 1

13
\$\begingroup\$

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<T> 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<StateChangedEventArgs> StateChanged;

Using such a EventHandler<StateChangedEventArgs> 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<T> 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
{
    /// <summary>
    /// Indicates that the state machine is idle.
    /// </summary>
    NotChoosen,
    /// <summary>
    /// Indicates that the state machine is waiting for a signal.
    /// </summary>
    Waiting,
    /// <summary>
    /// Indicates that the state machine is communicating with a client.
    /// </summary>
    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<T> 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 && OldState != NewState)
{
    // here, another thread could remove the last handler for StateChanged
    StateChanged(...);

The work-around is simple:

var handler = StateChanged;
if (handler != null && 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 => 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 => lambda operator instead: the compiler is able to infer the correct delegate type from the constructor overload.

\$\endgroup\$
9
  • 1
    \$\begingroup\$ Thank you very much, I just wanted to confirm you are right depending on the ThreadStart.Invoke. But a ThreadStart.BeginInvoke()() would solve the problem ;-) - now Im going to Mythbust the Thread Safety Comment :-P \$\endgroup\$
    – Locke
    Commented Aug 12, 2016 at 18:36
  • \$\begingroup\$ TBH the thread safety issue is a race condition edge case; most of the time it doesn't matter, but just to be rigorously thread-safe, that null check is necessary. \$\endgroup\$ Commented Aug 12, 2016 at 18:40
  • \$\begingroup\$ A) Maybe @DeadMG needs to read up more on .NET eventing. B) Maybe the OP should use the TPL for their asynchronicity. \$\endgroup\$
    – user34073
    Commented Aug 12, 2016 at 19:04
  • 2
    \$\begingroup\$ @Hosch250 totally. OP didn't mention why they needed to invoke handlers on a separate thread - in all likelihood what they really want is async handlers. Also with COM interop raising events on another thread than the one it was registered with (assuming handlers on the COM side, since non-EventArgs event delegates are a good thing for COM interop) will completely break the code. \$\endgroup\$ Commented Aug 12, 2016 at 19:07
  • 1
    \$\begingroup\$ @Mat's Mug I wrote some code that forced the race condition, and you are totally right... your solution is thread save, mine is not... Thanks I didn't knew hat. \$\endgroup\$
    – Locke
    Commented Aug 12, 2016 at 19:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.