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.

So looking around earlier I noticed some comments about long methods being bad practice.

I am not sure I always agree that long methods are bad (and would like opinions from others).

For example I have some Django views that do a bit of processing of the objects before sending them to the view, a long method being 350 lines of code. I have my code written so that it deals with the paramaters - sorting / filtering the queryset, then bit by bit does some processing on the objects my query has returned.

So the processing is mainly conditional aggregation, that has complex enough rules it can't easily be done in the database, so I have some variables declared outside the main loop then get altered during the loop.

variable_1 = 0
variable_2 = 0
for object in queryset :
     if object.condition_condition_a and variable_2 > 0 :
     variable 1+= 1
     .....
     ...    
     . 
      more conditions to alter the variables

return queryset, and context 

So according to the theory I should factor out all the code into smaller methods, so That I have the view method as being maximum one page long.

However having worked on various code bases in the past, I sometimes find it makes the code less readable, when you need to constantly jump from one method to the next figuring out all the parts of it, while keeping the outermost method in your head.

I find that having a long method that is well formatted, you can see the logic more easily, as it isn't getting hidden away in inner methods.

I could factor out the code into smaller methods, but often there is is an inner loop being used for two or three things, so it would result in more complex code, or methods that don't do one thing but two or three (alternatively I could repeat inner loops for each task, but then there will be a performance hit).

So is there a case that long methods are not always bad? Is there always a case for writing methods, when they will only be used in one place?

UPDATE: Looks like I asked this question a over a year ago.

So I refactored the code after the (mixed) response here, split it into methods. It is a Django app retrieving complex sets of related objects from the database, so the testing argument is out (it would have probably taken most of the year to create relevant objects for the test cases . I have a "this needs done yesterday" type work environment before anyone complains). Fixing bugs in that part of the code is marginally easier now, but not massively so.

before :

#comment 1 
bit of (uncomplicated) code 1a  
bit of code 2a

#comment 2 
bit of code 2a
bit of code 2b
bit of code 2c

#comment 3
bit of code 3

now:

method_call_1
method_call_2
method_call_3

def method_1 
    bit of (uncomplicated) code 1a  
    bit of code 2a

def method_2 
    bit of code 2a
    bit of code 2b
    bit of code 2c

def method_3
    bit of code 3
share|improve this question
138  
All absolutes are bad. Always. –  Joachim Sauer Oct 15 '12 at 11:27
29  
I see the "extract methods if you can reuse them" argument quite often (in this or similar forms), but I don't buy it: if the method is doing more than one thing, you should extract methods from it for readability/maintainability even if those new methods are called from only one spot in your code. –  Joachim Sauer Oct 15 '12 at 11:40
1  
In one of my projects I have been explicitly forced by manager and tech lead to write long methods. We were coding an optimized multimedia processing, compiler was not capable of inlining and every method call cost whopping 5 CPU ticks which was unacceptable –  gnat Oct 15 '12 at 12:16
3  
@gnat: wow, wouldn't some kind of manual inlining (via a preprocessor) in the build-step be a better solution here? –  Joachim Sauer Oct 15 '12 at 12:22
8  
One of the best programmers I know commented that if you really want a measure, the number of local variables is a better one than actual length. He was working on a complex path-optimizer where the guts was one method several hundred lines long, but the amount of state (local variables) being kept was very small. –  Chuu Oct 15 '12 at 19:46
show 11 more comments

18 Answers 18

up vote 70 down vote accepted

No, long methods are not always bad.

In the book Code Complete, it is measured that long methods are sometimes faster and easier to write, and don't lead to maintenance problems.

In fact, what is really important is to stay DRY and respect separation of concerns. Sometime, the computation is just long to write, but really won't cause issue in the future.

However, from my personal experience, most long methods tend to lack separation of concern. In fact, long methods are an easy way to detect that something MAY be wrong in the code, and that special care is required here when doing code review.

EDIT: As comments are made, I add an interesting point to the answer. I would in fact also check complexity metrics for the function (NPATH, cyclomatic complexity or even better CRAP).

In fact, I recommend to not checking such metrics on long functions, but to include alert on them with automated tools (such as checkstyle for java for instance) ON EVERY FUNCTION.

share|improve this answer
34  
+1: "No, long methods are not alway bad" but they're nearly always bad –  Binary Worrier Oct 15 '12 at 11:41
58  
Long method bodies are a classical code smell: it's not in itself a problem, but it's an indication that there's probably a problem there. –  Joachim Sauer Oct 15 '12 at 11:46
6  
+1, but I'd still recommend checking the cyclomatic complexity of the long method. High values indicate methods which are effectively impossible to unit test (and long methods are very rarely devoid of control flow logic). –  Daniel B Oct 15 '12 at 12:12
11  
I use method names to minimize comments. Which sometimes leads to stuff like "getSelectedNodeWithChildren", but my colleague keep telling me, that my code is nicely readable. I also try to avoid abbreviations, they are nice to write, but not so nice to read. –  K.. Oct 15 '12 at 13:09
4  
@da_b0uncer That is also a policy I follow. It is harder to read code than to write it, so extra effort when writing to make code more readable does pay back. –  deadalnix Oct 15 '12 at 13:12
show 7 more comments

Most of the focus here seems to be around the word always. Yes, absolutes are bad, and software engineering is almost as much art as it is science, and all that... but I'm going to have to say that for the example you gave, the method would be better if it was split up. These are the arguments I'd typically use to justify splitting up your method:

Readability: I'm not sure about others, but I can't read 350 lines of code quickly. Yes, if it's my own code, and I can make many assumptions about it, I could skim through it very rapidly, but that's besides the point. Consider how much easier that method would be to read, if it consisted of 10 calls to other methods (each with a descriptive name). Doing this, you've introduced a layering in the code, and the high-level method gives a short, sweet outline to the reader regarding what's going on.

Edit - to put that in a different light, think about it is like this: how would you explain that method to a new team member? Surely it has some structure that you can summarise along the lines of "well, it starts off doing A, then B, then sometimes C, etc". Having a short "overview" method calling other methods makes this structure obvious. It's exceedingly rare to find 350 lines of code which do not benefit; the human brain is not meant to be dealing with lists 100s of items long, we group them.

Reusability: Long methods tend to have low cohesion - they often do more than one thing. Low cohesion is the enemy of reuse; if you combine multiple tasks into one method, it's going to end up being re-used in fewer places than it should have been.

Testability and cohesion: I mentioned cyclomatic complexity in a comment above - it's a pretty good measure of how complex your method is. It represents the lower bound of number of unique paths through your code, depending on the inputs (edit: corrected as per MichaelT's comment). It also means that to properly test your method, you'd need to have at least as many test cases as your cyclomatic complexity number. Unfortunately, when you put together pieces of code that aren't really dependent on each other, there's no way to be sure of this lack of dependency, and the complexity tends to multiply together. You can think of this measure as an indication of the number of different things you are trying to do. If it's too high, it's time to divide and conquer.

Refactoring and structure: Long methods are often signs that some structure is lacking in the code. Often, the developer couldn't figure out what the commonalities are between the different parts of that method, and where a line could be drawn between them. Realising that a long method is a problem, and trying to split it into smaller methods is the first step on a longer road to actually identifying a better structure for the whole thing. Perhaps you need to create a class or two; it's not necessarily going to be more complex in the end!

I also think that in this case, the excuse for having a long method is "... some variables declared outside the main loop then get altered during the loop". I'm not an expert on Python, but I'm fairly certain that this problem could be trivially fixed by some form of passing by reference.

share|improve this answer
1  
Minor correction. Cyclomatic complexity is the lower bound for the number of paths (the npath complexity) through some code. Consider the code at en.wikipedia.org/wiki/… which has a cyclomatic complexity of 3, but there are 4 paths through the code. –  MichaelT Oct 15 '12 at 13:38
9  
+1 for disavowing the always part of the question and focusing on the meat: Whether or not long methods are bad. I think the OP is looking for justification as though his scenario is an edge case, though usually when I hear people explain away their bad practices as necessary for the uncommon scenario, it's just because they didn't try very hard to use good practices. The uncommon scenarios are truly very uncommon, long methods are sadly however quite common. –  Jimmy Hoffa Oct 15 '12 at 14:58
1  
OK looking at the list above: readability I would say from past experience is increased by having the method longer, containing lots of comments, and well formatted, rather than having to jump about the code from method to methods Though this is probably quite subjective. I don't expect the parts of the code to be reused. Most of the reuse of the code is achieved from inheritance at the moment. –  wobbily_col Oct 15 '12 at 16:03
5  
@wobbily_col You say that having to jump around too much to understand the code with many methods is confusing, I think the missing point here is on the importance of naming. If a method is well named, you do not need to look at it to find out what it's doing, the calling method should be completely comprehensible without any underlying knowledge of what the methods it calls are doing, for instance have you ever used someVar.toString() and felt you needed to see the code of toString to know what it is doing? You just read right past it because of good method naming. –  Jimmy Hoffa Oct 15 '12 at 22:59
4  
On a side note, having a method that needs n parameters is also a code smell and indicates that the method may do more than one thing. Same goes for having a method thats hard to name. And if it really needs all those parameters, they usually are a part of a bigger concept, that can and should be enclosed in a class of its own. Of course, we could come up with an example thats better off not using this rule - my point is that if you see such a method, investigate it throughly, it probably is bad in some way. –  K.L. Oct 16 '12 at 9:34
show 7 more comments

Long methods are always bad, but are occasionally better than the alternatives.

share|improve this answer
1  
without an explanation, this answer may become useless in case if someone else posts an opposite opinion. For example, if someone posts a claim like "Long methods are never bad, but are occasionally worse than the alternatives.", how would this answer help reader to pick of two opposing opinions? Consider editing it into a better shape –  gnat Oct 18 '13 at 18:39
add comment

Long methods are a code smell. They usually indicate that something's wrong, but it's not a hard-and-fast rule. Usually cases where they are justified involve lots of state and fairly complex business rules (as you have found).

As to your other question, it's frequently helpful to segregate chunks of logic into separate methods, even if they're only called once. It makes it easier to see the high-level logic, and can make exception handling a little cleaner. Just as long as you don't have to pass in twenty parameters to represent the processing state!

share|improve this answer
add comment

Let's just say there are good and bad ways to break up a long method. Having to "[keep] the outermost method in your head" is a sign that you're not breaking it up in the most optimal way, or that your submethods are poorly named. In theory, there are instances where a long method is better. In practice, it's extremely rare. If you can't figure out how to make a shorter method readable, get someone to review your code, and ask them specifically for ideas on shortening the methods.

As for multiple loops causing a supposed performance hit, there's no way to know that without measuring. Multiple smaller loops can be significantly faster if it means everything it needs can stay in cache. Even if there is a performance hit, it's usually negligible in favor of readability.

I will say that often long methods are easier to write, even though they're more difficult to read. That's why they proliferate even though no one likes them. There's nothing wrong with planning from the start to refactor before you check it in.

share|improve this answer
1  
"There's nothing wrong with planning from the start to refactor before you check it in." +1 for that. Most IDEs nowadays have refactoring tools that make this extremely easy as well. But there is a reverse method where you delegate things to non-existent functions, and later go and fill in the methods, but I've never been able to code that way, as much as I've tried. –  Tjaart Oct 16 '12 at 12:02
show 1 more comment

Long methods aren't always bad. They usually are a sign that there might be a problem.

On the system I'm working on we have a half a dozen or so methods that are more than 10,000 lines long. One is currently 54,830 lines long. And it's OK.

These ridiculously long functions are very simple and are autogenerated. That big 54,830 line long monster contains the daily polar motion data from January 1 1962 to January 10, 2012 (our last release). We also release a procedure by which our users can update that autogenerated file. That source file contains the polar motion data from http://data.iers.org/products/214/14443/orig/eopc04_08_IAU2000.62-now , auto-translated to C++.

Reading that web site on the fly is not possible in a secure installation. There is no connection to the outside world. Downloading the web site as a local copy and parsing in C++ isn't an option either; parsing is slow, and this has to be fast. Downloading, auto-translating to C++, and compiling: Now you have something that is fast. (Just don't compile it optimized. It's amazing how long an optimizing compiler takes to compile 50,000 lines of extremely simple straight line code. It takes over half an hour on my computer to compile that one file optimized. And the optimization accomplishes absolutely nothing. There's nothing to optimize. It's simple straight line code, one assignment statement after another.)

share|improve this answer
4  
"one assignment statement after another" ... I'd call that a "data file". Why is it code? –  Joachim Sauer Oct 15 '12 at 13:51
2  
@JoachimSauer - Because parsing a large data file at run time in a Monte Carlo setup is a bad idea. A very, very bad idea. –  David Hammen Oct 15 '12 at 14:04
3  
@DavidHammen: then do it at initialization time, just like you're forcing your linker/loader to do. Or write the data file as a C structure in a header file, instead of C code. At least then the loader would load as a data block. –  Javier Oct 15 '12 at 14:21
2  
@Javier: Even at initialization time it can be a very bad idea, at least in a Monte Carlo setup. A simulation that takes minutes to initialize, but only seconds to run goes against the grain of getting tens of thousands of runs in overnight. Changing key initialization time tasks to compile time tasks fixes that problem. We've tried a number of techniques, including the compiled data structure approach. It just doesn't work or would be very hard to make work in some cases (e.g., a huge gravity model). The straight line code approach is easy to autogenerate, easy to verify. It's just ugly code. –  David Hammen Oct 15 '12 at 15:44
5  
+1 interesting story. generated code isn't really source code of course so one could argue that this doesn't 'break' the rule. one assumes the code generator itself had nice short methods –  jk. Oct 16 '12 at 9:07
show 1 more comment

Long methods can be more computationally and space efficient, it can be easier to see the logic and easier to debug them. However these rules only really apply when only one programmer touches that code. The code is going to be a pain to extend if it is not atomic, essentially the next person will have to start from scratch and then debugging and testing this will take for ever as it is not using any known good code.

share|improve this answer
30  
There are always at least two programmers involved: "you" and "you, three weeks from now". –  Joachim Sauer Oct 15 '12 at 12:23
add comment

There is something what we call Functional Decomposition implying to break up your longer methods into smaller ones wherever possible. As you mentioned that your method involves sorting / filtering then you better have separate methods or functions for these tasks.

Precisely, your method should be focussed on perforimng 1 task only.

And if it needs to call another method dor some reason then do so otherwise carry on with the one you are already writing. Also for readability pruposes, you can add comments. Conventionally, programmers use multi-line comments (/**/ in C, C++, C# and Java) for method descriptions and use single-line comments (// in C, C++, C# and Java). There are also good documentation tools available as well for greater code readability (eg. JavaDoc). You can also look into XML based comments if you are a .Net developer.

Loops do impact program performance and may cause application overhead if not used properly. The idea is to design your algorithm in such a that you use nested loops as little as possible.

share|improve this answer
show 1 more comment

It's perfectly alright to write lengthy functions. But it varies on the context whether you really need or not. For e.g. some of the best algorthms are expressed best when it a peice. On the other hand, a large percentage of routines in object-oriented programs will be accessor routines, which will be very short. Some o of the lengthy processing routines which has lengthy switch-cases,if conditions can be optimized via table driven methods.

There's an excellent short discussion in Code Complete 2 about the length of the routines.

The theoretical best 497 maximum length is often described as one or two pages of program listing, 66 to 132 lines. Modern programs tend to have volumes of extremely short routines mixed in with a few longer routines.

Decades of evidence say that routines of such length are no more error prone than shorter routines. Let issues such as depth of nesting, number of variables, and other complexity-related considerations dictate 535 the length of the routine rather than imposing a length

If you want to write routines longer than about 200 lines, be careful. None of the studies that reported decreased cost, decreased error rates, or both with larger routines distinguished among sizes larger than 200 lines, and you’re bound to run into an upper limit of understandability as you pass 200 lines of code. 536 restriction per se.

share|improve this answer
add comment

Another vote that it's almost always wrong. I find two basic cases where it's the proper answer, though:

1) A method that basically just calls a bunch of other methods and does no real work itself. You have a process that takes 50 steps to accomplish, you get a method with 50 calls in it. There's usually nothing to be gained by trying to break that up.

2) Dispatchers. OOP design has gotten rid of most such methods but incoming sources of data are by their very nature just data and thus can't follow OOP principles. It's not exactly unusual to have some sort of dispatcher routine in the code that handles the data.

I would also say that one should not even consider the question when dealing with autogenerated stuff. Nobody's trying to understand what autogenerated code does, it matters not one iota whether it's easy for a human to understand.

share|improve this answer
1  
The 50 step process can probably be summarized into several buckets through. Step 1 - 9 are parameter checks so create a new method called parameter checking. (I am sure there are some examples where this is not possible. I would be interested in seeing one). –  sixtyfootersdude Oct 17 '12 at 16:17
show 1 more comment

I wanted to address the example you gave:

For example I have some Django views that do a bit of processing of the objects before sending them to the view, a long method being 350 lines of code. I have my code written so that it deals with the paramaters - sorting / filtering the queryset, then bit by bit does some processing on the objects my query has returned.

At my company our biggest project is built on Django and we have long view functions too (many are more than 350 lines). I'd argue ours don't need to be that long, and they're hurting us.

These view functions are doing a lot of loosely related work that should be extracted to the model, helper classes, or helper functions. Also, we end up reusing views to do different things, which should instead be divided into more cohesive views.

I suspect your views have similar characteristics. In my case, I know it causes problems and I'm working to make changes. If you don't agree that it's causing problems, then you don't need to fix it.

share|improve this answer
add comment

I don't know if someone has already mentioned this, but one of the reasons long methods are bad is because they usually involve several different levels of abstraction. You have loop variables and all kinds of things happening. Consider the fictitious function:

function nextSlide() {
  var content = getNextSlideContent();
  hideCurrentSlide();
  var newSlide = createSlide(content);
  setNextSlide(newSlide);
  showNextSlide();
}

If you were doing all the animation, calculation, data access etc in that function it would have been a mess. The nextSlide() function keeps a consistent abstraction layer (the slide state system) and ignores others. This makes code readable.

If you have to constantly step into smaller methods to see what they do then the exercise of dividing up the function has failed. Just because the code you are reading is not doing obvious things in child methods doesn't mean child methods are a bad idea, just that it was done incorrectly.

When I create methods I usually end up dividing them up into smaller methods as a sort of divide and conquer strategy. A method like

   if (hasMoreRecords()) { ... }

is certainly more readable than

if (file.isOpen() && i < recordLimit && currentRecord != null) { ... } 

Right?

I agree about absolute statements are bad and also agree that usually a long method is bad.

share|improve this answer
add comment

True story. I once encountered a method that was over two thousand lines long. The method had regions that described what it was doing in those regions. After reading through a region, I decided to do an automated extract method, naming it according to the region name. by the time I was done, the method was nothing but 40 method calls of about fifty lines each and it all worked the same.

What is too large is subjective. Sometimes, a method can't be broken down any further than what it is currently. It's like writing a book. Most people agree that long paragraphs should usually be split. But sometimes, there is only one idea and splitting it causes more confusion than would be caused by the length of that paragraph.

share|improve this answer
add comment

The point of a method is to help reduce code regurgitation. A method should have a specific function that it is responsible for. If you end up rehashing code in numerous places you run the risk of code having unexpected results if specifications the software is designed to address are changed.

For a method to have 350 lines would suggest that a lot of the tasks that it is carrying out are replicated elsewhere as it is unusual to require such a large amount of code to carry out a specialised task.

share|improve this answer
show 2 more comments

It's not really the long methods that are bad practice it's more leaving them like that that is bad.

I mean that the actual act of refactoring your sample from:

varaible_1 = 0
variable_2 = 0
for object in queryset :
     if object.condition_condition_a and variable_2 > 0 :
     variable 1+= 1
     .....
     ...    
     . 
      more conditions to alter the variables

return queryset, and context 

to

Status status = new Status();
status.variable1 = 0;
status.variable2 = 0;
for object in queryset :
     if object.condition_condition_a and status.variable2 > 0 :
     status.variable1 += 1
     .....
     ...    
     . 
      more conditions to alter the variables (status)

return queryset, and context 

and then to

class Status {
    variable1 = 0;
    variable2 = 0;

    void update(object) {
        if object.condition_condition_a and variable2 > 0 {
            variable1 += 1
        }
    }
};

Status status = new Status();
for object in queryset :
     status.update(object);
     .....
     ...    
     . 
      more conditions to alter the variables (status)

return queryset, and context 

you are now well on the way to not just a much shorter method but a much more useable and understandable one.

share|improve this answer
add comment

I think the fact that the method is very long is something to check on, but definitely not an instant anti-pattern. The big thing to look for in huge methods is lots of nesting. If you have

foreach(var x in y)
{
    //do ALL the things
    //....
}

and the body of the loop isn't extremely localized(ie, you could send it less than 4 parameters), then it's probably better to convert it to:

foreach(var x in y)
{
    DoAllTheThings(x);
}
...
void DoAllTheThings(object x)
{
    //do ALL the things
    //....
}

In turn, this can vastly reduce the length of a function. Also, make sure to look for duplicate code in the function and move that into a separate function

Finally, some methods are just long and complicated and there is nothing you can do. Some problems need solutions that do not lend themselves to easy to code. For instance, parsing against a grammar of much complexity can make really long methods that you really can't do much about without making it worse.

share|improve this answer
add comment

Truth is, it depends. As mentioned, if the code does not separate concerns and tries to do everything in one method then its a problem. Separating code into multiple modules makes it easier to read code, as well as write code(by multiple programmers). Adhering to one module(class) per source file is a good idea to begin with.

Secondly, when it comes to functions/procedures:

void setDataValueAndCheckForRange(Data *data) {/*code*/} 

is a good method if it checks for the range of only "Data". It is a BAD method when the same range applies for multiple functions(example of bad code):

void setDataValueAndCheckForRange(Data *data){ /*code */}
void addDataValuesAndCheckForRange(Data *result, Data *d1, Data *d2){ /*code*/}
void subDataValuesAndCheckForRange(Data *result, Data *d1, Data *d2){ /*code*/}
void mulDataValuesAndCheckForRange(Data *result, Data *d1, Data *d2){ /*code*/}

This has to be refactored to:

bool isWithinRange(Data *d){ /*code*/ }
void setDataValue(Data *d) {/*code*/ if(isWithinRange(d)){/*continue*/}else{/*warn/abort*/} 
void addDataValue(Data *d, Data *d1, Data *d2) {/*code*/ if(isWithinRange(d)){/*continue*/}else{/*warn/abort*/} 
void subDataValue(Data *d, Data *d1, Data *d2) {/*code*/ if(isWithinRange(d)){/*continue*/}else{/*warn/abort*/} 
void mulDataValue(Data *d, Data *d1, Data *d2) {/*code*/ if(isWithinRange(d)){/*continue*/}else{/*warn/abort*/} 

REUSE code as much as possible. And that's possible when each function of your program is SIMPLE(not necessarily easy) enough.

QUOTE: The MOUNTAIN is composed of tiny grains of earth. The ocean is made-up of tiny drops of water.. (- Sivananda)

share|improve this answer
add comment

Long methods tend towards "bad" in imperative languages which favor statements, side-effects, and mutability, precisely because those features increase complexity and thus bugs.

In functional programming languages, which favor expressions, purity, and immutability, there is less reason for concern.

In functional and imperative languages alike, it's always best to factor out reusable chunks of code into common top-level routines, but in functional languages which support lexical scoping with nested functions etc., it's actually better encapsulation to hide sub-routines within the top-level function (method) than to break them out into other top-level functions.

share|improve this answer
add comment

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.