2
\$\begingroup\$

Following is the code for a logger facade that I have created, which can be used to log via any of the standard and configurable loggers like log4net, BitFactory, currently I have just integrated these two. Salient features are:

  1. Logger can be changed at runtime, every iteration can use a different logger (I know that may not be required, but essence is process needn't be stopped to change a logger)

  2. Prominent log severity supported

  3. Supports Log Category for Bit Factory

  4. Single logging method Log for all loggers

  5. Thread safe

Please share your view, what can be done to improve this further

Code underneath

Constants

public static class LogSettings
{
    public const string LoggerTypeKey = "LoggerType";
    public const string LogCategoryKey = "LogCategory";
    public const string LogSeverityKey = "LogSeverity";

    public const LogLevel logLevelDefault = LogLevel.Info;

    public const LogCategory logCategoryDefault = LogCategory.All;

    public static LoggerType  loggerType;
    public static LogLevel logSeverity;
    public static LogCategory logCategory;
}

Logger Code

using BitFactory.Logging;
using log4net;
using log4net.Config;
using System.Web.Configuration;


/// <summary>
/// Singleton Custom Logger class, which can wrap multiple loggers.
/// </summary>
public class ViewRLogger
{
    /// <summary>
    /// Custom Logger class object - Singleton
    /// </summary>
    private static ViewRLogger viewrLogger;

    /// <summary>
    /// Object to ensure the logger creation and release in a thread safe manner
    /// </summary>
    private static readonly object lockViewRLogger = new object();

    /// <summary>
    /// Counter check to ensure no one is using logger before it can be released in a thread safe scenario
    /// </summary>
    private static int viewRLoggerCounter;

    /// <summary>
    /// Log4Net logger type
    /// </summary>
    private ILog log4NetLogger;

    /// <summary>
    ///BitFactory Logger Type 
    /// </summary>
    private Logger BitFactoryLogger;

    /// <summary>
    ///  Private cosntructor of the CustomLogger, initialize a particular type of logger
    /// </summary>
    /// <param name="loggerType"></param>
    private ViewRLogger(LoggerType loggerType)
    {
        switch (loggerType)
        {
            case LoggerType.Log4Net:
                if (log4NetLogger == null)
                    log4NetLogger = log4net.LogManager.GetLogger(typeof(Log4NetConfigurationSectionHandler));
                    XmlConfigurator.Configure();
                break;
            case LoggerType.BitFactory:
                if (BitFactoryLogger == null)
                    BitFactoryLogger = ConfigLogger.Instance;
                break;
            default:
                if (BitFactoryLogger == null)
                    BitFactoryLogger = ConfigLogger.Instance;
                break;
        }
    }

    /// <summary>
    /// GetLogger method to initiate the CustomLogger object
    /// </summary>
    /// <param name="loggerType"></param>
    /// <returns></returns>
    public static ViewRLogger GetLogger()
    {
        // Thread safe acquire
        lock (lockViewRLogger)
        {
            // Increment counter - suggest usage
            viewRLoggerCounter++;

            if (viewrLogger == null)
            {
                LogSetup();
                LoggerType loggerType = LogSettings.loggerType;
                viewrLogger = new ViewRLogger(loggerType);
            }

            return (viewrLogger);
        }
    }

    /// <summary>
    /// Release the Custom Logger and logger type instance
    /// </summary>
    public static void ReleaseLogger()
    {
        // Thread safe release
        lock (lockViewRLogger)
        {
            // decrement counter - suggest release
            viewRLoggerCounter--;

            if (viewrLogger != null && viewRLoggerCounter == 0)
            {
                viewrLogger.log4NetLogger = null;
                viewrLogger.BitFactoryLogger = null;
                viewrLogger = null;
            }
        }
    }

    /// <summary>

/// Log method to log a message based on logger type and logging severity
/// </summary>
/// <param name="logMessage"></param>
/// <param name="logSeverity"></param>
/// <param name="logCatgeory"></param>
public void log(string logMessage, // message for logging
                LogLevel logSeverity = LogSettings.logLevelDefault, // logSeverity refers to level of logging, which is compared to application logging setting
                LogCategory logCatgeory = LogSettings.logCategoryDefault)
{
    LoggerType loggerType = LogSettings.loggerType; // loggerType refers to the logger type like log4net, objectguy logger

    // Compares the current message log severity with the application setting for log severity
    // Logs the message only if it is less than oir equal to the application logging or else do nothing
    if (logSeverity <= LogSettings.logSeverity)
    { 
        switch (loggerType)
        {
            case LoggerType.Log4Net:
                Log4NetLogging(logMessage,logSeverity);
            break;
            case LoggerType.BitFactory:
                BitFactoryLogging(logCatgeory.ToString(), logMessage,logSeverity);
            break;
        }
    }
}

/// <summary>
/// Logging various severity levels for Log4Net logger
/// </summary>
/// <param name="logMessage"></param>
/// <param name="logSeverity"></param>
private void Log4NetLogging(string logMessage, LogLevel logSeverity)
{
    switch (logSeverity)
    {
        case LogLevel.Fatal:
            log4NetLogger.Fatal(logMessage);
            break;
        case LogLevel.Error:
            log4NetLogger.Error(logMessage);
            break;
        case LogLevel.Warn:
            log4NetLogger.Warn(logMessage);
            break;
        case LogLevel.Info:
            log4NetLogger.Info(logMessage);
            break;
        case LogLevel.Debug:
            log4NetLogger.Debug(logMessage);
            break;
    }     
}

/// <summary>
/// Logging various severity levels for BitFactory logger
/// </summary>
/// <param name="logCategory"></param>
/// <param name="logMessage"></param>
/// <param name="logSeverity"></param>
private void BitFactoryLogging(string logCategory,string logMessage, LogLevel logSeverity)
{
    switch (logSeverity)
    {
        case LogLevel.Fatal:
            BitFactoryLogger.LogFatal(logCategory, logMessage);
            break;
        case LogLevel.Error:
            BitFactoryLogger.LogError(logCategory, logMessage);
            break;
        case LogLevel.Warn:
            BitFactoryLogger.LogWarning(logCategory, logMessage);
            break;
        case LogLevel.Info:
            BitFactoryLogger.LogInfo(logCategory, logMessage);
            break;
        case LogLevel.Debug:
            BitFactoryLogger.LogDebug(logCategory, logMessage);
            break;
    }
}

/// <summary>
/// Initial Log setup, which defines the logger type setting, Log severity anf Log category for the Bit Factory logger
/// </summary>
private static void LogSetup()
{
    // Set the Logger type
    switch (WebConfigurationManager.AppSettings[LogSettings.LoggerTypeKey])
    {
        case "Log4Net":
            LogSettings.loggerType = LoggerType.Log4Net;
            break;
        case "BitFactory":
            LogSettings.loggerType = LoggerType.BitFactory;
            break;
        default:
            LogSettings.loggerType = LoggerType.BitFactory;
            break;
    }

    // Set the logging severity
    switch (WebConfigurationManager.AppSettings[LogSettings.LogSeverityKey])
    {
        case "None":
            LogSettings.logSeverity = LogLevel.None;
            break;
        case "Fatal":
            LogSettings.logSeverity = LogLevel.Fatal;
            break;
        case "Error":
            LogSettings.logSeverity = LogLevel.Error;
            break;
        case "Warning":
            LogSettings.logSeverity = LogLevel.Warn;
            break;
        case "Info":
            LogSettings.logSeverity = LogLevel.Info;
            break;
        case "Debug":
            LogSettings.logSeverity = LogLevel.Debug;
            break;
        default:
            LogSettings.logSeverity = LogLevel.Info;
            break;
        /*
         *  None = 1,
         *  Fatal = 2,
         *  Error = 3,
         *  Warning = 4,
         *  Info = 5,
         *  Debug = 6
         * */
    }

    // Set the Log Category for the Bit Factory logger
    switch (WebConfigurationManager.AppSettings[LogSettings.LogCategoryKey])
    {
        case "Error":
            LogSettings.logCategory = LogCategory.Error;
            break;
        case "DB":
            LogSettings.logCategory = LogCategory.DB;
            break;
        case "BL":
            LogSettings.logCategory = LogCategory.BL;
            break;
        case "All":
            LogSettings.logCategory = LogCategory.All;
            break;
        default:
            LogSettings.logCategory = LogCategory.All;
            break;
        /**
         *  Error,
            DB,
            BL,
            All
         * */
    }
}
}

Config settings

<log4net>
  <appender name="FileAppender" type="log4net.Appender.FileAppender">
      <param name="File" value="C:\\Users\\Administrator\\ViewrLog4net.log" />
       <!-- <param name="File" value="C:\\Users\\Administrator\\Desktop\\Work\\Project\\Pynpoint_VIEWr\\VIEWR_Host_YM\\host\\VIEWR\\Viewr.log"/>
     Example using environment variables in params -->
      <!-- <param name="File" value="${TMP}\\ApplicationKit.log" /> -->
      <param name="AppendToFile" value="true" />
      <layout type="log4net.Layout.PatternLayout">
     <param name="ConversionPattern" value="%date - %-5level - %message%newline %exception" />
     <!-- <param name="ConversionPattern" value="%date [%thread] %-5level %logger [%ndc] - %message%newline"/> -->
     <!-- <param name="ConversionPattern" value="%d [%t] %-2p %c [%x] - %m%n"/> -->
      </layout>
</appender>

<root>
<level value="ALL" />
<appender-ref ref="FileAppender" />
</root>
</log4net>
  <BitFactory.Logging name="global" xmlns="http://BitFactory.Logging">
    <rollingSizeFileLoggers>
      <rollingSizeFileLogger name ="AllFile" fileName="C:\\Users\\Administrator\\ViewrBitFactoryALL_{0}.log" maxSize="20000" includeCategories="All" formatString="[{timestamp:G}]-[{severity}]-[{category}]- {message}"/>
      <rollingSizeFileLogger name ="DBFile" fileName="C:\\Users\\Administrator\\ViewrBitFactoryDB_{0}.log" maxSize="20000"  includeCategories="DB" formatString="[{timestamp:G}]-[{severity}]-[{category}]- {message}"/>
      <rollingSizeFileLogger name ="BLFile" fileName="C:\\Users\\Administrator\\ViewrBitFactoryBL_{0}.log" maxSize="20000"  includeCategories="BL" formatString="[{timestamp:G}]-[{severity}]-[{category}]- {message}"/>
      <rollingSizeFileLogger name ="ErrorFile" fileName="C:\\Users\\Administrator\\ViewrBitFactoryError_{0}.log" maxSize="20000"  includeCategories="Error" formatString="[{timestamp:G}]-[{severity}]-[{category}] - {message}"/>
    </rollingSizeFileLoggers>
  </BitFactory.Logging>
\$\endgroup\$
  • \$\begingroup\$ Is your goal to be able to log to both log4net and BitFactory at the same time, or to allow a program to switch from one to the other? \$\endgroup\$ – David Nelson Oct 17 '14 at 5:15
  • \$\begingroup\$ @David Nelson To allow the program to switch from one to another at runtime. At a given time it can only use only one logger \$\endgroup\$ – Mrinal Kamboj Oct 17 '14 at 5:20
5
\$\begingroup\$

I have to agree with @David's answer, IMO this smells:

using BitFactory.Logging;
using log4net;

Say you had to also support NLog, you'd have quite a lot of code to change, not to mention:

using BitFactory.Logging;
using log4net;
using NLog;

I think the singleton needs a higher level of abstraction, so that these 3 using statements can disappear.

I too would create an interface, to...

WRAP ALL THE LOGGERS!

public interface IWrappedLogger
{
    void Log(string logMessage, 
             LogLevel logSeverity = LogSettings.logLevelDefault, 
             LogCategory logCatgeory = LogSettings.logCategoryDefault);
}

Then you can implement the BitFactory and Log4Net -specifics in their own class:

public class BitFactoryLogger : IWrappedLogger
{
    public void Log(...)
    {
    }
}

public class Log4NetLogger : IWrappedLogger
{
    public void Log(...)
    {
    }
}

And then ViewRLogger.GetLogger can use an abstract factory to get an IWrappedLogger implementation, without caring whether it's Log4Net, BitFactory, NLog, or even a custom-made logger.

You'd only need an IWrappedLogger private field, and then you could support 20 logging frameworks without even modifying the ViewRLogger class.


I have to mention something about the comments.

    // Thread safe acquire
    lock (lockViewRLogger)

don't need.

    // Increment counter - suggest usage
    viewRLoggerCounter++;

don't need either. Don't make comments say what the code already does!

This is more worrying though:

/// <param name="logMessage"></param>
/// <param name="logSeverity"></param>
/// <param name="logCatgeory"></param>
public void log(string logMessage, // message for logging
                LogLevel logSeverity = LogSettings.logLevelDefault, // logSeverity refers to level of logging, which is compared to application logging setting
                LogCategory logCatgeory = LogSettings.logCategoryDefault)

You use XML comments, ...without using them!

Should look like this:

/// <param name="logMessage">message for logging</param>
/// <param name="logSeverity">logSeverity refers to level of logging, which is compared to application logging setting</param>
/// <param name="logCategory"></param>
public void Log(string logMessage,
                LogLevel logSeverity = LogSettings.logLevelDefault,
                LogCategory logCatgeory = LogSettings.logCategoryDefault)

This will make your comment appear in the client code's IntelliSense when you (or someone else) writes a call to this log method.. which should be called Log - because method names in C# should be PascalCase.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the detailed explanation. I will modify the design using an interface, to make it more flexible. Regarding XML comments ideally, I would have entered the details as suggested, currently I just pasted the code with generated XML comments, which lead to a different understanding. I will take care of removing the comments, where the working is clear from the code itself \$\endgroup\$ – Mrinal Kamboj Oct 17 '14 at 19:21
3
\$\begingroup\$

Your class is trying to do too much: all of the log4net and BitFactory implementation is mixed together throughout the class, and you have to include an if statement in every public method to figure out which implementation to call. As a result the implementation is much more complex than it needs to be.

The better way to go about this would be to create an interface with the public methods you want consumers to be able to call, and then have a separate class for each implementation, along with a factory method that can create an instance of the appropriate class. That way the implementation-specific logic for each library is contained in a single class, and you don't need to constantly re-check which one to use. It will also make integrating other libraries much simpler: all you need to do is add a new class and modify the factory method, rather than modifying all of the members of your single class.

\$\endgroup\$
  • \$\begingroup\$ using interface is a good idea from design perspective, but if you take a closer look none of the public method use an if loop to decide the logger, it certainly use the Switch Case, but that would be used even for the interface based implementation, when it has to decide the correct logger class to call at runtime \$\endgroup\$ – Mrinal Kamboj Oct 17 '14 at 6:14
  • \$\begingroup\$ You currently have three different switch statements that test which implementation is being used; if you add any new methods to the class, you will have to add another switch, and if you add a new implementation, you will have to modify all of those switch statements. If you separate the implementations behind an interface and use a factory method to return the correct implementation, you only need one switch statement. \$\endgroup\$ – David Nelson Oct 17 '14 at 14:40

Your Answer

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

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