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.

Imagine a long and complicated process, which is started by calling function foo(). There are several consecutive steps in this process, each of them depending on result of the previous step. The function itself is, say, about 250 lines. It is highly unlikely that these steps would be useful on their own, not as a part of this whole process.

Languages such as Javascript allow creating inner functions inside a parent function, which are inaccessible to outer functions (unless they are passed as a parameter).

A colleague of mine suggest that we can split up the content of foo() into 5 inner functions, each function being a step. These functions are inaccessible to outer functions (signifying their uselessness outside of the parent function). The main body of foo() simply calls these inner function one by one:

foo(stuff) {
    var barred = bar(stuff);
    var bazzed = baz(barred);
    var confabulated = confabulate(bazzed);
    var reticulated = reticulate(confabulated);
    var spliced = splice(reticulated);

    return spliced;

    // function definitions follow ...
}

Is this an anti-pattern of some sort? Or is this a reasonable way to split up long functions? Another question is: is this acceptable when using OOP paradigm in Javascript? Is it OK to split an object's method this way, or does this warrant another object, which contains all these inner functions as private methods?

See also:

share|improve this question

marked as duplicate by gnat, MichaelT, Telastyn, GlenH7, Ampt Aug 12 at 14:06

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

4  
How is this different from the earlier question? Just by being in JavaScript with its peculiar object model? –  Kilian Foth Aug 11 at 9:06
1  
@KilianFoth The previous question was motivated by perceived "callability" of the smaller methods into which the original method was split up. When using inner functions, the programmer seems to be able to indicate a more narrow scope for a function. I wanted to see whether there are significant drawbacks to this. In particular, janos notes that these functions may be hard to test. –  exizt Aug 11 at 9:15
1  
Your specific example has hints of an anti-pattern called sequential coupling, but it may be the correct way to go given the specific algorithm involved - or there could be a better way to split your code into different functions that reads better. –  Izkata Aug 11 at 13:55
    
The stacktrace let you faster find the Problem. –  Peter Rader Aug 11 at 16:17
    
Break them up. But do it by re-ordering lines of code to minimize the scope of variables; then extracting into a function name that serves as a comment. If you have a giant function, then diffs and merges get disturbed for no good reason; such as pointless white-space changes due to re-indentation. Functions can have clear preconditions, postconditions, mutability and functional guarantees, and a clear name. When I first started programming, I also used to believe what the poster suggests; but big functions are very good at hiding long lived bugs. –  Rob Aug 11 at 19:26

8 Answers 8

Inner functions are not an anti-pattern, they are a feature.

If it doesn't make sense to move the inner functions outside, then by all means, don't.

On the other hand, it would be a good idea to move them outside so you can unit test them easier. (I don't know if any framework lets you test inner functions.) When you have a function with 250+ lines, and you make any changes in the logic, are you sure you're not breaking anything? You really need unit tests there, and it won't be feasible with one giant function.

Splitting up long functions to smaller ones is a good idea in general. It's a common refactoring technique to replace a comment with a function named after the comment, called "extract method". So yes, do it!

As @Kilian pointed out, see also this related post:

Should I place functions that are only used in one other function, within that function?

share|improve this answer
2  
See also for where to put the smaller functions. –  Kilian Foth Aug 11 at 9:19
2  
The big problem with inner function is scope. You cannot Unit-Test them individually, you can not easily reuse them and you have no overview what side-effects they have. Inner functions are like GOTO-Labels, and name shadowing can become horrible... –  Falco Aug 11 at 13:38
2  
@Falco: Inner functions are not goto's, they are functions, just like any other function. They are gosubs, if you like. Side-effects are part and parcel of OOP; if you don't like them, the answer is to write in functional style; merely making your methods public won't get it done. Inner functions are encapsulated, so naming problems should be less difficult, not more. –  Robert Harvey Aug 11 at 15:42
1  
@Janos: Visual Studio's Unit Testing Framework can test inner functions. It would surprise me if it's the only testing framework that can. –  Robert Harvey Aug 11 at 15:44
3  
@user1068: If this is the sort of application where function calls negatively impact performance, something else is wrong. Also, it hurts readability in no way; you know from good naming whether or not you need to look into a function. Your way, taken to extremes, results in having no functions , which is generally considered to be a bad idea. –  Magus Aug 11 at 18:43

It's not an anti-pattern. It's the right way to do it.

It's easier to read, to maintain, to test. It helps avoid duplication.

See : clean code (the bible for devs) and this answer

share|improve this answer
5  
This is part of the Single Responsibility Principle. Each method should only have a single responsibility - in this case, breaking a long method up into smaller methods is exactly the right thing to do. –  Stephen Orr Aug 11 at 8:41
13  
this reads more like an opinion, lacking an explanation. Links don't help either, first one is to the book at Amazon (it's large, do you suggest one to read it all to answer specific question?) and link labeled "answer" actually leads to a question. Consider editing into better shape. See How to Answer –  gnat Aug 11 at 8:58
3  
I would say that breaking a large method up into various small methods and then invoking all of those methods inside of the large method - and assuming the small methods are private - is not adhering to the SRP because the large method is still performing multiple responsibilities. If you refactored the small methods out into dependencies then you could argue that the method is then being a facade, but if the small methods are encapsulated inside the large one, you are really still violating SRP –  Dan Pantry Aug 11 at 10:48
3  
If you think Clean Code is "the bible for devs", you should read Code Complete. –  Mike Partridge Aug 11 at 12:56
1  
IMHO you missed the point of the OP - he is not asking if a big function should be split up into smaller functions, he is asking if this should be done by using inner functions. –  Doc Brown Aug 11 at 14:40

The most important issue is to manage complexity. Inner methods are one of the solutions, but I'm not convinced that a best one.

You want to split the method because it's long, but with inner methods the method is still long, just divided into "chapters". It's better than "comment chapters", though.

A better solution is to use an inner class that gathers all of the helper methods and - what is also important - data. Additionally, this class can be unit tested separately.

This is a variation of Fowler's "Replace Method with Method Object" refactoring.

share|improve this answer
    
"Replace Method with Method Object" doesn't make sense in Javascript, since methods are functions and functions are objects. Also, there are no classes; although some people approximate them using functions. Your suggestion sounds like it would introduce a bunch of boilerplate for no gain. –  Warbo Aug 12 at 15:59
    
@Warbo I disagree. Classes are easily and routinely implemented in Javascript. Properly declared, a method object would provide better encapsulation and testability. –  exizt Aug 13 at 7:16
1  
@Warbo - The question was about OOP paradigm in JavaScript. It's true that in JS "everything" is an object and it's rather prototype based language than a classical OO one. But it shouldn't stop anyone from using proven OO techniques. Proven patters are easy to maintain. –  andrew.fox Aug 13 at 10:10

It's not an anti-pattern, but it's a questionable move. If those functions are not valuable outside their parent function, there is no benefit of separating them, just more code. You could just put some comments in code saying "now we confabulate previously buzzed thingy", and it would have same value in less lines of code.

EDIT: I'd rephrase my answer, because I've used not very careful wording there, and, apparently, community didn't accept it, based on downvotes. What I really wanted to say is that one should take into account the amount of extra code and attention breaks while reading. When the original function is small enough (how small is opinion-based), it could be easier to read it as one stream of text with some helping comments, than jumping back and forth between inner functions. It could easily be that one chunk of code is easier to read and understand than a collection of several functions.

On the other hand, when the original function is big enough to understand as one piece, it totally makes sense to split it. Principle of lesser scope definitely comes into play here. It's just important to understand when the function is "big enough" for that. It's easy to make such judgement when the original one is 10k LOC, as was commented, but what about the OP's example, 250 LOC? I'd say it's becoming opinion-based to give as an answer.

share|improve this answer
5  
Disagree. That's like saying "oh don't split up this 10k LOC function into smaller ones because you only call each one once". And about whether inner functions are better than external ones, you need some really good arguments for why "smaller scope is better" shouldn't hold here. –  Voo Aug 11 at 9:48
1  
The benefit is to make code easier to read and more maintainable. following logic through a large function is more difficult than tracing through several small functions. –  Jaydee Aug 11 at 10:00
2  
@Jaydee, I'd argue that. Following one stream of text is easier than jumping around several functions back and forth, trying to understand their meaning and relations. Of course, it's again the matter of the actual size. If the function text small enough (say, fits your display height, but that's opinion based again), it's easier to just read it as one piece. –  Haspemulator Aug 11 at 10:03
2  
"jumping around several functions back and forth, trying to understand their meaning and relations"-in that case the problem is how you split up the functions, the naming and possibly documentation. You should be able to understand each function on its own, considering the other ones as blackboxes with documented behavior. Having to understand 3 small functions that do a single thing, is just plain easier than understanding one giant function even if it fits on one screen (hey I have a 30" monitor, so that's actually really long). Cost? Overhead of 2 line per function declaration - negligible. –  Voo Aug 11 at 10:26
16  
Reading a series of meaningful function names {loadData(); doCalculation(); printData();} is far more readable than figuring out what each section of code does each time you read it. Most compilers will inline the functions during optimization so there is no performance loss. –  Jaydee Aug 11 at 10:53

It is certainly better to keep each "inner function" separate: small, combinable parts are much easier to maintain than monolithic "big balls of mud". Be careful with your terminology though: functions and methods aren't the same thing, from a stylistic point of view.

Methods tend to favour one object ("this") above others, which limits their usefulness to that object. Functions are independent of any particular value, which makes them more flexible and easier to combine together.

In this case, your "foo" function is clearly the composition of those "internal functions", so you might as well define it that way and avoid all of the intermediate boilerplate ("stuff", "barred", "bazzed", "confabulated", "reticulated", "spliced", "function(stuff) {" and "return ...; }"):

var foo = [splice, reticulate, confabulate, baz, bar].reduce(compose, id);

If you haven't already got "compose" and "id" functions, here they are:

var compose = function(f, g) { return function(x) { return f(g(x)); }; };
var id = function(x) { return x; };

Congratulations, you've just gained two incredibly useful, reusable parts out of the supposedly "useless" internals of foo. I can't do much more without seeing the definitions of the "internal" functions, but I'm sure there's more simplification, generalisation and reusability available waiting to be discovered.

share|improve this answer
    
+1 for a radically different approach. –  exizt Aug 13 at 7:17
    
I wouldn't say it's "radically" different, it's just Don't Repeat Yourself applied to foo's repetitive "call this with the result of that" pattern. –  Warbo Aug 13 at 8:56

I think this type of code can be easily be converted to a chaining. Like this :

foo(stuff)
{
  return    
    stuff
      .bar()
      .baz()
      .confabulate()
      .reticulate()
      .splice()        
}

From my humble point of view, it's easy to read, keeps the separation of concern and limit the error rate (if one of the step fails, all the process fail).

share|improve this answer
4  
For this to work, your chained methods have to be a part of the public API of the stuff object; they can't be inner functions. –  Robert Harvey Aug 11 at 15:49

It is not an anti-pattern to split a big function into smaller functions.

However declaring all these functions inside the main one presents a certain risk: because the inner functions have access to all the variables of the outer function, and because in JavaScript variables are mutable, you cannot guarantee that the inner functions won't modify the outer function's state.

In this sense, by nesting them you lose most of the benefits of having small functions. Since they all share a common mutable state, you cannot easily reason about each one independently.

So, keep a flat structure. If you want to hide the private functions, you can use the module pattern or one of the many available JavaScript module libraries.

share|improve this answer

Yes, splitting is good, may it only be for better readabilty and testability.

Another approach might be to call the functions within each other, you start with:

foo(stuff) {
   var spliced = splice(stuff);
   return spliced;
}

Since the spliced function needs reticulated data, you can call that function first:

splice(stuff) {
    var reticulated = reticulate(stuff);
    //do splicing of reticulated
    return spliced;
}

So all functions would pass stuff through and work with the results they receive.

share|improve this answer

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