4
\$\begingroup\$

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();
        }
    }
}
\$\endgroup\$
1
  • \$\begingroup\$ I think State class is overkill here when simple enum or even declared int constants could be used. I would also consider 2 other states: None (with a value of 0) for when all lights are off or malfunctioning, and BlinkingRed. Where I live there is also blinking yellow left turn arrows to consider. \$\endgroup\$ – Rick Davin Sep 10 '18 at 14:47
7
\$\begingroup\$

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.
\$\endgroup\$
2
  • \$\begingroup\$ Thank you very much. I thought the whole point of state pattern is thr abstract class of state but i will try to implement with an enum as well. Thanks. \$\endgroup\$ – Gilad Sep 11 '18 at 8:51
  • \$\begingroup\$ The way I see it is that the state pattern is more about providing different behavior for different states. For example, a traffic light control system could have different modes: normal, where each lane is given the same amount of time, rush-hour, where certain lanes are prioritized, or maintenance mode, where lights are disabled or manually controlled. Each mode has its own logic and maybe mode-specific state. That seems like a good use-case for this pattern. \$\endgroup\$ – Pieter Witvoet Sep 11 '18 at 11:42
4
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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)

\$\endgroup\$
1
  • \$\begingroup\$ OP's solution is too complicate, IMHO, because he didn't pick the right names and he didn't set clear responsibilities. Outside very simple cases (which this might be or not) a state machine for traffic lights is a good use-case (think about time, calls for pedestrians, traffic conditions and so on...) \$\endgroup\$ – Adriano Repetti Sep 10 '18 at 15:03
1
\$\begingroup\$

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();
     }
 }
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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