0
\$\begingroup\$

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.

\$\endgroup\$
2
\$\begingroup\$
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.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ I will look through your comments but please bear in mind I am beginner in .NET and don't give too advanced feedback. I will check your comments bit later. \$\endgroup\$ – user86187 Oct 7 '15 at 9:16
  • \$\begingroup\$ What is the problem with the way I create files? I think it is easier to understand. It doesn't work you think? In my way if there is no file it will be created, if there is one, data will be apended \$\endgroup\$ – user86187 Oct 8 '15 at 7:18
  • \$\begingroup\$ Let's say how you suggested composeLogFilePath etc. To sum up my approach was also working, just you suggested better performance? Or optimization did I get right? But functionally my approach is also working? \$\endgroup\$ – user86187 Oct 8 '15 at 8:18
  • \$\begingroup\$ I understand but it maybe instead of config I will just pass the main path as property \$\endgroup\$ – user86187 Oct 8 '15 at 10:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy