Thread safe log class which creates log files according to the current date when entry was added

Question

From this question and some help I created a thread safe logging class on which the name of the log file is same as the current date (short date) when the log entry was added. e.g. If I create ten entries today say they will all get written to log file named "07.10.2015.txt". I want to use this class from DLL. I am curious on your opinion if this is reasonable and thread safe class?

public static class Logger
{
    static readonly object _syncObject = new object();

    public static void Write(string logMessage)
    {
        try
        {
            var todaysLogFilePath = Path.Combine(ConfigManager.GetAppSetting("logPath"), 
                                                    ("Log" + DateTime.Now.ToString("yyyy-MMMM-dd") + ".txt"));
            Log(logMessage, todaysLogFilePath);
        }
        catch (IOException e)
        {
            //log somewhere else if required
        }
    }

    static void Log(string logMessage, string logFilePath)
    {
        lock (_syncObject)
        {
            File.AppendAllText(logFilePath, 
                        string.Format("{0} {1} {2}", 
                        DateTime.Now.ToLongTimeString(), DateTime.Now.ToLongDateString(), logMessage));
        }
    }
}

PS. I don't want third party classes because they have some requirements sometimes if used from DLL which is my requirement, and in some cases I don't know directly how to enable in those classes ability to name the log entries same as the current date when log was added.

Answer 1

static readonly object _syncObject = new object();  

You should always add a scope identifier because it increases readability.

var todaysLogFilePath = Path.Combine(ConfigManager.GetAppSetting("logPath"), 
                                        ("Log" + DateTime.Now.ToString("yyyy-MMMM-dd") + ".txt"));
Log(logMessage, todaysLogFilePath);  

Each time the Write() method is called you are

• reading from the config to get the logging path
• construct a new filepath

I know why you are doing this, but it could be done in a better way. You could add a field

private static DateTime currentDate = DateTime.Now.Date; 

so it is initialized at startup.

Next you could add a field

private string logFilePath;

and a method ComposeLogFilePath(DateTime) like so

private static string ComposeLogFilePath(DateTime date)
{
    return Path.Combine(ConfigManager.GetAppSetting("logPath"), 
                             ("Log" + date.ToString("yyyy-MMMM-dd") + ".txt"));
}  

which you then call from this constructor

static Logger()
{
    logFilePath = ComposeLogFilePath(currentDate);
}  

By adding a method which checks if a change in the date has happened and if a new logFilePath needs to be composed like so

private static void CheckLogFileAndCreateIfNeeded()
{
    if (currentDate >= DateTime.Now.Date) { return; }  

    currentDate = DateTime.Now.Date;
    logFilePath = ComposeLogFilePath(currentDate);
} 

The method name is not the best, maybe you or some answerer will come up with a better one.

So each time the Write() method is called you call this method like so

public static void Write(string logMessage)
{
    try
    {
        CheckLogFileAndCreateIfNeeded();

        Log(logMessage, logFilePath);
    }
    catch (IOException e)
    {
        //log somewhere else if required
    }
}  

which leads to the next point: Usually a Logger will log something so better change the names Write()-> Log() and Log() -> Write().

static void Log(string logMessage, string logFilePath)
{
    lock (_syncObject)
    {
        File.AppendAllText(logFilePath, 
                    string.Format("{0} {1} {2}", 
                    DateTime.Now.ToLongTimeString(), DateTime.Now.ToLongDateString(), logMessage));
    }
}  

Here you should store DateTime.Now inside a variable to have the calls ToLongTimeString() and ToLongTimeString() using the same DateTime like so

static void Log(string logMessage, string logFilePath)
{
    lock (_syncObject)
    {
        DateTime date = DateTime.Now;
        File.AppendAllText(logFilePath, 
                    string.Format("{0} {1} {2}", 
                    date.ToLongTimeString(), date.ToLongDateString(), logMessage));
    }
}

Threadsafety is no issue here because of the lock but you could do this in a much better way by having a ConcurrentQueue in which you store the formatted message to log.

Then add a method to query the queue which ideally should spawn a thread to do the work. If there is already a thread running, you can just return from this method. If there is no thread running, start it and return.

By using a ConcurrentQueue you could get rid of the lock and you wouldn't need for each message to open the file.