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.