State Pattern for traffic lights

Question

I tried to implement a traffic light system using state pattern, please comment on the OOP/design pattern use.

using System;
using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace DesignPatternsQuestions
{
    /// <summary>
    /// implement a traffic light system
    /// </summary>
    [TestClass]
    public class StatePatternTest
    {
        [TestMethod]
        public void StateTrafficLightTest()
        {
            TrafficLight light = new TrafficLight(new RedState());
            light.ChangeLight();
            Assert.IsInstanceOfType(light.State,typeof(GreenState));
            light.ChangeLight();
            Assert.IsInstanceOfType(light.State, typeof(YellowState));
            light.ChangeLight();
            Assert.IsInstanceOfType(light.State, typeof(RedState));
            light.ChangeLight();
            Assert.IsInstanceOfType(light.State, typeof(GreenState));
        }
    }

    public class TrafficLight
    {
        private State _state;

        public TrafficLight(State state)
        {
            _state = state;
        }
        // Gets or sets the state

        public State State
        {
            get { return _state; }
            set
            {
                _state = value;
                Console.WriteLine("State: " +
                  _state.GetType().Name);
            }
        }
        //this is the trickS
        public void ChangeLight()
        {
            _state.SetState(this);
        }
    }

    public abstract class State
    {
        public abstract void SetState(TrafficLight light);
    }

    public class RedState: State
    {
        public override void SetState(TrafficLight light)
        {
            light.State = new GreenState();
        }
    }

    public class YellowState : State
    {
        public override void SetState(TrafficLight light)
        {
            light.State = new RedState();
        }
    }

    public class GreenState : State
    {
        public override void SetState(TrafficLight light)
        {
            light.State = new YellowState();
        }
    }
}

Answer 1

Design

This sort of design isn't necessarily a bad thing, but in this example it's making things a lot more complicated, without providing any benefits:

• ChangeLight suggests that null is not a valid state, but your TrafficLight class does not prevent its state from being initialized or set to null.
• It's possible to create classes that inherit from State or one of your other state classes. This essentially means that there are an infinite number of possible states (PurpleState, RandomState, SchrodingerCatState, ...). That makes it unclear how exactly your traffic light is meant to be used.
• Those state classes don't contain any actual state - it's only their type (and functionality) that matters. In that case, you might as well create static instances that can be reused. And because they all contain essentially the same logic (just different values), you don't really need separate classes either.
• Methods like AllowTraffic and StopTraffic(TimeSpan delay) are probably more useful than a ChangeLight method that doesn't say anything about what the next state will be. But supporting such methods with logic inside those state classes alone is cumbersome.

Using a simple enum instead makes things a lot easier: there's no null to worry about and it's immediately clear what all the possible states are. The light-changing logic is fairly simple, so that can easily be kept in TrafficLight itself.

Further considerations

Different countries use different light patterns, and traffic lights may have different modes (such as daylight versus night, or an error state). In other words: red is not always followed by green - in some countries there's an orange-red state in-between, in some modes a light is flashing red continually, and so on.

With such requirements, the current light doesn't tell us everything about the traffic light's internal state. It would be more useful to treat each pattern as a different state. But even then I wouldn't create separate state classes with their own logic: a single data-driven class and some state-tracking logic inside TrafficLight itself should be sufficient.

Other notes

• That Console.WriteLine is obviously some left-over test code, but be careful not to leave it lingering around. There's no reason why this code should depend on the presence of a console. If you're using Visual Studio, you may want to use tracepoints instead (breakpoints with a logging action). They're more flexible and don't clutter the code.
• Comments like // Gets or sets the state are of no use: they're just stating the obvious, and they can only cause confusion when they become out-of-date.
• State is a very generic name - TrafficLightState or TrafficLightColor may be more appropriate here.

Answer 2

Pieter's answer already covers most of the important things, I just want to add a small bit (that's why it's a community wiki instead of a comment) about the logic behind a state machine for a complex system.

Imagine to describe the health state of someone: you don't list all the symptoms or the visible outcomes, you just say "flu" (for example). This is a simplification but I hope it helps to understand the point: instead of TrafficLightColor the state should describe what it is, not the visible outcome (light is red).

For example (better names are highly recommended): AboutToStopTraffic, StopTraffic, AboutToAllowTraffic and AllowTraffic (if these are the states your traffic light system can manage).

Outcomes (color combination and/or other things like traffic cameras' state) are derived by the state but they're not the state (even if sometimes they may coincide).

The main advantage is the the derived outcomes can be configured separately and independently from the main immutable logic. If, for example, in one country there is not a separate light configuration between stop and go then your logic does not need to be updated in a per-country base.

Answer 3

First, I see no reason to make the setter of TrafficLight.State public, as there's no reason the client should be able to change it directly.

Second, your solution looks a bit too complicated to me. As long as you're talking about traffic lights where the number of possible states is finite and should never change, I wouldn't make theState property an object, but rather an enum. The way you defined it allows the client to call light.State.SetState(anotherInstanceOfLight), which can result in an undesired state (that is, the state of the system, not of a specific traffic light)

Answer 4

Like others stated, using the state pattern for this problem is overkill. But when you decide to use it, make the following considerations.

Events ChangeLight received by the state machine TrafficLight should be forwarded to the state. Your implementation stipulates the state machine decides what the state must do. You have actually implemented a visitor pattern, rather than a state pattern.

public void ChangeLight()
{
    _state.SetState(this);  // <- this is a visitor pattern
}

 public void ChangeLight()
 {
     _state.ChangeLight(this);  // <- let the state handle the event
 }

 public class RedState: State
 {
     public override void ChangeLight(TrafficLight light)
     {
         // Each state decides itself what to do, rather than the traffic light
         // telling them what to do; it just happens to be so that whatever the 
         // traffic light wants the state to do (in your implementation) is what
         // each of these states want to do.
         light.State = new GreenState();
     }
 }