A Simple State Machine using a Static class in C#

Question

I've been trying to write a simple Static-class State Machine for my application to notify other controls and code when the system state changes. And I think I almost have it, but I'm running into a small issue that I'm not sure how to work around.

Here's the code:

// An enum denoting the 3 States
public enum Status { Error = -1, Working, Ready }

// The main state change class
public static class Sys
{
    // system status
    private static Status state;

    // delegate and event
    public static delegate void StateChangeHandler(object sys, SysInfoEventArgs sysStateInfo);
    public static event StateChangeHandler OnStateChange;

    public static Status State
    {
        get { return state; }
        set
        {
            SysInfoEventArgs sysInfo = new SysInfoEventArgs(state, value);
            state = value;
            OnStateChange(this, sysInfo);
        }
    }
}

/// <summary>Contains previous and current state info</summary>
public class SysInfoEventArgs : EventArgs
{
    public readonly Status oldState;
    public readonly Status newState;
    public SysInfoEventArgs(Status oldState, Status newState)
    {
        this.oldState = oldState;
        this.newState = newState;
    }
}

The problem I am having is with this line:

 OnStateChange(this, sysInfo);

Specifically, the word "this" is illegal. And I understand why: "this" is supposed to refer back to the self of an instantiated object (not a static class).

I would prefer to have a Static class for my state machine rather than one that I can instantiate multiple copies of. (Not that it would be such a bad thing, but I feel it makes the code cleaner having a static class.)

So how am I supposed to work this?

Update:

As a follow-up, I selected Jon Skeet's answer as the correct one because the issue was more about the approach I was taking, rather than a technical failure that I was having. Although, pretty much all of the other answers below fix the technical glitch I was dealing with.

Oddly enough, as I was reviewing with my co-worker the application that I wrote, she pointed out that the program should probably track both the state of the server connection as well as the state of the work being done. (Yes, Virginia, this means I need 2 state machines... Ergo, remove all the "static" keywords from the code above and make it a regular class was the smart approach.)

Thanks again, everyone!

Answer 1

You can't use "this" when you're working within a static scope, such as a static class or a static method.

You have two options here. You can pass null for the "sys" parameter. Really, this parameter, in the case of a static class, is really not useful, since the "sender" is always the static class.

Alternatively, you might want to consider making your state notifier a singleton. This would allow you to have a single instance of a non-static class. This does have the one advantage of making it easier to transition to a non-static implementation if future requirements change, as well.

---

In addition, you really should check to make sure there are subscribers prior to raising this event. Not doing so could cause problems:

public static Status State
{
    get { return state; }
    set
    {
        SysInfoEventArgs sysInfo = new SysInfoEventArgs(state, value);
        state = value;
        var handler = OnStateChange;
        if (handler != null)
            handler(null, sysInfo);
    }
}

Answer 2

I'd change this code:

public static delegate void StateChangeHandler(object sys, SysInfoEventArgs sysStateInfo);
public static event StateChangeHandler OnStateChange;

to:

public static event Action<SysInfoEventArgs> OnStateChange;

Answer 3

Modify your delegate:

from:

public static delegate void StateChangeHandler(object sys, SysInfoEventArgs sysStateInfo);

to:

public static delegate void StateChangeHandler(SysInfoEventArgs sysStateInfo);

Answer 4

Why would you want a static class? It's a state machine - it has state - that naturally suggests using a non-static class. You can always have a static variable referring to a single instance of it if you really want to.

Basically, your instinct is incorrect in my view - having a normal class would make the code cleaner than a static one. Static classes should very rarely have any state at all - perhaps a cache (although even that's dubious), or counters for diagnostic purposes etc. Try to think in terms of objects rather than classes. Does it make sense to have two separate state machines, with a different current state and perhaps different event handlers? It's easy to imagine that's the case - and it means it's easy to create new instances for tests etc. (It also allows independent tests to run in parallel.) Having the state in an instance of the machine is therefore a natural fit.

There are some people who believe there should be no static methods, no static classes etc. I think that's taking it a bit far, but you should always at least consider the testability impact of introducing statics.

Answer 5

If you really want to keep the static class and use the semantics of object sender, then the proper thing to pass would be typeof(Sys). This is also analogous to the (old and rare) locking idiom on a static class.

But that's just being pedantic, because the event handler will never use that value, and in practice null would work just as well.