Are #regions an antipattern or code smell?

  • C# allows the use of #region/#endregion keywords to make areas of code collapsible in the editor. Whenever I do this though I do it 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 so to me.

    FYI: CodeMap pretty much eliminates the needs for regions. http://visualstudiogallery.msdn.microsoft.com/1c54d1bd-d898-4705-903f-fa4a319b50f2 - Click and it takes you to the method/attribute/whatever. I use it every day and it's amazing how much you gain in productivity and in cognitive terms as well. You get a much clearer 'birds eye view' of the class.

    Can anyone make this a wiki? There is no right or wrong answer to this question (well, within reason), it's almost completely subjective.

    For what it's worth, Jeff Atwood hates them. He argues that they hide bad code.

    Code smell is a developer who don't shower and don't use a deodorant!

    Carefully crafted Regions in stinky code is what a few squeeze of Febreeze does on a crusty couch. It makes it bearable until you find the (time) money to replace it.

    What about hidding helper methods? Mike uses regions in his wonderful Export to Excel 2007 code http://mikesknowledgebase.azurewebsites.net/pages/CSharp/ExportToExcel.htm

    bit of a loaded question, leaves no room for being in defence of regions.

    VS2017 addon supercharger, works quite nice with regions. In coding #region makes no difference, but in CodeMap navigation quite usefull.

    There is a religious quality to the region hating answers. "You should use partial classes!" Note there is no fundamental difference between regions used to separate class members and partial classes. They serve the exact same purpose. The only difference is regions are more powerful in that purpose: you can apply them on a finer grained level than partial classes if you wanted to. Whether you should want to apply them in any given scenario is a separate question. "This has the capability to be abused so it sucks". Really? Anything that does not have the capability to be abused is useless.

  • 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 to separate 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.

    What makes property groupings "StyleCop" compliant?

    I think there are at least a few defensible uses of #regions - e.g. collapsing of guard clauses (checking input parameters) which, if left uncollapsed, detract from the gist or "meat" of the method. Another example is exception handling on API boundaries; you often don't want to impact the stack trace by calling other methods to wrap the exception, so you have several lines of code to wrap and rethrow. This is also often non-relevant code, and can safely be collapsed.

    Reality Check. I've seen plenty of inter-method #regions that were helpful. When you've got a several hundred line method with nested control logic with dozens to hundreds of lines w/in some of those - thank goodness the idiot at least put in regions.

    @radarbob: in short, regions are helpful in crappy code which shouldn't exist in the first place.

    -1 (if I had the reputation). Even if your type members are well organised and separated, there might be a lot of them. Regions save you having to scroll past 10 or 12 properties and the associated getters and setters just to get to a method you want to work on. The only alternative is to collapse all your properties individually, which is not something I'm a fan of. The large scale show/hide functionality regions provide is extremely useful. I agree about intra-method regions though.

    @Asad: Why are you collapsing all properties individually? I would suggest to you to start using an IDE like Visual Studio which does that for you, and my answer covered only the situations where such IDE is used.

    @MainMa I'm already using Visual Studio (Express though, so maybe I'm missing some features). I can't collapse groups of class members, only individual class members, blocks inside them, or the whole class.

    @Asad: Strange. Try Ctrl + M, O (or right click → Outlining → Collapse to definitions). I would be surprised if Visual Studio Express doesn't have this feature.

    @MainMa I think I see what you mean, `Ctrl + M, O` collapses **everything** (recursively no less). I thought you were describing a way I could fold all fields into a single group, properties into another, and so forth for methods, internal types etc. Folding and unfolding everything simultaneously isn't very useful to me, since the point is to have a "region" of code that I'm focusing on front and center and other groups of interest neatly tucked away, only a *single* click away.

    @Asad: so you want to collapse only your 10-12 properties (which means 29-35 LOC including whitespace). Question: why? Won't you spend more time collapsing and uncollapsing your region then scroll up and down those 35 LOC?

    @MainMa I actually work mostly with VB, and can't fit getters, setters elegantly onto a single line as in C#. That means 10-12 properties (all of which are usually Readonly, and therefore not autoimplemented) take up about a hundred lines of code.

    @Asad: I'm not sure we are talking about the same thing. I'm not talking about auto-implemented properties, but properties with a fully-implemented custom getter and setter, collapsed to two LOC (one for the property header, another one for XMLDoc) with the help of Visual Studio. According to this screenshot, you can do that.

    @MainMa: What about ASP.NET WebForms and generated method InitializeComponent() - you cannot move it safely to another partial class, so best you can do is to surround it with regions (containing text generated code). For example to ensure not being processed by StyleCop/FXCop/Sonar.

    One acceptable case for regions I've found--when you have a whole block of data embedded in your code. Who needs to look at that table with all possible permutations of a set of values?

    I Strictly disagree with this answer: Code stink and region have nothing to do with each other. If your code stinks, it would stink with or without regions. The way I use regions is to segregate my classes into regions. Generally I stick to the same pattern though: Public Properties, Public Methods, Private Fields, Private Methods. That is the only use I would have regions for. Anything else, you are probably breaking SRP principal in your code.

    This convinces me, I'll replace my field/constructor/method grouping regions with comment decorations like `//---- getters -----------`. At least I can't fold them, which forces me to see how big a class becomes.

    @MainMa re: **regions are helpful in crappy code which shouldn't exist in the first place** - Which means my primary use is when working with old code that has evolved over decades into a bloody mess. I group things as I read to help me understand this old code, which I must do before it is safe to refactor. But you are right - `well written code that follows the standards of style` does not need regions. Sadly, for old code, that is a rare situation.

    I disagree with that one should extract methods instead of creating regions in existing method. I think in OOP you shouldn't have unnecessary private methods as it's more a part of functional programming. It's better to make a class OR [extract methods *only if they gonna be REused*]. There is no sense to have one more method if it's called from only one place and you have to navigate there to understand what it does so I prefer scopes and regions when it's not too nested. (But there *is* a sense to have a class even if it's used only in one place because you can substitude implementation).

    @Vlad if *you have to navigate there to understand what it does*, then you need better method names. *Abstraction* is also one of the 4 pillars of OOP AFAIK.

    @Mat'sMug ok, you are right about names but anyway if you need to open it you have to jump may be a few screens below... If it's called only one time I would not extract a method in such case.

    @Vlad that's Ctrl+Click in my IDE ;-)

    @Mat'sMug 1: you will still leave your current context in the source. 2: abstraction with methods is a part of functional programming; in OOP we use *classes for abstraction*.

    This answer would be valid if we used a brain-dead simple text editor for code files, not advanced IDE like Visual Studio with possible additional add-ons and tools for code folding and navigation. It doesn't matter I have my 50 events grouped all together. When I work on internal handlers I don't want to see them in code. Regions improve readability hugely in this case. Regions CAN be anti-pattern as everything: IF USED INCORRECTLY. If the author can't find the correct use for regions it doesn't mean it doesn't exist.

    I feel this answer applies to 99% of the time for 99% of circumstances, but I think it's heavy-handed to say "never". For example, Suppose I'm re-writing the `DateTime` class (actually I am, but not in C#). There are hundreds of fields and functions, and inheritance isn't really a good solution in this case. Most of my code recently is actually in complex mathematical optimisation heuristics, and the functions are often longer and more complex than those typically found for a web or desktop application. Long, complex functions with many parameters in several highly-coupled stages.

    @Ozzah: so you are suggesting to cram hundreds of lines of complex code in a single method, and then use regions to make it *look all right*? Wow. Hopefully I'll never have to be that poor guy who maintains your code. BTW, I already addressed this point in the *very first* part of my answer. Also, feel free to check yourself the *actual* source code of the `DateTime` class. How many regions do you see? Right, zero. And no “long, complex functions with many parameters in several highly-coupled stages” either.

    @ArseniMourzenko No need to be rude. I never said `DateTime` had long complex functions with many parameters; I said some parts of my code does (high performance scientific computing code run in competition scenarios). I said `DateTime` had a large number of functions - which it does. You could, for example, put the operator overloads in a region, the getters/setters in one region, the formatters in one region, etc. No need to scroll up and down 1000 lines of code when only working on a very small subset. As I said, I 100% agree with what you said 99% of the time. There are always edge cases.

    Is there a good use for regions? Yes, e.g. in order to separate members of an implicit interface implementation in C#. That may help other people understand _faster_ how a type is organized.

    A good use for a #region directive is to hide away a long block of documentation (like a copyright notice) in the top of a class.

    While I agree with this post, when working in VS Professional/Enterprise and CodeLens is on, that adds a lot of extra white space to properties. Being able to collapse those properties (say 20 properties takes up ~40 lines with CodeLens on) saves me from scrolling so much.

    All these comments made for an interesting read. I agree that regions should not be necessary in clean code. I've also seen the way it completely destroyed the readability of code. And I've seen bugs and even compile-time errors caused by region use. All in all, this made me write a rule in our coding guidelines, banning regions from regular code. The one exception is generated code, though.

    Why call it "InternalFoo", instead of "PrivateFoo"?

    @AustinWBryan I think perhaps because they are both private methods, but "internal" tells you that it is called only from "Foo". But I don't know.

    @ButtleButkus That would be very misleading if that was the case.

    @AustinWBryan well it would be a way to organize a class with a lot of non-public methods into groups. Of course, the code could be refactored so that each group of methods is its own class, perhaps. “Internal” is a weird choice of word, though.

    @AustinWBryan: as stated in my answer, this was the choice which was made by .NET Framework developers. Only them would be able to answer your question; anything else is only speculation.

License under CC-BY-SA with attribution


Content dated before 6/26/2020 9:53 AM