WPF: should i use string ob StringBuilder with my Log

Question

So i have WPf application and with Log4Netso each time i want to add log i just add it this way:

log4net.Info("bla bla");

In addition i want to add logger form so i created another form and from my main form i opens it this way:

LoggerForm loggerForm = new LoggerForm();
loggerForm.Show();

And create Log object:

public class LogEntry : PropertyChangedBase
{
    public string DateTime { get; set; }
    public int Index { get; set; }
    public string Source{ get; set; }
    public Level Level { get; set; }        
    public string Message { get; set; }
}

And LogHelper that hold this LogEvent objects inside List and also add every LogEvent into this List:

public static class LogHelper
{
    public static ObservableCollection<LogEntry> LogEntries { get; set; }
    public static bool AddLogToList { get; set; }

    private static int Index;

    public static void AddLog(Level level, string message, string source)
    {
        if (AddLogToList)
        {
            Application.Current.Dispatcher.Invoke(new Action(() =>
            {
                if (LogEntries.Count == 1000)
                    LogEntries.RemoveAt(0);

                LogEntry logEntry = new LogEntry()
                {
                    DateTime = DateTime.Now.ToString("yyyy-MM-dd HH:mm:ss,fff"),
                    Index = Index++,
                    Level = level,
                    Source = source,
                    Message = message.Trim()
                };

                LogEntries.Add(logEntry);
            }));
        }
    }
}

And from my Logger form after InitializeComponent register into my list CollectionChanged:

LogHelper.AddLogToList = true;
LogHelper.LogEntries.CollectionChanged += LogEntries_CollectionChanged;

This line:

LogHelper.AddLogToList = true;

Indicate that my Logger form is opened so i can insert my LogEvent iunto my List.

CollectionChanged:

Each time new LogEvent added into my List i update my ItemSource into my ListView:

ListView lvLogger;

private void LogEntries_CollectionChanged(object sender, System.Collections.Specialized.NotifyCollectionChangedEventArgs e)
{
    lvLogger.ItemsSource = LogHelper.LogEntries;
}

Ok so those are my questions:

1. Each time i want to create new Log i type it twice:

   log.Info("bla bla");
   LogHelper.AddLog(Level.Info, "bla bla", $"MyClassName\\MyMethodName"); 
   

   So as you can see i use here string twice, so i wonder if this will better to use maybe String.Builder instead of string ?

I also want to mention that i update my log via different threads.

2. When my Logger form closed i register to its closing event and clear my LogEvent list:

   private void MetroWindow_Closing(object sender, System.ComponentModel.CancelEventArgs e)
   {
       LogHelper.AddLogToList = false;
       LogHelper.LogEntries.Clear();
   }
   

   So my question here is should i unregistered here to my LogEntries_CollectionChanged event:

   LogHelper.LogEntries.CollectionChanged -= LogEntries_CollectionChanged;

Or this is redundant ?

Answer 1

1) Stringbuilder is only worth using if you're repeatedly appending text numerous times. Unless that's what you're doing then I would just use a string. If it is what you're doing then maybe you shouldn't be.

Looking at your code, you already seem to have a message variable so I'm maybe a bit confused by what this part about using it twice. Maybe you're not talking about message though. I would pass in a string to a method and use that variable in both your lines of code there. Assuming they're in the same method.

log.Info(message);
LogHelper.AddLog(Level.Info, message, $"MyClassName\\MyMethodName");

2) I read this part several times and it's confusing. As a general principle though, if you're subscribing to an event from something which isn't a private member then you should unsubscribe to that event in order to obviate any memory leaks. So if the collection is newed up by a different window or something then you should unsubscribe that handler so the instance of that window can be garbage collected if you don't need it any more.

From further explanation in the comments, it's become apparent that you could probably just sort your collection in decreasing datetime and see the latest log entry at the top. This would obviate that collectionchanged event handler completely. Sorting of collectionview: https://social.technet.microsoft.com/wiki/contents/articles/26673.wpf-collectionview-tips.aspx#Sorting

After even more explanation... It seems the text file is for display. I think you should forget the text file entirely and use an observablecollection as a circular list.

You are calling the add method on logentries:

LogEntries.Add(logEntry);

What that does is appends to the collection. You can instead insert at a specific index using insert.

LogEntries.Insert(0, logEntry);

That adds to the "top" of the collection. Which I think would also obviate sorting. You don't want that collection to get huge, so once you hit a number ( say 100 ) then you can remove the oldest.

LogEntries.RemoveItem(100);

Remember to first check you have over a 100 entries or it'll error.

You might want to consider fuller explanation on any further questions. We only know what you tell us and people are unlikely to sit there thinking about what you're doing for long enough to realise... "hey that's all the logs and the file could be quite big".

Answer 2

Using StringBuilder won't make your code any less. If you want less code, put log4net.Info("bla bla"); in LogHelper, or create something that does both jobs a the same time.

StringBuilder is not relevant here as Andy said Stringbuilder is only worth using if you're repeatedly appending text numerous times.

For more information -> String vs Stringbuilder