Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
}
share|improve this question

2 Answers 2

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.
share|improve this answer
1  
little note that HasFlag is slower than a manual bitwise operation, if speed is important – Fredou yesterday
4  
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. – Heinzi yesterday
2  
Adding to what @Heinzi said, just in general checking File.Exists prior to taking action based on the result is a poor idea - what happens if some other program creates a file in between the check and the action? – Bob yesterday
1  
@Heinzi I edited my answer to reflect your comment. Thanks – Heslacher 16 hours ago
1  
@Bob valid point – Heslacher 16 hours ago
  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".

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.