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.

• Phillip Copley: This is a nice, comprehensive overview. I like a lot of the advice here, but I can't help but think there's a better way to do this without using .Replace() for the tokens. I don't have any idea what that would be off the top of my head, however.
• Mathieu Guindon: You could use a regex replace, but I believe the string method is going to be faster.
• RubberDuck: What about String.Format? That was my first thought when I saw this, but I wasn't sure how it would deal with being passed more arguments than it had tokens to replace.

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.

• Mathieu Guindon: There is no naming convention I've heard of, that uses an underscore for parameter names. Underscore is typically used as a prefix for private fields.
• craftworkgames: One of the best ways I've found to get into the habit of good naming conventions is to install Resharper and do as it suggests. The code will be much cleaner and consistent. Resharper is a commercial product, but it has a 30 day trial and is awesome enough to buy in my opinion.
• Mathieu Guindon: @craftworkgames damn right!! Careful with suggestions though - they're suggestions, not law.
• craftworkgames: @Mat'sMug Yes of course, as you get used to it you'll customize the suggestions to your liking. Resharper does a pretty good job out of the box though.