Basic traffic light simulation library

Question

I'm planning on completing the April 2017 Community Challenge, Simulate a Multi-Way Intersection. And so I've decided to do this by going in cycles. There are two sections to my code; the generic code, that powers the creation of Delay<T>, and the subclass of Delay<T>, TrafficLightsDelay, which contains the business logic. I personally think this to be a good design, but at least allows me to better understand C# inheritance.

Coming from Python, I don't like having to constantly create new instances of objects, and prefer to use sugar, and so DelayBuilder is largely sugar for creating a List<DelayItem>. Whilst it's not needed I find reading the chained functions simpler to read, rather then lots of object creation.

I personally find DelayBuilder.Build<T> to be a little hack to instantiate the wanted child object of Delay<T>, and would like to know if there is a better way to do this.

I'm unsure on how good Delay<T>.ChangeList is for the overall structure of this code. Is this ok, or should I use another way, possibly one which doesn't use DelayTypes.

Anyway, any and all improvements are welcome. Here is the code:

using System;
using System.Collections.Generic;
using System.Linq;


namespace TrafficLights {
    [Flags]
    internal enum DelayTypes {
        Sleep = 1,
        Change = 2
    }


    class DelayItem {
        public object[] Data;
        public DelayTypes Type;

        public DelayItem(DelayTypes type, object[] data) {
            Type = type;
            Data = data;
        }
    }


    class DelayItemSleep : DelayItem {
        public DelayItemSleep(int amount) : base(DelayTypes.Sleep, new object[] {amount}) { }
    }


    class DelayItemChange : DelayItem {
        public DelayItemChange(object data) : base(DelayTypes.Change, new[] {data}) { }
        public DelayItemChange(object[] data) : base(DelayTypes.Change, data) { }
    }


    class DelayBuilder {
        private readonly List<DelayItem> _list;

        public DelayBuilder() {
            _list = new List<DelayItem>();
        }

        public DelayBuilder Sleep(int amount) {
            _list.Add(new DelayItemSleep(amount));
            return this;
        }

        public DelayBuilder Change(object data) {
            _list.Add(new DelayItemChange(data));
            return this;
        }

        public DelayBuilder Change(params object[] data) {
            _list.Add(new DelayItemChange(data));
            return this;
        }

        public Delay<object> Build() {
            return Build<Delay<object>>();
        }

        public T Build<T>(params object[] args) {
            var arguments = new List<object> {_list};
            arguments.AddRange(args);
            return (T) typeof(T)
                .GetMethod("Build")
                .Invoke(null, arguments.ToArray());
        }
    }


    class Delay<T> {
        protected readonly T _state;
        protected List<DelayItem> _list;

        public Delay(T state, List<DelayItem> list) {
            _list = ChangeList(list);
            _state = state;
        }

        public Delay(T state) {
            _state = state;
        }

        public static Delay<T> Build(List<DelayItem> list, T state) {
            return new Delay<T>(state, list);
        }

        public List<DelayItem> ChangeList(List<DelayItem> list) {
            foreach (var item in list) {
                if (item.Type == DelayTypes.Sleep) {
                    if (item.Data.Length != 1) {
                        throw new ArgumentException("Sleep should only contain a duration");
                    }
                    if (!(item.Data[0] is int)) {
                        throw new ArgumentException("Sleep duration has to be an int");
                    }
                }
            }
            return list;
        }

        internal virtual void Change(object[] data) { }

        internal virtual void Off(object[] data) { }

        public void Start() {
            foreach (var item in _list) {
                if (item.Type == DelayTypes.Sleep) {
                    continue;
                }
                Change(item.Data);
            }
        }

        public void Stop() {
            foreach (var item in _list) {
                if (item.Type == DelayTypes.Sleep) {
                    continue;
                }
                Off(item.Data);
            }
        }

        public IEnumerable<T> Iter(int delay) {
            var time = 0;
            while (true) {
                foreach (var item in _list) {
                    if (item.Type != DelayTypes.Sleep) {
                        Change(item.Data);
                    } else {
                        time += (int) item.Data[0];
                        for (; time > 0; time -= delay) {
                            yield return _state;
                        }
                    }
                }
            }
        }
    }


    [Flags]
    internal enum LightState {
        Off = 0,
        Green = 1,
        Orange = 2,
        Red = 4
    }


    class Light {
        public readonly string Name;
        public LightState Status;

        public Light(string name, LightState status = LightState.Off) {
            Name = name;
            Status = status;
        }
    }


    class TrafficLights {
        public Dictionary<string, Light> Lights;

        public TrafficLights() {
            Lights = new Dictionary<string, Light>();
        }

        public TrafficLights AddLight(string name) {
            return AddLight(new Light(name));
        }

        public TrafficLights AddLight(Light light) {
            if (Lights.ContainsKey(light.Name)) {
                throw new ArgumentException($"Junction already contains light '{light.Name}'");
            }
            Lights[light.Name] = light;
            return this;
        }
    }


    class TrafficLightsDelay : Delay<TrafficLights> {
        public TrafficLightsDelay(List<DelayItem> list, TrafficLights state) : base(state) {
            _list = ChangeList(list);
        }

        public new static TrafficLightsDelay Build(List<DelayItem> list, TrafficLights state) {
            return new TrafficLightsDelay(list, state);
        }

        public new List<DelayItem> ChangeList(List<DelayItem> list) {
            foreach (var item in list) {
                if (item.Type == DelayTypes.Change) {
                    if (item.Data.Length != 2) {
                        throw new ArgumentException("Changes to traffic lights have to have the name and status");
                    }
                    if (!(item.Data[0] is string && _state.Lights.ContainsKey((string) item.Data[0]))) {
                        throw new ArgumentException("Traffic light has to be valid traffic light");
                    }
                    if (!(item.Data[1] is LightState)) {
                        throw new ArgumentException("Traffic light status has to be a LightState");
                    }
                }
            }
            return base.ChangeList(list);
        }

        internal override void Change(object[] data) {
            var light = (string) data[0];
            var status = (LightState) data[1];
            _state.Lights[light].Status = status;
        }

        internal override void Off(object[] data) {
            var light = (string) data[0];
            _state.Lights[light].Status = LightState.Off;
        }
    }


    class Program {
        private static void Main(string[] args) {
            var lights = new TrafficLights()
                .AddLight("North")
                .AddLight("East")
                .AddLight("South")
                .AddLight("West");

            var delay = new DelayBuilder()
                .Change("North", LightState.Green)
                .Change("West", LightState.Red)
                .Sleep(20)
                .Change("East", LightState.Green)
                .Change("North", LightState.Red)
                .Sleep(20)
                .Change("South", LightState.Green)
                .Change("East", LightState.Red)
                .Sleep(20)
                .Change("West", LightState.Green)
                .Change("South", LightState.Red)
                .Sleep(20)
                .Build<TrafficLightsDelay>(lights);

            delay.Start();
            var t = 0;
            foreach (var lightState in delay.Iter(1)) {
                var lightItems = lightState.Lights.Select(i => $"{i.Value.Name}: {i.Value.Status}");
                Console.WriteLine(string.Join(", ", lightItems));
                if (t++ == 100) {
                    break;
                }
            }
            delay.Stop();
            Console.ReadLine();
        }
    }
}

Answer 1

Without a deep understanding of the logic of this challenge here are some general ideas about the code.

---

fields & properties

class DelayItem
{
  public object[] Data;
  public DelayTypes Type;

  public DelayItem(DelayTypes type, object[] data)
  {
      Type = type;
      Data = data;
  }
}

Public mutable fields are generally a bad practice. You should use properties (if possible read-only) (immutable objects are easier to debug):

public DelayTypes Type { get; }

---

inheritance

class DelayItemSleep : DelayItem
{
  public DelayItemSleep(int amount) : base(DelayTypes.Sleep, new object[] { amount }) { }
}


class DelayItemChange : DelayItem
{
  public DelayItemChange(object data) : base(DelayTypes.Change, new[] { data }) { }
  public DelayItemChange(object[] data) : base(DelayTypes.Change, data) { }
}

This inheritance isn't necessary as the derived classes do not add any new functionality. It would be ok to have just the DelayItem. With the inheritance here there is a redundancy because for one the Type already stands for DelayType and secondly the type of the class stands the same too. Lastly base classes are usually abstract because they are not ready to be instantiated yet.

Ideally the DelayItem could be implemented like this:

class DelayItem
{    
    private DelayItem(DelayTypes type, object[] data)
    {
        Type = type;
        Data = data;
    }

    public IEnumerable<object> Data { get; }
    public DelayTypes Type { get; }

    public static DelayItem Create(DelayTypes type,  params object[] data) 
    {
        return new DelayItem(type, data);
    }
}

With a private constructor. With read-only properties and with a factory method Create which can take a various number of data parameters so that you don't need multiple overloads.

---

reflection

public T Build<T>(params object[] args)
{
  var arguments = new List<object> { _list };
  arguments.AddRange(args);
  return (T)typeof(T)
      .GetMethod("Build")
      .Invoke(null, arguments.ToArray());
}

Using reflection is rather a last resort solution. Here you can avoid it by removing the Build method from the Delay. It does not belong to this class. It should be only a responsibility of the builder. On the other hand it's called DelayBuilder but it builds some random T which is very confusing.

Instead you can add a static property to the Delay<T> that will return a new builder and already pass it the T.

public static DelayBuilder<T> Builder => new DelayBuilder<T>();

The builder must be generic for this now

class DelayBuilder<T> {}

and its Build method should only be able to build a Dela<T>

public Delay<T> Build(T state, params object[] args)
{
    return new Delay<T>(state);
}

I couldn't figure out what the other generic Build method does and how to incorporate the args.

---

command & query separation

public IEnumerable<T> Iter(int delay)

This method has unexpected side effects. One could think it only iterates over something by using the passed parameter (for filtering?) but it actually changes the state of the object by calling Change(item.Data);. So it has two responsibilities: updating/setting something and returning the states yield return _state;. Those should be separated. There should be a method for updating the state and another one for returning the states which could be ideally implemented by adding the IEnumerable<T> interface to the class signature. Iter is also not a very good name for a method. Not only it's an abbreviation which are almost never used in C# (unless a well know one) but also it changed the object.

See CommandQuerySeparation

---

builder vs factory

public static Delay<T> Build(List<DelayItem> list, T state)
{
  return new Delay<T>(state, list);
}

In situations like this with factory methods we usually use the Create name because Build suggests it's a builder but it isn't.

---

There is more that can be improved like the public new static TrafficLightsDelay Build that uses the new keyword (I'm pretty sure there is neater solution) but for this I need to find out how it works first ;-)

---

object vs strongly typed

I have one more advice. Try to make everything strongly typed. This means try to get rid of anything of type object. Currently the DataItem has this field/property public object[] Data { get; }. This being of object type has huge implications on the rest of the code. You have to check the parameter types and counts everywhere. Here are a few examples:

if (item.Data.Length != 1)
{
  throw new ArgumentException("Sleep should only contain a duration");
}
if (!(item.Data[0] is int))
{
  throw new ArgumentException("Sleep duration has to be an int");
}

or

time += (int)item.Data[0];

or

if (item.Type == DelayType.Change)
{
  if (item.Data.Length != 2)
  {
      throw new ArgumentException("Changes to traffic lights have to have the name and status");
  }
  if (!(item.Data[0] is string && _state.Lights.ContainsKey((string)item.Data[0])))
  {
      throw new ArgumentException("Traffic light has to be valid traffic light");
  }
  if (!(item.Data[1] is LightState))
  {
      throw new ArgumentException("Traffic light status has to be a LightState");
  }
}

In a perfect solution you should not have to cast anything. Getting rid of the object[] Data and replacing it with real types would allow you to remove all the quoted code above and just check the actual values, not their types or even existence. You could assure that they are defined during object construction already and later assume they are correct.