Implementing a thread safe log class with simple functionality

I have finally assembled an attempt to create a thread safe logging class and also ensured by file flags that file should not be accessed by different processes at same time. This classed will be inside DLL. Below is the implementation,

static class GMLogger
    {

       private static List _buffer = new List();
       private static int _maxBufferSize = 25;  // number of messages  permitted in the buffer at a time
       public static string _logDir = "C:\\Logs";
       private static readonly object _syncObject = new object();

       // Log message
        public static  void Log(string logMessage)
        {
            try
            {
                lock (_syncObject)
                {
                    _buffer.Add(logMessage);

                    Save(_buffer);
                }
            }
            catch (Exception ex)
            {
                throw;
            }
        }

       // Save buffer if needed
        private static void Save(List buffer)
        {
            if (buffer.Count > _maxBufferSize)
            {
                // Write to file
                if (!_logDir.EndsWith("\\")) _logDir += "\\"; 
                DirectoryInfo di = Directory.CreateDirectory(_logDir);
                var todaysLogFilePath = Path.Combine(_logDir,
                                                       ("Log" + DateTime.Now.ToString("yyyy-MMMM-dd") + ".txt"));
                using (FileStream aFile = new FileStream(todaysLogFilePath, FileMode.Append, FileAccess.Write, FileShare.None))
                using (StreamWriter sw = new StreamWriter(aFile))
                {
                    for (int i = 0; i < buffer.Count(); i++)
                    {
                        sw.WriteLine(buffer[i]);
                    }
                }

                // Clear buffer
                buffer.Clear();
            }

    }
}

Please note I am a beginner first of all, and if this class achieves the simple functionality of writing simple lin

One of the things you can do to get some performance back (albeit not a lot, but still more than none) is rewrite Save to have one responsibility.

As it stands now, Save has several responsibilities. It's responsible for:

• Checking if the buffer is of sufficient length to be saved.

• Validating that _logDir ends with a \.

• Creating the _logDir directory.

• Coming up with the filename for the log file.

• Saving the buffer to disk.

• Clearing the buffer after saving.

The Save method should only be responsible for saving (and maybe clearing) the buffer. If you move the if check outside to the Log method, you remove a (minor) bit of overhead.

A better style would be to create a method SaveIfFull which does the if-check, then calls Save which actually saves the buffer.

You have a:

public static string _logDir = "C:\\Logs";

Which is generally frowned upon. The best-practice for public members is PascalCase with no leading characters. So, instead, it should be:

public static string LogDir = "C:\\Logs";

Never omit braces from if statements, and never one-line them. Adding braces won't prevent bugs, but it will help prevent bugs.

if (!_logDir.EndsWith("\\")) _logDir += "\\";

Should be rewritten as:

if (!_logDir.EndsWith("\\"))
{
    _logDir += "\\";
}

Likewise, you should make _logDir (which we to LogDir) a property, with a private backing field.

private static string _logDir = "C:\\Logs\\";
public static string LogDir
{
    get
    { 
        return _logDir;
    }
    set
    { 
        if (string.IsNullOrWhiteSpace(value))
        { 
            throw new ArgumentException("The specified value must not be null, whitespace, or an empty string.", "value");
        }

        _logDir = value;

        if (!_logDir.EndsWith("\\"))
        {
            _logDir += "\\";
        }
    }
}

You can also add other validation logic in there as you need, but this way it helps separate responsibilities further.

To get rid of some of the other additional responsibilities Save has, we can consider a method GetLogFileName(DateTime loggedAt) which would return the filename, then Save would call string todayFileName = GetLogFileName(DateTime.Now) or string todayFileName = GetLogFileName(DateTime.UtcNow) as appropriate.

If you have C#6.0 a few of these tasks become easier/simpler:

• Path.Combine(_logDir, ("Log" + DateTime.Now.ToString("yyyy-MMMM-dd") + ".txt")); simplifies to: Path.Combine(_logDir, $"Log{DateTime.Now.ToString("yyyy-MMMM-dd")}.txt");

• You can replace "value" at the end of ArgumentException with nameof(value). (Which allows you to refactor that message cleanly.)

As far as your primary concerns:

• Yes, the idea of the lock is to mark a shared resource as "in-use" so that threads know to wait on each other before they can proceed. You could, however, replace it with a bool instead of an object for smaller overhead.

• You could have performance implications by the lock, but that's the downside of multithreading: somewhere you will have a bottleneck from context-switching, et al.

• That is beyond the scope of Code Review.

Another concern: if there is a substantial delay between messages, it may occur that the log is never written to before the application is closed (or messages are left in the buffer), this can be alleviated by scheduling a Timer to save the log at periodic intervals as well, so that if there is a substantial delay the log is saved anyway.

StackExchange Code Review Q#108280, answer score: 7