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.

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.

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

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!

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.)

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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)

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.