C# static class holding list of member class instances 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&lt;Event&gt; events = new List&lt;Event&gt;();
    private static string[] eventDescriptions = new string[8]
    {
        "&lt;fromUnit&gt; walked",
        "&lt;fromUnit&gt; waited",
        "&lt;fromUnit&gt; inquired",
        "&lt;fromUnit&gt; allured &lt;toUnit&gt;",
        "&lt;fromUnit&gt; persuaded &lt;toUnit&gt;",
        "&lt;fromUnit&gt; bribed &lt;toUnit&gt;",
        "&lt;fromUnit&gt; hired &lt;toUnit&gt;",
        "&lt;fromUnit&gt; attacked &lt;toUnit&gt;",
    };

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

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

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

public static string GetEventDescription(int eventIndex)
{
    if (eventIndex &lt; 0 || eventIndex &gt; 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("&lt;fromUnit&gt;", _fromUnit.GetComponent&lt;UnitData&gt;().firstName + " " + _fromUnit.GetComponent&lt;UnitData&gt;().lastName);
        description = description.Replace("&lt;toUnit&gt;", _toUnit.GetComponent&lt;UnitData&gt;().firstName + " " + _toUnit.GetComponent&lt;UnitData&gt;().lastName);
    }
}
I think your tokens &lt;fromUnit&gt; and &lt;toUnit&gt; 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&lt;UnitData&gt;().firstName + " " + _fromUnit.GetComponent&lt;UnitData&gt;().lastName);
    description = description.Replace(EventLog.ToUnitToken, _toUnit.GetComponent&lt;UnitData&gt;().firstName + " " + _toUnit.GetComponent&lt;UnitData&gt;().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 &gt; maxEventsInLog)
{
    while (events.Count &gt; 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&lt;Event&gt; instead.