4

Ok so after reading danben's answer on this post, I guess I'm convinced of the need for writting this kind of code, atleast in a lot of cases. My managers seem to be agreeing too.

if (log.IsDebugEnabled)
     log.Debug("ZDRCreatorConfig("+rootelem.ToString()+")");
if (log.IsInfoEnabled)
     log.Info("Reading Configuration . . .");

The problem with it is it bugs the heck out of me seeing all these if statements placed everywhere just to do a simple log statement.

My question is, how might we refactor this into a class without reproduceing the performance problem of having to evaluate the arguments to the log method?

Simply putting it in a class as a static method doesn't help, because when you pass the Object message it still has to evaluate the argument:

public class LogHelper {
     public static Info(ILog log, Object message) {
          if(log.IsInfoEnabled) { log.Info(message); }
     }
}

C# apparently doesn't support forcing a method to be inline, so that solution isn't available. MACROs are not supported in C#. What can we do?!?!

UPDATE: Thanks for the replies, I have not forgot about this one; it's just low priorty on my list right now. I will get to it and award the answer once I get caught up a bit. Thanks.

Another UPDATE:
well ... I still haven't look at this closly yet, and both of you deserve the correct answer; but I awarded Tanzelax the answer because I agree, I think they will be automatically inlined. The link he posted does good job of convincing me I shouldn't worry too much about this right now, which is also good lol. I'll still look at those lambda things later. Thanks for the help!

Community
  • 1
  • 1
cchampion
  • 7,607
  • 11
  • 41
  • 51

3 Answers3

15

One simple solution is to use lambda expressions to effectively defer the message generation until it's needed, if it's needed:

public static class LogHelper {
    public static void Info(this ILog log, Func<Object> messageProvider) {
        if(log.IsInfoEnabled) { log.Info(messageProvider()); }
    }
}

Call it with:

log.Info(() => "This is expensive: " + CalculateExpensiveValue());
Phil Ross
  • 25,590
  • 9
  • 67
  • 77
Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • ok, i'll give that a try when i get a chance and let you know how I like it. You answer nearly all my questions .... and I appreciate it!! lol. – cchampion Feb 11 '10 at 22:05
  • This is off the subject of this question, but what unit testing framework do you use for dot net? Unfortunately my company doesn't have unit testing in place for their dot net projects, and I'm gonna to start putting them in there. Thanks. – cchampion Feb 12 '10 at 17:23
  • @cchampion: Personally I use NUnit, but there are plenty of alternatives. – Jon Skeet Feb 12 '10 at 18:10
3

If the static helper method is that simple, it should be inlined automatically, and will have the performance to match.

At what level C# compiler or JIT optimize the application code?

Community
  • 1
  • 1
Tanzelax
  • 5,406
  • 2
  • 25
  • 28
  • thanks, and I was actually thinking that might be the case. I'll read that when I get a chance and update here later. – cchampion Feb 11 '10 at 22:06
1

Just a comment about the static log helper function...

If you use a LogHelper function like you are proposing, you will lose the ability to log the call site info.

So, if you have the static helper class like this (leaving aside deferring the evaluation of the message parameters):

public class LogHelper 
{ 
     public static Info(ILog log, Object message) 
     { 
          if(log.IsInfoEnabled) 
          { 
            log.Info(message); 
          } 
     } 
} 

And you use it like this:

public class MyClass
{
  ILog logger = LogManager.GetLogger(<blah blah>);
  public void MyFunc()
  {
    logger.Info("Hello!");
  }
}

If you have "call site" logging turned on, the call site information will be from your helper class: LogHelper.Info rather than your real class MyClass.MyFunc.

That might not matter much if you don't rely on logging the call site information.

wageoghe
  • 27,390
  • 13
  • 88
  • 116