29

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:

Community
  • 1
Stas Bichenko
  • 3,731

8 Answers8

51

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?

Community
  • 1
janos
  • 1,829
15

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

Community
  • 1
Maxime ARNSTAMM
  • 307
5

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.

andrew.fox
  • 543
3

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.

Haspemulator
  • 456
2

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.

Warbo
  • 1,225
  • 8
  • 12
1

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

eka808
  • 204
  • 1
  • 5
1

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.

lortabac
  • 1,442
0

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.

gnat
  • 20,543
  • 29
  • 115
  • 306
user145856
  • 1