Are #regions an antipattern or code smell?

In C# code it allows the #region/#endregion keywords to made areas of code collapsible in the editor. Whenever I am doing this though I find it is to hide large chunks of code that could probably be refactored into other classes or methods. For example I have seen methods that contain 500 lines of code with 3 or 4 regions just to make it manageable.

So is judicious use of regions a sign of trouble? It seems to be to me.

A code smell is a symptom which indicates that there is a problem in the design which will potentially increase the number of bugs: this is not the case for regions, but regions can contribute creating code smells, like long methods.

Since:

An anti-pattern (or antipattern) is a pattern used in social or business operations or software engineering that may be commonly used but is ineffective and/or counterproductive in practice

regions are anti-patterns. They require more work which doesn’t increase the quality or the readability of the code, which doesn’t reduce the number of bugs, and which may only make the code more complicate to refactor.

Don’t use regions inside methods; refactor instead

Methods must be short. If there are only ten lines in a method, you probably wouldn’t use regions to hide five of them when working on other five.

Also, each method must do one and one only thing. Regions, on the other hand, are intended toseparate different things. If your method does A, then B, it’s logical to create two regions, but this is a wrong approach; instead, you should refactor the method into two separate methods.

Using regions in this case can also make the refactoring more difficult. Imagine you have:

private void DoSomething()
{
    var data = LoadData();
    #region Work with database
    var verification = VerifySomething();
    if (!verification)
    {
        throw new DataCorruptedException();
    }

    Do(data);
    DoSomethingElse(data);
    #endregion

    #region Audit
    var auditEngine = InitializeAuditEngine();
    auditEngine.Submit(data);
    #endregion
}

Collapsing the first region to concentrate on the second is not only risky: we can easily forget about the exception stopping the flow (there could be a guard clause with a return instead, which is even more difficult to spot), but also would have a problem if the code should be refactored this way:

private void DoSomething()
{
    var data = LoadData();
    #region Work with database
    var verification = VerifySomething();
    var info = DoSomethingElse(data);

    if (verification)
    {
        Do(data);
    }

    #endregion

    #region Audit
    var auditEngine = InitializeAuditEngine(info);
    auditEngine.Submit(
        verification ? new AcceptedDataAudit(data) : new CorruptedDataAudit(data));
    #endregion
}

Now, regions make no sense, and you can’t possibly read and understand the code in the second region without looking at the code in the first one.

Another case I sometimes see is this one:

public void DoSomething(string a, int b)
{
    #region Validation of arguments
    if (a == null)
    {
        throw new ArgumentNullException("a");
    }

    if (b <= 0)
    {
        throw new ArgumentOutOfScopeException("b", ...);
    }
    #endregion

    #region Do real work
    ...
    #endregion
}

It’s tempting to use regions when arguments validation starts to span tens of LOC, but there is a better way to solve this problem: the one used by .NET Framework source code:

public void DoSomething(string a, int b)
{
    if (a == null)
    {
        throw new ArgumentNullException("a");
    }

    if (b <= 0)
    {
        throw new ArgumentOutOfScopeException("b", ...);
    }

    InternalDoSomething(a, b);
}

private void InternalDoSomething(string a, int b)
{
    ...
}

Don’t use regions outside methods to group

  • Some people use them to group together fields, properties, etc. This approach is wrong: if your code is StyleCop-compliant, then fields, properties, private methods, constructors, etc. are already grouped together and easy to find. If it’s not, than it’s time to start thinking about applying rules which ensure uniformity across your codebase.
  • Other people use regions to hide lots of similar entities. For example, when you have a class with hundred fields (which makes at least 500 lines of code if you count the comments and the whitespace), you may be tempted to put those fields inside a region, collapse it, and forget about them. Again, you are doing it wrong: with so many fields in a class, you should think better about using inheritance or slice the object into several objects.
  • Finally, some people are tempted to use regions to group together related things: an event with its delegate, or a method related to IO with other methods related to IO, etc. In the first case, it becomes a mess which is difficult to maintain, read and understand. In the second case, the better design would probably be to create several classes.

Is there a good use for regions?

No. There was a legacy use: generated code. Still, code generation tools just have to use partial classes instead. If C# has regions support, it’s mostly because this legacy use, and because now that too many people used regions in their code, it would be impossible to remove them without breaking existent codebases.

Think about it as about goto. The fact that the language or the IDE supports a feature doesn’t mean that it should be used daily. StyleCop SA1124 rule is clear: you should not use regions. Never.

Examples

I’m currently doing a code review of my coworker’s code. The codebase contains a lot of regions, and is actually a perfect example of both how to not use regions and why regions lead to bad code. Here are some examples:

4 000 LOC monster:

I’ve recently read somewhere on Programmers.SE that when a file contains too many usings (after executing “Remove Unused Usings” command), it’s a good sign that the class inside this file is doing too much. The same applies to the size of the file itself.

While reviewing the code, I came across a 4 000 LOC file. It appeared that the author of this code simply copy-pasted the same 15-lines method hundreds of times, slightly changing the names of the variables and the called method. A simple regex allowed to trim the file from 4 000 LOC to 500 LOC, just by adding a few generics; I’m pretty sure that with a more clever refactoring, this class may be reduced to a few dozens of lines.

By using regions, the author encouraged himself to ignore the fact that the code is impossible to maintain and poorly written, and to heavily duplicate the code instead of refactor it.

Region “Do A”, Region “Do B”:

Another excellent example was a monster initialization method which simply did task 1, then task 2, then task 3, etc. There were five or six tasks which were totally independent, each one initializing something in a container class. All those tasks were grouped into one method, and grouped into regions.

This had one advantage:

  • The method was pretty clear to understand by looking at the region names. This being said, the same method once refactored would be as clear as the original.

The issues, on the other hand, were multiple:

  • It wasn’t obvious if there were dependencies between the regions. Hopefully, there were no reuse of variables; otherwise, the maintenance could be a nightmare even more.
  • The method was nearly impossible to test. How would you easily know if the method which does twenty things at a time does them correctly?

Fields region, properties region, constructor region:

The reviewed code also contained a lot of regions grouping all the fields together, all the properties together, etc. This had an obvious problem: source code growth.

When you open a file and see a huge list of fields, you are more inclined to refactor the class first, then work with code. With regions, you take an habit of collapsing stuff and forgetting about it.

Another problem is that if you do it everywhere, you’ll find yourself creating one-block regions, which doesn’t make any sense. This was actually the case in the code I reviewed, where there were lots of#region Constructor containing one constructor.

Finally, fields, properties, constructors etc. should already be in order. If they are and they match the conventions (constants starting with a capital letter, etc.), it’s already clear where on type of elements stops and other begins, so you don’t need to explicitly create regions for that.

http://programmers.stackexchange.com/questions/53086/are-regions-an-antipattern-or-code-smell

WebConfigurationManager tipado e com valor padrão

int operatorId = WebConfigurationManager.AppSettings.Get<int>("OperatorId");

int operatorIdComDefault = WebConfigurationManager.AppSettings.Get<int>("OperatorId", 1);

public static class NameValueCollectionExtension
{
    public static T Get<T>(this NameValueCollection collection, string key, T defaultValue = default(T)) where T : IConvertible
    {
        var value = collection.Get(key);
        if (value != null)
        {
            return (T)Convert.ChangeType(value, typeof(T));
        }
        return defaultValue;
    }
}