C# static class holding list of member class instances

Question

I was wondering if such a design was a bad idea in C#. Here I have a static class EventLog which holds a list of instances of Event class. This is a simple event log of actions players can do in a game. What can I be doing in a better way?

public class EventLog
{
    private const int maxEventsInLog = 50;
    private static List<Event> events = new List<Event>();
    private static string[] eventDescriptions = new string[8]
    {
        "<fromUnit> walked",
        "<fromUnit> waited",
        "<fromUnit> inquired",
        "<fromUnit> allured <toUnit>",
        "<fromUnit> persuaded <toUnit>",
        "<fromUnit> bribed <toUnit>",
        "<fromUnit> hired <toUnit>",
        "<fromUnit> attacked <toUnit>",
    };

public static void AddEvent(GameObject _fromUnit, GameObject _toUnit, Utils.ActionType _actionType)
{
    events.Add(new Event(_fromUnit, _toUnit, _actionType));

    if (events.Count > maxEventsInLog)
    {
        while (events.Count > maxEventsInLog)
        {
            events.RemoveAt(0);
        }
    }
}

public static int NumEventsInLog()
{
    return events.Count;
}

public static string GetEventDescription(int eventIndex)
{
    if (eventIndex < 0 || eventIndex > events.Count - 1)
        return null;

    return events[eventIndex].description;
}

private class Event
{
    private GameObject fromUnit;
    private GameObject toUnit;
    private Utils.ActionType actionType;
    public string description;

    public Event(GameObject _fromUnit, GameObject _toUnit, Utils.ActionType _actionType)
    {
        fromUnit = _fromUnit;
        toUnit = _toUnit;
        actionType = _actionType;

        description = eventDescriptions[(int)_actionType];
        description = description.Replace("<fromUnit>", _fromUnit.GetComponent<UnitData>().firstName + " " + _fromUnit.GetComponent<UnitData>().lastName);
        description = description.Replace("<toUnit>", _toUnit.GetComponent<UnitData>().firstName + " " + _toUnit.GetComponent<UnitData>().lastName);
    }
}

Answer 1

I think your tokens <fromUnit> and <toUnit> should be public static readonly string fields, instead of being hard-coded the way they are. This turns eventDescriptions into something like this:

private static string[] eventDescriptions = new string[8]
{
    FromUnitToken + " walked",
    FromUnitToken + " waited",
    FromUnitToken + " inquired",
    FromUnitToken + " allured " + ToUnitToken,
    FromUnitToken + " persuaded " + ToUnitToken,
    FromUnitToken + " bribed " + ToUnitToken,
    FromUnitToken + " hired " + ToUnitToken,
    FromUnitToken + " attacked " + ToUnitToken,
};

And then the Event constructor can do something like this:

    description = description.Replace(EventLog.FromUnitToken, _fromUnit.GetComponent<UnitData>().firstName + " " + _fromUnit.GetComponent<UnitData>().lastName);
    description = description.Replace(EventLog.ToUnitToken, _toUnit.GetComponent<UnitData>().firstName + " " + _toUnit.GetComponent<UnitData>().lastName);

The description field is problematic though:

public string description;

Instance fields shouldn't be public - that's breaking encapsulation, anyone that can use an Event instance can do whatever they want with that value. Better encapsulate it:

private readonly string description;
public string Description { get { return description; } }

Fields initialized from constructor should be marked readonly if that's the intent - it makes the intent clearer.

---

In AddEvents, you're adding events, and then removing whatever exceeds your max capacity:

events.Add(new Event(_fromUnit, _toUnit, _actionType));

if (events.Count > maxEventsInLog)
{
    while (events.Count > maxEventsInLog)
    {
        events.RemoveAt(0);
    }
}

Looks like you want some First In, First Out data structure - a List just isn't practical for that - consider using a Queue<Event> instead.

Answer 2

• I find it a little odd that you use underscores in your parameter names, sometimes. I guess it would be okay if you were consistent about it, but you weren't. On the other hand, I'm just not accustomed to seeing underscores used in parameter names. Whether to use underscores or this for private fields is holy war territory, but I think most people would agree that they don't expect to see them the way you used them.

• This is kind of awkward too.

  public static int NumEventsInLog()
  

  I think EventCount would be plenty clear and a good alternative.

• I think both your Event class and the (unknown) GameObject may benefit from overriding the ToString() method. Use a combination of properties and a ToString method that properly builds and formats the string to replace the public description field.