Factory Method and Log writer class

Question

I Have this class want to practice how to write a factory method pattern

and I can change the logPlace by set the app.config,

and I try to improve the performance so I use static List<string>,I want to decrease IO loading

Here is my question

1. This class can call factory method pattern or not?
2. Is my code as clear as it could be?
3. I think the LogBoth class can be much better, but I have no idea how to improve it.

   public enum LogLevel
    {
        Error, Debug, Info, Fatal
    }
    static class Msg
    {
        public static List<string> msgList = new List<string>();
    }
    public class LogHelper
    {
        public static ILog Create()
        {
            string logPlace = System.Web.Configuration.WebConfigurationManager.AppSettings["LogPlace"];
            switch (logPlace)
            {
                case "DB":
                    return new LogDB();
                case "File":
                    return new LogFile();
                case "Both":
                    return new LogBoth();
                default:
                    return new LogFile();
            }
            throw new NullReferenceException();
        }
    }
    public interface ILog
    {
        void WriteLog(string msg);
        void ExecWrite(LogLevel logLevel);
    }
    public class LogDB : ILog
    {
        public LogDB()
        {
            Msg.msgList.Clear();
        }
        public void WriteLog(string msg)
        {
            Msg.msgList.Add(msg);
        }
        public void ExecWrite(LogLevel logLevel)
        {
            throw new NotImplementedException();
        }
    }
    public class LogFile : ILog
    {
       
        private static object _lockObject = new object();

        public LogFile()
        {
            Msg.msgList.Clear();
        }

        public void WriteLog(string msg)
        {
            Msg.msgList.Add(msg);
        }
        public void ExecWrite(LogLevel logLevel)
        {
            try
            {
                Policy.Handle<Exception>().Retry(1).Execute(()=>WriteToFile(logLevel));
            }
            catch (Exception ex)
            {
                LogDB logDB = new LogDB();
                logDB.ExecWrite(logLevel);
            }
        }

        private void WriteToFile(LogLevel logLevel)
        {
            lock (_lockObject)
            {
               
                using (FileStream fs = new FileStream(Path.Combine(path, fileName), FileMode.Append, FileAccess.Write, FileShare.ReadWrite))
                {
                    using (StreamWriter sw = new StreamWriter(fs))
                    {
                        foreach (string msg in Msg.msgList)
                        {
                            sw.WriteLine(string.Format("{0} [{1}] {2}", DateTime.UtcNow.AddHours(08).ToString("yyyy/MM/dd HH:mm:ss"), logLevel, msg));
                        }
                    }
                }
            }
        }

    }
    public class LogBoth : ILog
    {
        public LogBoth()
        {
            Msg.msgList.Clear();
        }
        public void ExecWrite(LogLevel logLevel)
        {
            LogDB logDB = new LogDB();
            LogFile logFile = new LogFile();

            logDB.ExecWrite(logLevel);
            logFile.ExecWrite(logLevel);
        }

        public void WriteLog(string msg)
        {
            Msg.msgList.Add(msg);
        }
    }

Answer 1

Here are my observations:

LogLevel

• LogLevel: Usually the industry calls this Severity Levels. I would suggest to try to follow these and do not re-invent the wheel.
  In some cases it might worth to differentiate these two concepts:
  • Log Severity: The seriousness / weight / riskiness of a given event.
  • Log Level: Usually there is a particular order between the levels. The logger should log everything which is at a given level or above
    • For example: if you have this ordering: Debug > Information > Warning and you set the level to Information then Debug logs will be omitted from persistence.

Msg

• Msg: Please try to avoid using abbreviations or short-forms in case of class names.
• msgList: Please try to avoid Hungarian notation.
• Msg.msgList: This seems quite clumsy and stuttering.
  • Please try to come up with better naming and encapsulation.

LogHelper

• LogHelper: This is a LoggerFactory. If you have only just static members then mark your class static as well. By the way this is more like an abstract factory than a factory method.
  • Please read this related discussion.
• throw new NullReferenceException: It is really rare to throw NullReferenceException manually. And fortunately this is not needed at all. You have at least two options:
• single return statement:

ILog logger;
switch (logPlace)
{
    case "DB":
        logger = new LogDB();
        break;
    case "File":
        logger = new LogFile();
        break;
    case "Both":
        logger = new LogBoth();
        break;
    default:
        logger = new LogFile();
        break;
}

return logger;

or switch expression (in case of C# 8):

return logPlace switch
{
    "DB" => new LogDB(),
    "File" => new LogFile(),
    "Both" => new LogBoth(),
    _ => new LogFile(),
};

ILog

• ILog: This should be ILogger to be precise.
• void WriteLog(string msg): Usually here should the logger receive the log level / severity. Or it has several overloads with different severity levels, like LogInformation, LogWarning, LogError, etc.
  • Usually loggers can accept any arbitrary data (in a form of object) to give the ability to include/attach any useful information to the event.
• void ExecWrite(LogLevel logLevel): Usually this is called Flush.
  • And usually there are some automation behind the scenes, which calls this periodically or when the logs count reaches a given threshold.
  • Here you could consider to make this function asynchronous to take advantage of async I/O (file and database writes are usually support async I/O)

LogDb

• LogDb: Yet again naming: DatabaseLogger would be way better in my opinion.
• Msg.msgList.Clear(): This could be quite error-prone if you call it inside the ctor. Image the following situation: You have created a LogDb and wrote some log into it then you create a LogFile. Now all your logs are now gone.
• LogDb'sExecWrite: It might make sense to use NotSupportedException if you have no willing to implement it. But in that case the whole class does not make too much sense.

LogFile

• LogFile: FileLogger as a naming alternative.
• LogFile' ExecWrite: This whole retry logic is quite naive. For example if the file path is invalid (pointing to a non-existing file or folder) then retrying the same action would not change anything.
  • With the current implementation of LogDb your catch block does not make any sense.
  • Also you are swallowing the exception without logging the fact that something went wrong.
• WriteToFile: What if the file does not exists? Why do you use FileShare.ReadWrite? Why don't use async I/O?
• DateTime.UtcNow.AddHours(08): This hard-coded time-zone handling makes your utility hard to maintain and even harder to make it reusable.
  • Also please bear in mind that all events will have the same timestamp that are written to the file at the same time. A better approach would be to add the timestamp to your log message at the time when the consumer of your API calls the WriteLog method.

LogBoth

• LogBoth: DatabaseAndFileLogger as a naming alternative.
• LogBoth's ExecuteWrite: Here you have determined the ordering. If you have to change that then you have create a new class or create an overload.
  • If you would use async I/O then you could take advantage of concurrent persistence calls.
  • In the industry standard loggers there is no such thing like this. The consumer of your API can set up multiple sinks (persistence storage) in configuration file and the logger will take care of them.
  • It is quite challenging to implement it in the right way, because what should happen if either of them fails?