Locking mechanism in C#

Question

I would like you to review the locking mechanism I implement in C# class. Will it be working correctly with many threads? Please don't mind rest of the code because I just try to repair the class quickly to have a thread safe logging class for my project. Any other suggestions are welcome.

public static class Debug
{

    static readonly object _consoleLocker = new object();
    static readonly object _diskDriveLocker = new object();

    public static void Log(Tryb tryb, int level, string caller, string message, string messageExt)
    {
        String strAppDir = Path.GetDirectoryName(Assembly.GetExecutingAssembly().GetName().CodeBase);
        strAppDir = strAppDir.Replace("file:\\", ""); // dla windowsa
        strAppDir = strAppDir.Replace("file:", "");  // dla linuksa
        string file = Path.Combine(strAppDir, "log.txt");
        ProcessLogMessage(tryb, "[log]", caller, message, messageExt, file);
    }

    public static void Log(Tryb tryb, int level, string caller, string message, string messageExt, string file)
    {
        ProcessLogMessage(tryb, "[log]", caller, message, messageExt, file);
    }

    public static void Error(Tryb tryb, string caller, string message, string messageExt)
    {
        String strAppDir = Path.GetDirectoryName(Assembly.GetExecutingAssembly().GetName().CodeBase);
        strAppDir = strAppDir.Replace("file:\\", ""); // dla windowsa
        strAppDir = strAppDir.Replace("file:", "");  // dla linuksa
        string file = Path.Combine(strAppDir, "error.txt");
        ProcessLogMessage(tryb, "[error]", caller, message, messageExt, file);
    }

    public static void Error(Tryb tryb, string caller, string message, string messageExt, string file)
    {
        ProcessLogMessage(tryb, "[error]", caller, message, messageExt, file);
    }    

    static void ProcessLogMessage(Tryb tryb, string rodzaj, string caller, string message, string messageExt, string file)
    {
        string dane = String.Format("{0}, {1}, {2}, {3}, {4}", DateTime.Now, rodzaj, caller, message, messageExt);
        switch (tryb)
        {
            case Tryb.FILE:
                lock (_diskDriveLocker)
                {
                    WriteLogToFile(dane, file);
                }
                break;
            case Tryb.CONSOLE:
                lock (_consoleLocker)
                {
                    Console.WriteLine(dane);
                }
                break;
            case Tryb.FILEANDCONSOLE:
                lock (_consoleLocker)
                {
                    Console.WriteLine(dane);
                }
                lock (_diskDriveLocker)
                {
                    WriteLogToFile(dane, file);
                }
                break;
        }
    }

    static void WriteLogToFile(string strLogMessage, string strLogFile)
    {
        StreamWriter swLog = null;

        try
        {
            if (!File.Exists(strLogFile))
                swLog = new StreamWriter(strLogFile);
            else
                swLog = File.AppendText(strLogFile);

            swLog.WriteLine(strLogMessage);
        }
        finally
        {
            if (swLog != null)
            {
                swLog.Close();
                swLog.Dispose();
            }
        }
    }

}

public enum Tryb
{
    FILE = 0,
    CONSOLE = 1,
    FILEANDCONSOLE = 2
}

Answer 1

First of all, your locking seems ok, I don't expect any drawbacks by using multiple threads.

That being said, let us review this from bottom to top.

public enum Tryb
{
    FILE = 0,
    CONSOLE = 1,
    FILEANDCONSOLE = 2
}  

• The name of that enum is at the nicest say unclear. A name like LogTarget would be more clear and its meaning would be obvious to any reader/user of this code.

• Because it only has 3 members you should consider to add the [Flags] atribute to that enum which would result in a much shorter code, because you could omit the switch statement in ProcessLogMessage method.

This would look like so

[Flags]
public enum LogTarget
{
    FILE = 1,
    CONSOLE = 2,
    FILEANDCONSOLE = FILE | CONSOLE
}

static void WriteLogToFile(string strLogMessage, string strLogFile)
{
    StreamWriter swLog = null;

    try
    {
        if (!File.Exists(strLogFile))
            swLog = new StreamWriter(strLogFile);
        else
            swLog = File.AppendText(strLogFile);

        swLog.WriteLine(strLogMessage);
    }
    finally
    {
        if (swLog != null)
        {
            swLog.Close();
            swLog.Dispose();
        }
    }
}

• instead of having a Try..Finally and 2 different ways of creating the StreamWriter you should use the overloaded constructor StreamWriter(string, boolean) and enclose its usage in a using block which takes care of disposing and closing automatically.

  Based on the valid comment of @Heinzi

  You don't need bool append = File.Exists(strLogFile);, just use new StreamWriter(strLogFile, true). According to the documentation‌​, the Boolean parameter has no effect if the file does not exist

  you can just use true for append so no need to check if the file exists.

• although braces {} are optional for single line if..else statements you really should use them because they will make your code less error prone.

Applying this will lead to

static void WriteLogToFile(string strLogMessage, string strLogFile)
{
    using (StreamWriter swLog = new StreamWriter(strLogFile, true))
    {
        swLog.WriteLine(strLogMessage);
    }
}

static void ProcessLogMessage(Tryb tryb, string rodzaj, string caller, string message, string messageExt, string file)
{
    string dane = String.Format("{0}, {1}, {2}, {3}, {4}", DateTime.Now, rodzaj, caller, message, messageExt);
    switch (tryb)
    {
        case Tryb.FILE:
            lock (_diskDriveLocker)
            {
                WriteLogToFile(dane, file);
            }
            break;
        case Tryb.CONSOLE:
            lock (_consoleLocker)
            {
                Console.WriteLine(dane);
            }
            break;
        case Tryb.FILEANDCONSOLE:
            lock (_consoleLocker)
            {
                Console.WriteLine(dane);
            }
            lock (_diskDriveLocker)
            {
                WriteLogToFile(dane, file);
            }
            break;
    }
}

Now we can use the enum by calling the HasFlags() method which results in only 2 if statements.

static void ProcessLogMessage(LogTarget target, string rodzaj, string caller, string message, string messageExt, string file)
{
    string logMessage = String.Format("{0}, {1}, {2}, {3}, {4}", DateTime.Now, rodzaj, caller, message, messageExt);

    if (target.HasFlag(LogTarget.FILE))
    {
        lock (_diskDriveLocker)
        {
            WriteLogToFile(logMessage, file);
        }
    }

    if (target.HasFlag(LogTarget.CONSOLE))
    {
        lock (_consoleLocker)
        {
            Console.WriteLine(logMessage);
        }
    }   
}

---

• The composition of the error.txt and log.txt filenames should be done in separate methods and be assigned to class level variables so they could be reused.

Answer 2

1. This code block

   String strAppDir = Path.GetDirectoryName(Assembly.GetExecutingAssembly().GetName().CodeBase);
   strAppDir = strAppDir.Replace("file:\\", ""); // dla windowsa
   strAppDir = strAppDir.Replace("file:", "");  // dla linuksa
   string file = Path.Combine(strAppDir, "error.txt");
   

   is clearly copy-pasted. :) You should extract this logic to separate method.

2. Console class is aready thread-safe. You gain nothing by locking it.

3. Performance-wise it makes more sense to open log files once and keep them open for the life-time of your Debug class. Re-opening file stream every time you need to write a new line is a huge waste of resources, if you write to log often.

4. You should probably use separate locks for different files. There is no point in locking "error.txt" while you are writing to "log.txt".