Is the #region directive a Code Smell or an Anti-Pattern?

I recently did a twtPoll to get a feel for what other developers think about the use of the #region directive. The results may surprise you.

First, some definitions:

Code Smell: In computer programming, code smell is any symptom in the source code of a program that possibly indicates a deeper problem. Often the deeper problem hinted by a code smell can be uncovered when the code is subjected to refactoring in small, controlled steps, and the resulting design is examined to see if there are any further code smells that indicate the need of more refactoring. From the point of view of a programmer charged with performing refactoring, code smells are heuristics to indicate when to refactor, and what specific refactoring techniques to use. Thus, a code smell is a driver for refactoring.

Anti-Pattern: In software engineering, an anti-pattern (or antipattern) is a pattern that may be commonly used but is ineffective and/or counterproductive in practice.

In the .NET world, many developers use the region directive to hide complexity. It is used as illustrated below:

#region "Import Logic"
/*
// Many lines of code here....
*/
#endregion

Let's have a look at the poll results:

You can see that the choices have been worded to make it easier to choose the first one. Yet, developers who participated in this poll are a lot smarter than I thought they might be. Only 22% chose "Regions help organize code and make it more readable (the "Fanboi" choice). Fully 31% took the hard line and agree with me that "The #Region directive is bad and should be abolished."

If you are using an IDE like Visual Studio, you can collapse all the code in between a region directive with a single click. There is a certain code smell associated with regions when they are used blindly. If a class' logic needs to be hidden for any reason, chances are it has too many responsibilities.

In the OOP world, according to the SOLID approach,  each class should only have a single responsibility. That is not to say that regions do not have their place, but they are often overused because it is easier than trying to refactor the code. One thing I  have noticed is that many regions' names often describe a responsibility that "should be" another class (Import Logic as used in the example). That is definitely a sign of code smell because the class could possibly delegate the responsibility to another class.

Back in the early days of .Net (ie 2000-2003), I started using  #region moderately in my code. One thing I found out right away was that if a #region is collapsed and you are searching in the Code Window for something that is contained within that #region, it won't be found.

But since reading Martin Fowler's Refactoring book where the concept of Code Smells was introduced, I found that I really don't need C# #regions.

Why are regions a Code Smell?

Using Regions to group members.

In many companies, it is commonplace (and even required practice) to group similar class members together and place a region block around them. The problem is that Visual Studio already has a facility to do this automatically: it's been around in VS for well over a decade. It's called the Class View window, accessed with the shift-ctrl-C shortcut. So I don't know why anyone would care about building a class "DOM" into their code using Regions, rather than rely on the Class View window.  You can search for anything from the Class View Window, and it works just fine.

Using Regions to hide blocks within a method.


This is where it gets ugly. I've seen multi-hundred line long methods containing regions, and subregions and sub-subregions. I've seen switch() statements wrapped in a region, then each individual case have a region, then subregions inside of the region inside of the case. Switch statements are already a potential Code Smell, but having region after region after region is a sure-fire way of telling you the method is too long.
    
So:  If #regions are good at categorizing many lines of code, why is it a code smell to use them?

This leads to another question: Why do we need to categorize?

The answer is easy. Because of the many lines of code we've created!

And finally, the question we should be asking: Why do we allow many lines of code in a single file?

A single class with many lines of code is called a God object. This is a well known anti-pattern, and will kill maintenance.

In C# code the #region/#endregion keywords allows making 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.

Regions are a bad practice inside methods

Methods should be short. The shorter, the better. If methods are too long, there is a problem. If there are only ten lines in a method, you probably wouldn't use regions to hide five of them when working on the other five.

Methods should do one and one only thing. Regions, on the other hand, are intended to separate different things. If your method does A, then does B, it's logical to collapse A into a region and B into a second region, but this is a wrong approach: you should instead refactor the method into two separate methods.

Use
more, smaller methods:

Another important reason why methods should be small: performance. The .NET Runtime invokes the JIT compiler to translate the IL code in your assemblies. This task is amortized across the lifetime of your AppDomain: Instead of JITing your entire application when it starts, the CLR invokes the JITer on a function-by-function basis. This minimizes startup cost. Methods that do not ever get called do not get JITed. You can minimize the amount of extraneous code that gets JITed by factoring your code into more, smaller methods. If you have an if - else block where only one block of code will end up being the path, consider refactoring both code blocks in the if - else into separate methods. When you do this, only the one that is used gets JITed.

Regions are bad practice outside methods

Some people use them to group together fields, properties, etc. This approach is wrong: if your code does not have any StyleCop warnings, then fields, properties, private methods, constructors, etc. are already in order and easy to find without the need for a region.

Others use regions to hide lots of code. For example, when you have in a class a hundred fields (which can make at least 500 lines of code), 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 be thinking about using inheritance or slice the object into several objects. We have partial classes for this purpose, too. If you look at the codebehind for an ASP.NET page, it has a Designer partial class where all the fields are defined. This keeps the main class cleaner and uncluttered.

Finally, some people are tempted to use regions to group together related things: an event with its delegate for example, or group file- related methods together and calculation methods together. In the first case, it becomes a mess which is difficult to maintain, read and understand. In the second case, there is certainly a flow: a class should again be sliced into different classes or inheritance should be implemented.

I'd be happy to see regions removed from the compiler altogether. Every developer comes up with his or her own pointless grooming scheme that will rarely be of value to another programmer. It has everything to do with programmers wanting to decorate and beautify their baby, but rarely anything to do with any real value.

Can you think of one example where you thought, "Geez, I wish my colleague has used some regions here!"?

The //comment directive can also be a Code smell:

for ( int i=0;i<Rows.Count;i++) // iterate "i"

Yes, people actually do stuff like the above silly example. Method names and code blocks that are descriptive usually eliminate the need for such comments.

Even though I can configure my IDE to automatically expand all regions they are still an eyesore and detract from reading the real code.

Poorly factored code: Much of the code that I've seen uses regions as a poor-man's factoring tool. For example, a class which has grown to the point where it makes sense to specialize it for different purposes might be instead split into separate regions, one for each purpose, instead of refactoring to separate classes.

Code written using the wrong libraries, and sometimes the wrong language, for the problem domain Often, when a programmer is not using the right set of libraries for the problem domain, you'll see code becoming incredibly verbose - with lots of little helper functions which really don't belong (they probably belong in their own library).

Code written by students or recent graduates. Some programs and courses seem to try and inculcate students with the use of regions for all sorts of strange purposes. You'll see regions littering the source code to the point where the ratio of region tags to lines of code is in the range of 1:5 or worse.

When you see a multi-hundred line class where the regions demarcate distinct actions that are being done within the method, it is often a candidate to do a refactor -> extract method on each of the regions without impacting the functionality of the code.

In general, regions used to hide boiler plate code are useful. I would advise against using regions to hide properties, fields, and the like because if they are too unwieldy to look at when working within the class, it's probably a sign that the class should be further broken down. But as a hard rule, if you're putting a region within a method, you'd probably be better off extracting another method that explains what is happening than wrapping that block in a region.

The best thing a developer can do - both for themselves and for the other developers who must read their code - is to abolish the #region directive and not use it at all.

By Peter Bromberg   Popularity  (3298 Views)