2008-07-10

Visual Studio #regions are a code smell

Why do #regions exists?

My best guess is that back in the the good old .NET 1.0 and 1.1 days, before partial classes, MS needed a way to visually remove, all the noise the auto generated Windows Forms code made. It kind a made sense, because you don't need to understand the auto generated code, you can just open the form in design view.

Why use #regions?

Often I see #regions used to group different code artifacts, like "Fields", "Constructors", "Properties", "Events", "Methods" etc. When the class gets big, navigating to an event handler becomes as easy as:

  1. Collapse all "regions.
  2. Expand the "Events" region
  3. Start to scroll for the event handler.

That is faster than scrolling through all the code in a big file.

Do we need #regions?

As with most other "coding tools", #regions tackle complexity. A class with several thousands lines of code, can be visually laid out in 20 lines or so. If you have a class with many, many lines of code, #regions do offer an advantage over normal layout. The #regions don't take up to much space compared to the total number of lines, so it is easy to ignore them if not needed. But if the #regions makes sense, they can be used to slice the lines into categories.

An obvious problem with #regions

Like any other categorization it falls apart if there are multiple dimensions. What if we are looking for an event handler, but the #regions define business rules?

My wife categorizes our pictures by month. She creates a folder with a "Month Year" name, and places pictures taken that month there. Every now and then we need to find a picture of someone, it could be my son. The problem with the "Month Year" categorization is that is does not help, we need a "who is on the picture" categorization.

If google created a fixed categorization, it would be much harder to find anything. Because the same topic could be categorized with many dimensions.

A not so obvious code smell lurking behind the #regions

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.

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. It is a well known anti-pattern, and will kill maintenance. I will not in depth with the god obejct anti-pattern, but it is well described elsewhere.

Moving away from the god class

Is it possible? It most certainly is...

CustomerDetailsForm

This form god'ified could look like this in code.

   1: using System;
   2: using System.Windows.Forms;
   3:  
   4: namespace GodClass
   5: {
   6:     public partial class CustomerDetails : Form
   7:     {
   8:         public CustomerDetails()
   9:         {
  10:             InitializeComponent();
  11:         }
  12:  
  13:         #region Events
  14:         private void CustomerDetails_Shown(object sender, EventArgs e)
  15:         {
  16:             DefaultFocus(_nameTextBox);
  17:         }
  18:  
  19:         private void _saveButton_Click(object sender, EventArgs e)
  20:         {
  21:             FocusAll(_nameTextBox, _name2TextBox, _addressTextBox, _address2TextBox, _phoneTextBox, _emailTextBox);
  22:             //Save the object
  23:         }
  24:  
  25:         private void _nameTextBox_TextChanged(object sender, EventArgs e)
  26:         {
  27:             TextBoxRequired(_nameTextBox);
  28:         }
  29:  
  30:         private void _name2TextBox_TextChanged(object sender, EventArgs e)
  31:         {
  32:             MoveTextToTextBoxIfTextIsEmpty(_nameTextBox, _name2TextBox);
  33:         }
  34:  
  35:         private void _addressTextBox_TextChanged(object sender, EventArgs e)
  36:         {
  37:             TextBoxRequired(_addressTextBox);
  38:         }
  39:  
  40:         private void _address2TextBox_TextChanged(object sender, EventArgs e)
  41:         {
  42:             MoveTextToTextBoxIfTextIsEmpty(_nameTextBox, _name2TextBox);
  43:         }
  44:  
  45:         private void _phoneTextBox_TextChanged(object sender, EventArgs e)
  46:         {
  47:             TextBoxRequired(_phoneTextBox);
  48:         }
  49:  
  50:         private void _emailTextBox_TextChanged(object sender, EventArgs e)
  51:         {
  52:             TextBoxRequired(_emailTextBox);
  53:         }
  54:  
  55:         #endregion
  56:  
  57:         #region Procedures
  58:         public void DefaultFocus(Control control)
  59:         {
  60:             control.Focus();
  61:         }
  62:  
  63:         public void TextBoxRequired(TextBox textBox)
  64:         {
  65:             if (string.IsNullOrEmpty(textBox.Text))
  66:             {
  67:                 textBox.Focus();
  68:             }
  69:         }
  70:  
  71:         public void FocusAll(params Control[] controlsToFocus)
  72:         {
  73:             foreach (Control controlToFocus in controlsToFocus)
  74:             {
  75:                 controlToFocus.Focus();
  76:             }
  77:         }
  78:  
  79:         public void MoveTextToTextBoxIfTextIsEmpty(TextBox firstTextBox, TextBox secondTextBox)
  80:         {
  81:             if (string.IsNullOrEmpty(firstTextBox.Text))
  82:             {
  83:                 firstTextBox.Text = secondTextBox.Text;
  84:                 secondTextBox.Text = "";
  85:             }
  86:         }
  87:         #endregion
  88:     }
  89: }

In addition, the tab order is set in the visual designer.

The problem with this sample is ofcourse, it is not several thousans of lines.

If we wanted to de-god'ify the class it could look like this.

   1: using System.Windows.Forms;
   2:  
   3: namespace NonGodClass
   4: {
   5:     public partial class CustomerDetails : Form
   6:     {
   7:         public CustomerDetails()
   8:         {
   9:             InitializeComponent();
  10:             new DefaultFocus(this, _nameTextBox);
  11:             new TextBoxRequiredMicroController(_nameTextBox);
  12:             new TextBoxRequiredMicroController(_addressTextBox);
  13:             new TextBoxRequiredMicroController(_emailTextBox);
  14:             new TextBoxRequiredMicroController(_phoneTextBox);
  15:             new ValidateFormMicroController(_saveButton, _nameTextBox, _name2TextBox, _addressTextBox, _address2TextBox,
  16:                                             _phoneTextBox, _emailTextBox);
  17:             new MoveTextToTextBoxIfTextIsEmptyMicroController(_nameTextBox, _name2TextBox);
  18:             new MoveTextToTextBoxIfTextIsEmptyMicroController(_addressTextBox, _address2TextBox);
  19:         }
  20:     }
  21: }

Things to notice, there are zero regions!

The microcontrollers look like this.

   1: using System.Windows.Forms;
   2:  
   3: namespace NonGodClass
   4: {
   5:     public class TextBoxRequiredMicroController
   6:     {
   7:         private readonly TextBox _textBox;
   8:  
   9:         public TextBoxRequiredMicroController(TextBox textBox)
  10:         {
  11:             _textBox = textBox;
  12:             _textBox.TextChanged += ((sender, e) => FocusIfEmpty());
  13:         }
  14:  
  15:         private void FocusIfEmpty()
  16:         {
  17:             if (string.IsNullOrEmpty(_textBox.Text))
  18:             {
  19:                 _textBox.Focus();
  20:             }
  21:         }
  22:     }
  23:  
  24:     public class ValidateFormMicroController
  25:     {
  26:         private readonly Button _button;
  27:         private readonly Control[] _controlsToFocus;
  28:  
  29:         public ValidateFormMicroController(Button button, params Control[] controlsToFocus)
  30:         {
  31:             _button = button;
  32:             _controlsToFocus = controlsToFocus;
  33:             _button.Click += ((sender, e) => FocusAll());
  34:         }
  35:  
  36:         private void FocusAll()
  37:         {
  38:             foreach (Control controlToFocus in _controlsToFocus)
  39:             {
  40:                 controlToFocus.Focus();
  41:             }
  42:         }
  43:     }
  44:  
  45:     public class DefaultFocus
  46:     {
  47:         private readonly Form _form;
  48:         private readonly Control _defaultControlToFocus;
  49:  
  50:         public DefaultFocus(Form form, Control defaultControlToFocus)
  51:         {
  52:             _form = form;
  53:             _defaultControlToFocus = defaultControlToFocus;
  54:             _form.Shown += ((sender, e) => FocusDefaultControl());
  55:         }
  56:  
  57:         public void FocusDefaultControl()
  58:         {
  59:             _defaultControlToFocus.Focus();
  60:         }
  61:     }
  62:  
  63:     public class MoveTextToTextBoxIfTextIsEmptyMicroController
  64:     {
  65:         private readonly TextBox _firstTextBox;
  66:         private readonly TextBox _secondTextBox;
  67:  
  68:         public MoveTextToTextBoxIfTextIsEmptyMicroController(TextBox firstTextBox, TextBox secondTextBox)
  69:         {
  70:             _firstTextBox = firstTextBox;
  71:             _secondTextBox = secondTextBox;
  72:             _secondTextBox.TextChanged += ((sender, e) => MoveText());
  73:         }
  74:  
  75:         private void MoveText()
  76:         {
  77:             if (string.IsNullOrEmpty(_firstTextBox.Text))
  78:             {
  79:                 _firstTextBox.Text = _secondTextBox.Text;
  80:                 _secondTextBox.Text = "";
  81:             }
  82:         }
  83:     }
  84: }

Even more these MicroControllers are reusable in the next form!

Things normally done(or forgotten) in the visual designer can also be micro controlled.
Tab order is as easy as.

   1: namespace NonGodClass
   2: {
   3:     public class TabOrderMicroController
   4:     {
   5:         public TabOrderMicroController(params Control[] controls)
   6:         {
   7:             for (int i = 0; i < controls.Length; i++)
   8:             {
   9:                 controls[i].TabIndex = i;
  10:             }
  11:         }
  12:     }
  13: }

Conclusion

If you use #regions, you might need to apply separation of concerns. This forces your code to be more readable, although it creates a navigation issue, when having lots of classes.

16 comments:

Søren Skovsbøll said...

Great post, Morten!

I'll tell my Code-Smell-O-Meter to look out for regions hiding large classes.

The microcontroller idea is neat. Maybe the syntax for setting them up could be made as a fluent interface?

/Søren

Jamie said...

Nice example - however, when I do use regions, I simply use them to segment the class into areas, so for example I can easily collapse the IDisposable implementation, or collapse all the properties and just see the methods (maybe that's just because I don't like scrolling, and I've learned to navigate using regions quite effectively).

RichB said...

So many people with the same idea. We really should petition Microsoft and get Regions removed:

http://aspadvice.com/blogs/rbirkby/archive/2008/05/12/Code-Smells-in-C_2300_.aspx

Anonymous said...

You might also wish to check out:
Contrived Complexity

Anonymous said...

I don't quite follow the logic here. You're suggesting we remove an organizational tool because it enables developers to create classes that you consider "too large"? Wouldn't that be like suggesting we abolish folders from the file system because it enables people to create too many files?

I think code outlining (regions) are one of the greatest inventions for pure coding since the IDE. Code outlining enables a developer to hide complexity and reduce a 20-page code file down to a single screen. For a disabled developer such as myself, it's a life saver, eliminating the painful need to scroll and scroll and scroll.

As for the belief that any code file which is helped by regions must be too long... Since when is there a byte limit on source code? In the real world, sometimes classes can become large. Look at System.Windows.Forms.Control or any UI Form that can have tens or hundreds of event handlers. I suppose the alternative is to use partial classes and spread a class among several files, but then you've simply swapped one organizational tool (regions) for another (files and folders).

For anyone who has built non-trivial, real-world business applications, code outlining is a terrific organizational tool.

RichB said...

DevTopics: Object Oriented Programming is a system which allows you to organize your code according to objects and the behavior of those objects. As soon as you start using Regions to act as your unit of modular organization, you immediately leave behind all the benefits of OO development and go back to module-based structured programming of yesteryear.

I've worked with a couple of blind developers in my time - and I'm sure they Class Browser window provides much better accessibility than region outlining. And partial classes were never designed as a solution for code organization. They work quite well for code-generation systems and really should be restricted for this use.

I've worked on several 100KLOC systems and they were all improved by refactoring along OO concepts. Along the way, you realize that regions become unnecessary - so they get refactored away. I don't have the ROTOR/SSCLI source on my disk right now, but I'd be interested to know how many regions the are amongst the million+ LOC.

The system I'm currently working on is an execution engine for a turing-complete programming language for financial calculations. I came in to help get the system back on track after becoming a fairly unstructured tangle of code. 700 line methods with dozens of nested regions were not uncommon.

Since I refactored, cyclomatic complexity has fallen sharply. LOC has been reduced by >30%. All regions have been removed. Avg Lines/Method has decreased massively. Bugs have become easier to spot and it is much more maintainable.

Anonymous said...

RichB,

Regions do not supersede OO encapsulation into objects, methods and properties. Code outlining is a meta-organizational tool that has no effect on object structure or relationships. It's simply a way to organize and hide complexity in a large code file.

I also use the Visual Studio member list, class browser and third-party tool Visual Sidekick to quickly navigate among and within code files.

What code outlining does is reduce the visual clutter and navigational challenges. Instead of seeing possibly hundreds of methods and properties all at once, I only see the one or two methods that I'm working on at the moment.

To each his own, but I'm very glad that C# supports code outlining.

RichB said...

Hi devtopics,

You don't need regions to collapse a method/property. Ctrl-M M will collapse any member.

This gives you more flexibility to hide arbitrary members than regions would.

Anonymous said...

Re: You don't need regions to collapse a method/property. Ctrl-M M will collapse any member. This gives you more flexibility to hide arbitrary members than regions would.

So you are NOT adverse to the concept of code outlining! Terrific! However, you advocate outlining solely based on members, but are against any discretionary outlining using regions.

But what if I want to group members in a certain way? Or if I want to group a few statements within a method?

Given that you support code outlining, I'm struggling to see why you want to prevent me from performing additional outlining with regions that will reduce my visual and navigational complexity.

It's like dictating that folders be abolished from the file system and requiring all file access through Google search. Each access method has its own use, benefits and costs, but both are valuable.

RichB said...

DevTopics wrote:
> So you are NOT adverse to the
> concept of code outlining!

Not at all. Code outlining doesn't change what is checked into version control. Whereas with region outlining you are introducing no-op tokens into the compiler parse stream.

> But what if I want to group
> members in a certain way? Or if I
> want to group a few statements
> within a method?

Code smell again. Many devs will use vertical whitespace to visually group lines of code together. If they're good devs, they may place a comment above that group. This is a code smell and indicates you should be creating a new method. End Result: you can get rid of the comment because the new method fully describes the functionality by way of its variable name.

> Given that you support code
> outlining, I'm struggling to see
> why you want to prevent me from
> performing additional outlining
> with regions that will reduce my
> visual and navigational complexity.

Because region based outlining is code which does not get statically checked by the compiler. It is oblivious to reflection and a lot of code inspection tools. For the same reason, I advocate verbosity of variable names instead of terse variable names each with an explanatory comment.

> It's like dictating that folders
> be abolished from the file system
> and requiring all file access
> through Google search.

folders are not the right analogy. I'm not against structure. Quite the opposite - I'm advocating structure by being against regions.

You see, a folder structure is akin to the class/method structure of OO development. But if I began introducing grouping into the filenames (eg all files start with ! to sort first, or all files related to 2008 start their name with '2008') it would be a closer analogy.

Structure by convention isn't the correct way to introduce structure. The compiler gives us the means to introduce static structure, so we should use it.

Anonymous said...

Re: Whereas with region outlining you are introducing no-op tokens into the compiler parse stream.

RichB, you provide a lot of esoteric, theoretical arguments against discretionary code outlining.

Yet in the real world trenches of hand-coding, code outlining is a tangible tool that significantly reduces complexity and visual clutter while improving navigation and productivity.

I've been writing code for longer than I'd like to admit (nearly 3 decades), and from this old-timer's perspective, code outlining is one of the best inventions for hand-coding I've seen during my career, right up there with Intellisense.

While regions may be taboo in your code shop, they are highly prized in ours. Regards!

Anonymous said...

your mom is code smell.

Anonymous said...

Well said. Regions ARE a code smell.

rsenna said...

I know I'm quite late to this discussion. But just let me reiterate how #region is BAD for code. If you need regions, what you really need is big re-factoring.

RichB, you have given lots of good, pragmatic advice. Nothing "esoteric" about what you've said.

akjoshi said...

Regions may be code smell, so go and do refactoring; who the hell is stopping you from doing that, its not necessary to remove regions to do that.

Isn't that great to have both of both worlds, refactored code with regions; so you have good code and nicely organized code; whats problem with that?

Anonymous said...

"Use of regions may lead to bad practices. It should be banned".

Well.

Programming may lead to bad practices. Therefore the act of programming should be banned.