Thursday, November 10, 2011

#region is evil

Today at work I came across this piece of code. Let's focus on the #region part.

public void Validate(Guid instanceId, Guid ruleSetId)
    [code here]
#region logging
        if (logger.IsInfoEnabled)
            logger.InfoFormat("Excecuting validation for instance '{0}' with ruleset '{1}'", instanceId, ruleSetId);

        if (logger.IsDebugEnabled)
            logger.DebugFormat("Validation script for instance '{0}'':\r\n{1}", instanceId, script);
    [more code here]

When collapsed it looks like this:
    [code here]
+ "logging"
    [more code here]

We could improve it a bit by using a better region name:
    [code here]
+ "Log Validation Execution"
    [more code here]

But still: When the region is collapsed we don't see what's happening. It might be anything, because the code has access to all method and instance scoped variables. If the region is expanded we get lost in details.

What I would do:
  1. Move all the logging code to a seperate class (say ValidationLogger)
  2. Replace the #region with a single method call:
    [code here]
    log.LogValidationExecution(instanceId, ruleSetId);
    [more code here]

The advantage is that you see what happens (a method call) and what information will be used to log (the passed parameters) and it's also just one line.

Please, think twice before using the #region tag. In many cases it's a strong indicator that you need to refactor your code!

1 comment:

  1. I think this is a german implementation pattern :-)