Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

I hear a lot about keeping methods short and I've heard a lot of programmers say that using #region tags within a method is a sure sign that it is too long and should be refactored into multiple methods. However, it seems to me that there are many cases where separating code with #region tags within a method is the superior solution to refactoring into multiple methods.

Suppose we have a method whose computation can be separated into three rather distinct phases. Furthermore, each of these stages is only relevant to the computation for this method, and so extracting them into new methods gains us no code reuse. What, then, are the benefits of extracting each phase into it's own method? As far as I can tell, all we gain is some readability and a separate variable scope for each phase (which will help prevent modifications of a particular phase from accidentally breaking another phase).

However, both of these can be achieved without extracting each phase into its own method. Region tags allow us to collapse the code into a form which is just as readable (with the added benefit that we no longer have to leave our place in this file should we decide to expand and examine the code), and simply wrapping each phase in {} creates its own scope to work with.

The benefit to doing it this way is that we don't pollute the class level scope with three methods which are actually only relevant to the inner workings of a fourth method. Immediately refactoring a long method into a series of short methods seems to me to be the code-reuse equivalent to premature optimization; you are introducing extra complexity in order to address a problem which in many cases never arises. You can always extract one of the phases into its own method later should the opportunity for code reuse arise.

Thoughts?

share|improve this question
3  
I am currently working with a class that is fairly large. It follows the Single Responsibility Principle; everything in the class is required functionality. It has a lot of private methods, mostly due to refactoring. I use #region to divide these methods into functional categories, which has worked out very well for us. There are many other classes in this project that did not require such treatment. The right tool for the right job, I say. –  Robert Harvey Nov 9 '11 at 18:28
2  
@RobertHarvey, using them in a class is different than using them inside a method. –  CaffGeek Nov 9 '11 at 18:40
16  
@Chad: Ah, didn't notice that. Using them in a method does seem a bit extreme. If a method is so long that it requires #regions, I refactor it into separate methods. –  Robert Harvey Nov 9 '11 at 18:43
7  
Not really an answer, but not only do I hate all #region tags, I turn off code folding in Visual Studio altogether. I don't like code that tries to hide from me. –  Daniel Pryden Nov 10 '11 at 1:01
2  
"Furthermore, each of these stages is only relevant to the computation for this method, and so extracting them into new methods gains us no code reuse". OTOH, I find that if I split a function into 3, I often find later that the individual parts <i>are</i> useful later, eg. if some input data changes only one phase, you can test that in isolation, etc. –  Jack V. Nov 11 '11 at 9:42

9 Answers 9

up vote 44 down vote accepted

All you should ever care about is for your code to be usable, not reusable. A monkey can transform usable code to reusable code, if there are any transformations to be done at all.

The argument "I need this only here" is poor, to put it politely. The technique you're describing is often referred to as the headlines technique and is generally frowned upon.

  1. You can't test regions, but you can test true methods in isolation.
  2. Regions are comments, not syntactic elements. In the worst case the nesting of your regions and your blocks contradict each other. You should always strive to represent the semantics of your structure with the syntactic elements of the language you are using.
  3. After refactoring to methods, one no longer needs folding to read the text. One might be looking at source code in any tool that doesn't have folding, like a terminal, a mail client, a web-view for your VCS or a diff-viewer.
  4. After refactoring to methods, the resulting method is at least as good as with regions, plus it is simpler to move the individual parts around.
  5. The Single Responsibility Principle suggests that any unit should have one task and one task only. The task of the "main" method is to compose the "helper" methods to get the desired result. The "helper" methods solve a discrete, simple problem in isolation. The class containing all these methods should also only fulfill one task and only contain the methods related to that task. So the helper methods either belong into the class scope, or the code shouldn't be in the class in the first place, but at least in some global method or, better yet, should be injected.

Also relevant: Jeff Atwoods's thoughts on code folding

share|improve this answer
6  
1. Most programmers don't test all private methods. 2. A method name can't convey every thing there is to know about that method; what exceptions might be thrown? Is null a valid input? Sometimes it's OK to use non-syntactic constructs to make your code more understandable. In many languages, files are an example of a non-syntactic element that is fairly widely accepted as a means for organizing code. 3. Diving into the inner-workings of a method is best attempted in an IDE for a variety of reasons; situations where regions could bite you in your VCS or mail client are rather rare. –  jjoelson Nov 9 '11 at 18:51
2  
4. But you've just exported some serious complexity into the rest of your class. Is that worth it? Also, regions can be moved around as easily as a method call. 5. Now your talking about polluting your namespace with more classes which are only going to be used in a single place. Why are classes and methods the only units of encapsulation that you will accept? You can always create that new class later when (or if) it becomes necessary to have this separate. –  jjoelson Nov 9 '11 at 18:54
4  
@jjoelson: 1. So what? 2. Exactly my point: it is ok to use non-syntactic constructs if (and only if) syntax alone is not enough. 3. "Rare?" - I suppose you have numbers to show that. I can tell you for sure that the diff (or blame or whatever for that matter) tool bundled with TortoiseSVN doesn't have folding. 4. How is that related to the point I made. 5. That is why most modern languages have nestable namespaces. You are resorting to ASCII art rather than using a broad palette of available language elements to structure your code. –  back2dos Nov 9 '11 at 19:12
3  
@jjoelson: 1. That is your opinion and out of the scope of this question. 2. By extension of my argument nested functions are better than regions, yes. 3. Because I sometimes need to browse through a number of revisions to track a problem. Either way, source code is written for humans, not IDEs. 4. For one the argument is unrelated to my point, secondly that is yet again just your opinion, thirdly it's out of the scope of this question. 5. C# has other means of structuring code than nesting function. You should either use those, or a language that allows nesting functions. –  back2dos Nov 9 '11 at 20:14
10  
You guys should take this to chat if you still wish to debate... I have nothing left worth saying to the OP, he is a typical opinionated junior developer set in his beliefs. Nothing will change his mind but more experience IMO. –  maple_shaft Nov 9 '11 at 20:24

It's not the regions in the methods that are the problem, but the case when there's a need (or even a use) to put regions in methods.

Having had to debug plenty of 200-300 line methods, I can tell you that I wouldn't wish it on anyone. If you're writing and looking after your own code that's fine - do whatever you want. But once someone else looks at it, it should be immediately clear what a method is doing.

"First it gets the things, and then it jigs them around, then if they need buzzing it does that, otherwise it checks whether they are blue and inverts their Copernicus value. Then, if the second thing is greater than the first thing, we need to get the juice box and insert it... "

No, that is not good enough. Break it up into regions all you want but I'm still shaking my head just thinking about it.

If you break into methods you get many benefits:

  • Clearer understanding of what's happening where, even if just through the name of the method. And you are wrong that a method can't be fully documented - add the documentation block (hit ///) and enter the description, parameters, return type, and also exception, example, remarks nodes to detail those scenarios. That's where it goes, that's what it's for!
  • There are no side effects or scoping problems. You may say that you declare all your variables in a sub-scope with {}, but do I have to check every method to make sure you didn't 'cheat'? Is this dodgyObject I'm using here only because used in this region, or did it come from somewhere else? If I were in my own method I'd be easily able to see whether it was passed to me or whether I created it myself (or rather, whether you created it).
  • My event log tells me which method an exception occurred in. Yes, I may be able to find the offending code with a line number, but knowing the method name (especially if I've named them properly) gives me a very good indication of what happened and what went wrong. "Oh, the TurnThingsUpsideDown method failed - it's probably failing while turning a bad thing" is a lot better than "Oh, the DoEverything method failed - it could have been 50 different things and I'll have to dig around for an hour to find it".

This is all before any reusability or improvability concerns. Properly separated methods obviously facilitate reuse, and also allow you to easily replace a method that's not performing (too slow, too buggy, changed dependency etc). Have you ever tried refactoring any of these huge methods? There's no way to know that what you're changing won't effect anything else.

Please properly encapsulate your logic into reasonably sized methods. I know that it's easy for them to get out of hand, and arguing that it's not worth redesigning once at that point is another issue. But you must accept the fact that there is no harm and at least potential benefit in having properly encapsulated, cleanly written and simply designed methods.

share|improve this answer
    
Visual Studio's XML comment parsing, along with the #region tags, falls under the category of Visual Studio features. My whole point is that it's not wrong to rely on these features if everyone in your project is using Visual Studio. As for side effect and scoping problems, you can still screw things up with global fields. In either case you must rely on the programmers not to do silly things with side effects. –  jjoelson Nov 9 '11 at 23:07
3  
@jjoelson You can still read the XML comments without visual studio, but the region tags are almost completely useless outside VS. The logical extension of your argument is one giant method that runs your entire application is okay, as long as you have it separated into regions! –  Kirk Broadhurst Nov 10 '11 at 0:36
1  
@jjoelson please don't get us started on how bad global fields are. Saying something is not useful because you have a "workaround" to screw it up anyway is just silly. Just keep you methods short and variables scoped. –  OliverS Nov 10 '11 at 13:56
    
@OliverS, Kirk was the one who brought up shared state as a problem with my regions-in-a-method scheme. Basically, I was saying exactly what you are saying: the fact that a programmer could abuse state by sharing too many variables between regions is not an indictment of the entire scheme, just as the ability to screw up state shared between methods via globals isn't an indictment of the multiple-method scheme. –  jjoelson Nov 10 '11 at 14:36

I think that the example you give in your argument is less about YAGNI (You ain't gonna need it) and more about writing proper quality code that is easily maintainable.

In the earliest programming courses we learn that if a method is 700 LOC long, it is probably too big and certainly would be a giant pain to try and debug.

Secondly if I write methods A, B and C that are only used within method D, then I can more easily unit test A B and C independently of D, allowing for more robust unit tests in all 4 methods.

share|improve this answer
    
+1 for unit testing. –  CaffGeek Nov 9 '11 at 17:59
    
Unit testing is certainly a fair point, but I'm not sure it justifies pollution of the class scope in every case. Realistically speaking, does anyone actually unit test every little private method they write? I'd say people tend to unit test the private methods that get used a lot, that is to say, private methods which would be candidates for code-reuse anyway. –  jjoelson Nov 9 '11 at 18:04
    
@jjoelson If the method has meaningful logic and isn't wrapping some other functionality then I always write a unit test against it. Unit testing methods has nothing to do with code reuse. You should unit test methods to verify that for given inputs that you get expected outputs in a consistent and repeatable manner. If you are noticing that the class scope is getting polluted as you call it, then perhaps your class is violating the Single Responsibility Principle. –  maple_shaft Nov 9 '11 at 18:11
2  
Most programmers that I've met disagree with the idea of unit testing every private method. It's just not worth the time time it takes to implement and maintain tests on all private methods. –  jjoelson Nov 9 '11 at 18:33

Suppose we have a method whose computation can be separated into three rather distinct phases. Furthermore, each of these stages is only relevant to the computation for this method, and so extracting them into new methods gains us no code reuse. What, then, are the benefits of extracting each phase into it's own method? As far as I can tell, all we gain is some readability and a separate variable scope for each phase (which will help prevent modifications of a particular phase from accidentally breaking another phase).

Those are two benefits. But the important one, to me anyway, is debuggability. If you have to trace through that code, it's a lot simpler to be able to Step Over two of the three sections and only trace into the one that you care about. Refactoring into smaller methods allows this; region tags do not.

share|improve this answer
1  
ctrl-f10 - run to cursor –  Steven Jeuris Nov 9 '11 at 23:58

If your method is complex enough that it can be separated into three distinct phases, then not only should those be three separate methods...but your method should in fact be a separate class, dedicated to that computation.

"But then my class will have only one public method!"

You're right, and there's nothing wrong with that. The idea of the single responsibility principle is that your class does one thing.

Your class can call each of the three methods in turn, and return the result. One thing.

As a bonus, now your method is testable because it's not a private method anymore.

But never mind one class; in reality you should have four: One for each of the pieces of your method, plus one that takes each of the three as a dependency and calls them in the proper order, returning the result.

This makes your classes even more testable. It also makes it easy for you to change one of those three pieces. Just write a new class inheriting from the old one, and override the changed behavior. Or create an interface for the behavior of each of your three pieces; and then to replace one, just write a new class implementing that interface and substitute it for the old one in your dependency injection container configuration.

What? You're not using dependency injection? Well, you probably haven't seen the need yet, because you're not following the single responsibility principle. Once you start, you'll find that the easiest way to give each class its dependencies is to use a DI container.

EDIT:

OK, let's set aside the refactoring question. The purpose of #region is to hide code, which mainly means code that doesn't need to be edited under any normal circumstances. Generated code is the best example. It should be hidden in regions because ordinarily you don't need to edit it. If you do need to, then the act of expanding a region gives you a bit of "Here be dragons"-style warning to make sure you know what you're doing.

Therefore I would say #region should not be used in the middle of a method. If I open a method, I want to see the code. Using #region gives me another level of hiding that I don't need, and that becomes an annoyance: I unfold the method...and still can't see any code.

If you're worried about people seeing the structure of the method (and you refuse to refactor it), then add banner comments like this:

//
// ---------- COMPUTE SOMETHING OR OTHER ----------
//

These will help someone browsing the code see the individual parts of your method without having to deal with expanding and collapsing #region tags.

share|improve this answer
2  
All of this for a method which gets called in one place? Even most hardcore Lispers won't abstract out a higher order function until they've identified at least two places where it can be used to more easily define a function. –  jjoelson Nov 9 '11 at 21:29
    
@jjoelson Is it so hard to make a new class? What is it, one line, one opening brace and one closing brace. Oh, and you have to press ctrl+n –  Kirk Broadhurst Nov 9 '11 at 22:17
2  
It's not that it's difficult to make a new class, but there is some overhead to having an extra class. It's one more layer of indirection which makes it more difficult for a code maintainer to find what she's looking for. It's not that big of a deal, but it also doesn't buy you anything in the case where this bit of code is unlikely to be called from anywhere else. And, as you mentioned, it is quite easy to make that new class and put the code there if it turns out you do need to call this bit of code in multiple places. –  jjoelson Nov 9 '11 at 22:41
1  
@jjoelson, this is a pretty common reaction. It was mine, when I first learned about things like SRP and DI, and it was my co-workers' reaction when we started breaking out dependencies. If you're looking at one individual case, it doesn't seem worth it. The benefit is in the aggregate. Once you start doing SRP with all your classes, you find that it's far easier to change a piece of your code without the changes rippling through half your application. –  Kyralessa Nov 10 '11 at 2:13
    
@Kyralessa, these are methods that are only used in a single place. Changing such a method won't ripple through half your application. If such code is implemented as a region in the method that uses it, then you have perfect encapsulation; only the containing method is using the code and so you're free to change it all you want without worrying about the effect of it except in that method. –  jjoelson Nov 10 '11 at 13:29

My personal rule is that if a method is longer than one screen, say 40 lines, it is too long. If a class is longer than 500 lines, it is too big. I break these rules sometimes, occasionally even by a large multiple, but I generally regret it within the year.

Even a method that is 20 to 30 lines is getting a bit long in most cases. So I would say using regions in a method is usually a bad idea, simply because it would almost never make sense to collapse a region of 20 to 30 lines into multiple sub-regions.

Breaking down functions that are long by this definition has at least two benefits:

  1. You get to put a name on what those sub-functions are doing, which clarifies your present thinking and simplifies the task of later maintainers.

  2. Decomposing into smaller functions forces you to pass only the parameters those smaller functions need, so the scope of the data they require and modify is very clear. This assumes you aren't accessing and modifying object state in a hundred different leaf functions, which I would also discourage.

In my experience, these rules produce code that is easier to write without making common mistakes and can be understood and modified later without breaking subtle behaviors. It doesn't matter if the person who has to understand/modify the code is someone else, or myself after I have slept and forgotten everything.

So I would consider regions in methods to be a "code smell" indicating that unsustainable practices are being applied. As always, there could be exceptions, but they occur so infrequently that they almost aren't worth discussing.

share|improve this answer

Here we go again ... There are plenty of other topics here on programmers which have ended up in the same discussion.

You point out some very interesting arguments relating to short functions. It is not a popular statement, but I am with you on this. After having discussed it oft times before, it is my impression the discussion usually boils down to whether you primarily focus on proper encapsulation, or take test driven development to the max. These are the two main contenders in what seems to be turning out yet another holy war, and I'm afraid we are on the under powered side.

However ...

The solution in my opinion definitely isn't wrapping the 'phases' in regions. Code folding is used to sweep code under the rug. Leave the code there as it is, it might remind you that you might want to refactor that at a later time! To relate it to an argument in your question: how will you be able to see an opportunity for code reuse if you don't see any code? Long segments of code should be exactly that, a thorn in the eye which makes you want to refactor it.

As an appropriate replacement for regions, I suggest using code paragraphs as Alex Papadimoulis names them in a comment. You might actually already be using a similar approach. It are simply blocks of code with a comment header, explaining what the entire block does. You have the readability of functions, but are able to use normally spaced english sentences, and you don't lose any encapsulation.

share|improve this answer
    
Well I essentially use regions in order to delimit code paragraphs (regions can have comments which are visible even when the code is folded). I like using regions for this because it gives the maintainer the choice to see the code if she wants to examine the implementation or to fold it if she only wants to see the "big picture." –  jjoelson Nov 10 '11 at 13:34

Although I agree that usually it's better to refactor code than to use #region, it's not always possible.

To support that statement, let me give an example.

class Foo : IComparable
{
  private String name;
  private int foo;
  private Bar bar;

  public int CompareTo(Foo other)
  {
#region Ascending on name
     if (this.name < other.name) return -1;
     if (this.name > other.name) return 1;
#endregion
#region Usual order on bar
     int compBar = bar.compareTo(other.bar);
     if (compBar != 0) return compBar;
#endregion
#region Descending on foo
     if (this.foo > other.foo) return -1;
     if (this.foo < other.foo) return 1;
#endregion
     return 0; // equal
  }

It's easy to readjust the regions to alter the order, or to extend when addional fields are added to the class. Comments are required anyway to quickly see how the ordering is supposed to work. And most of all: I don't see how refactoring will help readability in this case.

However, those exceptions are rare. Usually it is possible to refactor, like many of the other answers say.

share|improve this answer

I’ve seen lots of C# code over the last few days, and 100% of it contains the #region construct. So far, I have see it used in only one way: to hide big code blocks. Some of you might be saying “Duh! That’s what they’re for!”.

Well, let’s think about that for a minute:

You need a language construct so that your IDE can help you hide your big messy code block from you (because it’s just so hideous, you don’t even want to look at it anymore)?

If your code is so bad, that you just want to shove it under the covers, then I would argue that your design and the solution to said design is too complex.

Most of the uses I’ve seen of regions are to group member fields, constructors (and related overloads), methods and properties together. This has to be the most retarded way to use a tool because you have other tools to perform this same function. Granted not everyone can buy Resharper, but the File Structure window provides all that same information. Heck, if you simply followed a simple code standard such as:

class <className>
{

  private members
  constructors
  properties
  methods

}

this would do the same job. Honestly, if you can’t LOOK at your code and tell a constructor apart from a property or field, then you should be looking for a different line of work. Please, stop commenting the obvious!

Now, you will eventually get classes that are thousands of lines long, however there are other constructs in the language that can help you keep things neat without using this lame excuse for a preprocessor tag. When you DO find your class encroaching on this large line number threshold, that is a prime time to look at your code for copy/paste refactoring tragedies and do it right. It might be a little painful now, but it could be the thing that saves your project in the long run.

Another lame way I’ve seen regions used is within a method. Usually there is a large block of setup code that is copy and pasted all over the place, with the same region tags. C’mon people, at that point, read about the Extract Method refactoring and actually REUSE that block of code intelligently!

I honestly can’t think of any reason to use #region tags because all the things they are hiding is just really bad code. When you start to just hide bad code, you are less inclined to go back and refactor it properly.

Let’s try this analogy: if you were looking to lose weight before the summer beach season, the best thing to do is get a mirror. This way, you will be re-motivated on a regular basis.

The mere act of having the bad code stare back at you will make sure that it is at least not forgotten. Nearly all codebases have some code that is not koesher. Let’s try to make that lump of code as small as possible. By then, you may even find a way to get rid of that eyesore (and system-sore) all together…after all, it was probably some hack anyway.

share|improve this answer
2  
Your answer would be better if you provided a basis behind your strong opinions. Your answer starts with an absolute dictat, but without backing. You then start to contradict yourself by citing an example of your own which uses regions as implied comments. ie. "All of these things belong together." –  GlenH7 Sep 9 '13 at 12:38
1  
now is it better or not.? –  ketan italiya Sep 9 '13 at 12:56
    
I may be wrong but unless this is from your blog then this entire post has been plagiarized (direct copy/paste): extractmethod.wordpress.com/2008/02/29/just-say-no-to-c-regions –  Mike D. Apr 2 at 6:39

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.