Refactoring Large Functions | Hacker News Reader

Refactoring Large Functions

robert.muth.org

72 pointsmuth0244638 comments

Do people really prefer:

      function doFirstBit() {}
      function doSecondBit() {}

      function big() {
          let x,y,z;
          [x,y] = doFirstBit();
          [x,z] = doSecondBit(x, y);
      }

to:

      function big() {
          let x,y,z;
          // First bit...
          ...
          // Second bit...
          ...
      }

To me, it's no harder to find the portion of interest in the latter when making a local change, and substantially easier to understand the whole thing (because you can sit down, maybe with a printout if that's your thing, and read through it linearly without having to jump back and forth to the subfunctions). And passing data between the portions can quickly end up nastier than my contrived example above, at which point people start doing things like creating a class just to hold what used to be the function's stack-frame.

Not saying very long functions are something to celebrate. But if they're organised sensibly they're not a huge problem, and to be honest probably bother me less than functions which aren't abstracting anything terribly interesting and only get called once.

Yes, people really prefer. You can get rid of the silly comments, for one thing, and express the idea by naming the functions properly.

With a printout? Well, if you allow functions to be so long that you have to print them out, then you probably a lot of other problems with your code base.

I think that if "First bit," "Second bit," etc can be called out that explicitly, I don't mind big functions that much.

The trouble comes from functions where the various bits are all mixed together. This means that comments cannot easily name the sections (for me, one of the main advantages of a function is that it must be given a name), and you cannot easily narrow your reading to just the part you care about.

Another advantage of smaller functions is that some of them end up being general enough functionality to be worth sharing with other code.

Finally, it's possible to test doFirstBit() independently of the other logic in big().

On the contrary, I have something like "a named section of a function should be refactored into a private helper" as a macros when doing code review. This explicitly to prevent the eventual evolution into a god function.

To make an analogy, the first code snippet is like a book with a table of contents and then a bunch of chapters, and the second is like the same book with no table of contents. Not a huge difference at least from that perspective, but the first one can be more clear at a glance, since it at least tries to describe the high-level steps without getting into the weeds. Your editor should allow you to easily jump in and out of each "chapter"/function to see its contents.

However, I prefer to put `doFirstBit` and `doSecondBit` below `big`, since that way the file is laid out in the order that the code is typically executed. That makes the two code snippets even more similar.

Yes, I'd greatly prefer the "many smaller functions" approach.

* Each function has well-defined inputs and outputs, no weird dependencies between parts.

* Parts can be reused when needed, and tested independently.

* Separating functions makes you think about good names, and good sets of inputs and outputs. This alone uncovers a few classes of bugs and mental mistakes.

> Parts can be reused when needed, and tested independently.

You honestly think having someone adding a sub-function as a dependency and adding more tests is 'good'

Have you seen how npm works?

You mean the npm where a hostile maintainer can insert malign code into your project?

Yes.

Yes totally agree and I want to add that it enables us to add docstring or similar where we document the function and expectations of inputs, which can be very helpful.

As an extreme case, one time I made a function with a single line that just delegated to a builtin function. But it had 5 lines of docstring explaining what it was doing in terms of business requirements and 5 lines of comments explaining how the builtin function accomplished that.

Before a few changes like this, the mixture of abstraction levels (what I was doing vs how I was doing it) was a big impediment for me. I would stare at the code for hours trying to figure out if it was correct or not and only being able to hold all the complexity in my head at the very peaks of concentration.

But yeah that was an unusual case.

The readability/understandability points other make are good.

But I'd add that large function like this tempt the programmer to do something like using one of the intermediate variables in the first "bit" to make a decision in the second "bit" out of convenience. Add ten more "bits" and ten more such "conveniences" and you get an incredibly fragile, bug-prone mess.

Which is to say, a purely lexical-order division of operations is bad because you easily wind-up with undefined defined interface between the various operations. Figuring out both how a simple for-loop really works as well as how the original programmer intended it to work, can be quite hard. Add several for-loops and you get a true mess.

Some people get really good at cranking out these long functions and as you say, it look superficially easier. I wonder if a language could support "functional abstraction brackets" that would allow these long sections but guarantee their logic isn't folded together

Something like

      function big() {
          let x,y,z;
          {{@First_bit //begin forced separation...
          ...
          {{@Second_bit //begin forced
          ...
      }

Here "First_bit" would be guaranteed to be equivalent to functions of just x, y, z.

If I'm understanding you right, "C-like-syntax" languages generally allow this:

       function big() {
           let x,y,z;
           {
              // First bit
              let localToThisSection = 42;
              ...
           }
           {
              // Second bit
              ...
           }
       }

Yes, If your x,y, and z are both inputs and outputs, that works.

If you wanted a syntax that exactly mirrored what happens in the gp's second example, you'd need more work. Plus if you force a begin-bracket to have a label, you avoid the editor having to use "fun3756" when automatically abstracting out the subroutine.

JavaScript's IIFEs can act like that:

    function big(x, y, z) {
        const firstBitResult = ((x, y, z) => {
          ...
        })(x, y, z)

        const secondBitResult = ((x, y, z) => {
          ...
        })(x, y, z)

        ...
    }

I've used this trick when hacking out on quick-hack-turns-into-humongous-mess type projects.

I don't like either. My decision about refactoring has to do with the mixing of different levels perspectives. So the different perspectives are "Conceptual, Specification, and Implementation". Most long functions are because those perspectives are getting jumbled together. I'd argue this approach to looking at your code is much more mentally clearer than the authors advice. With that in mind you can easily refactor things using any number of techniques to separate out each perspective fragment. The book "Working effectively with legacy code" has lot of techniques that are useful (some of which I contributed!). This article is like a highly abridged form of that book ( which is referenced at the end of the article).

The second example is exactly what your describe.

A high level function forms the conceptual idea, it contains many functions that are specify that concept, and within each are implementations.

If each function does one thing, it composes other functions, you're forced to follow this type of design.

I prefer third option: define two small functions inside the large one. That way, the "root" level of the large function is still small, but you can clearly see the scope in which the two smaller ones are used.

It only works if it's possible to understand what doFirstBit() does without understanding how it does it. E.g. compare "reorderArrayBasedOnSomeDomainSpecificConstraints(&array)" vs. a screenful of code that reorders the array in-place (especially if it's plain C and the temporary variables used in that part are mixed with the rest of the function's variables).

Understanding the entirety of a long function often takes quite a bit of time. Sometimes you just want to know what a section of code does, not how it does it. If I'm touching big() and doFirstBit() for a code change, it'll be a hell of a lot faster if I don't have to read through doSecondBit(), doThirdBit() etc.

Yes, it's much better code that solves all kinds of hidden problems in the second style. The second style seems ok to you, then you've not yet learned the benefits of abstraction nor of bottom up programming. It's easier to build simpler programs when you build them from many simple parts. It's easier to build parts if they do one thing. It's easier to test everything if they're all separate things. It's easier to reuse logic when they're separate things. It's easier to fix bugs when they're separate things. It's easier to separate implementation and interface, i.e. low level vs high level logic. It's easier to plug in alternate implementations when they're each separate things. That all adds up to being much easier to write, evolve, and grow a system using the first approach than with the second.

Abstractions are great. But in my experience the kind of functions you get by pulling chunks out of a big (but working) function aren't often abstracting things that are terribly valuable. If you think the extracted function does make sense on its own, then of course go for it! But if in doubt, I'd prefer to wait until there's actually a second call site to make the motivation for the extracted function clearer (and stand a better chance of getting its interface right first time).

Function abstraction is not only about reuse, it's about separating the high level details from the low level details. Separating the flow of logic, from the details of each step, is always valuable and should be done right from the start; replacing an inline implementation with a function name with args is abstraction even when it's not ever reused and it greatly improves the code.

Big methods that do many things inline with commented sections explaining what the section does, is bad programming. It allows variable reuse and hidden weird dependencies between what should be independent chunks of code in independent functions. It also forces the programmer reading it to always work at the lowest level of implementation rather than a higher level of abstraction where he can see the flow without needing to see all the details of each step. If you can point to a section of code, and put a comment on it saying it does X, then it belongs in a function named X and the comment needs deleted. Comments that explain what code does are a code smell. Comments are for explaining why, not what.

There shouldn't be any big functions to begin with except in a few rare cases like switching on a character or something.

No, that’s not better. What’s better is if you can change this:

    function big(a,b,c)

Into this:

    nameableIntermediateState = smaller(a, b)
    finishUp(c, nameableIntermediateState

This is better because there were things you were doing with A and B that had nothing to do with C.

That’s the ideal final state, where your big function doesn’t even know about C. However, refactoring often has to happen in phases, and so a pass through like the kind you wrote is sometimes a good intermediate step.

It’s about teasing apart names into smaller islands that the names never escape. If you are passing names between islands, there’s no value.

John Carmack on this topic:

http://number-none.com/blow/john_carmack_on_inlined_code.htm...

And I easily agree 100% with him having tried both approaches in bigger projects.

There's a bit of a fuzzy edge in there, where he favors "inline, don't break up artificially" but also "more pure functional code". Breaking pure logic out of a large, otherwise mixed-purity func gives you those stronger purity guarantees / separations, which seems to favor more funcs.

But he's also huge on "don't fight the compiler" and static analysis, all of which are aided by strictly pure funcs, so it seems in-character.

That's because he comes from a domain where mutable state is a necessary thing in making the different parts of game logic feed into each other. It is the focus of debugging.

The actual computations around what changed can be pure, but turning them into a concurrently operating loop is done most effectively with a static sequencing(and hence, straightline imperative).

Or in other terms, he is using a different strategy for different parts of the codebase. At the top level it's imperative, but when you drill down into the callstack, it isn't.

A lot of folks work on request-response systems all day, and in those, the necessary state changes tend to be once-per-request and wrapped in transaction or session logic. So it's a lot less crucial to have this kind of strategy for debugging purposes.

The anecdote Carmack shares from aviation is interesting:

  >Indeed, if memory serves (it's been a while since I read about this)...
  >
  >The fly-by-wire flight software for the Saab Gripen (a lightweight
  >fighter) went a step further.  It disallowed both subroutine calls and
  >backward branches, except for the one at the bottom of the main loop.
  >Control flow went forward only.  Sometimes one piece of code had to leave
  >a note for a later piece telling it what to do, but this worked out well
  >for testing:  all data was allocated statically, and monitoring those
  >variables gave a clear picture of most everything the software was doing.
  >The software did only the bare essentials, and of course, they were
  >serious about thorough ground testing.
  >
  >No bug has ever been found in the "released for flight" versions of that
  >code.
  >
  >                                                          Henry Spencer

Henry Spencer is one of the internet's greatest treasures.

Some things are better said in one chunk, there are no rules. I often write longer functions while figuring things out, when I need more context in front of me to stay in flow.

Then I wait, and wait; until I can't stand it any more, at which point I usually know enough to substantially improve the code.

Assuming it survived that long. Refactoring code that you're going to throw away is not very constructive. And the more effort put into making code pretty, the harder it will be to let go when that's the right thing to do.

None of this works in a corporate setting, where there's never enough time or money to do anything properly. There are no awesome short term profits to be made from prototyping and keeping code in good health. Corporations also usually prefer rigid rules over competence and intuition, which means whatever cure they come up with will be worse than the disease. And as a result, corporate code is usually crap code.

It is important to remember that functions exist for the sake of humans, not computers. The computer wouldn't care if all the code was in a giant main() function with GOTOs and a manually-managed stack, the programmers are the ones who have a problem with this.

Item number 0 MUST be :

Make a copy of current function as old_function and add unit test to check if input on refactored function still produces same results as old_function.

That's why you have version control and batches and fuzzing etc.

Not sure how version control can help here, as unit tests are not run on old versions.

About batches, could you be more specific, i might be missing something here.

And for fuzzing, it will test your new function, but will not provide any guarantees that your new functions is working exactly like the old one. You might be surprised, bet there could be bugs in old function which can be fixed by refactoring. Sounds nice? But might be some legacy codes is depending on the bug that you fixed, and for backwards compatibility reasons that bug must stay be renamed from "bug" to "functionality".

I meant "branches". Mobile devices speak for us, it's an unbearable situation.

I think the point is to have a unit test like

  for parameters in parameter_generator(n=100):
    assert(old_version(*parameters) == new_version(*parameters),
           f"Versions produced different outputs on {parameters}")

This lets you transform old_version with more confidence that you're not corrupting its logic.

The article mentions Steve McConnel's advice on parameter lists (from his great book "Code Complete: A Practical Handbook of Software Construction"), but left out some interesting commentary on function length from the same book:

Some studies of code quality from the 1980s reported that defects per KLOC was inversely correlated with function length, leveling off around 200–400 LOC per function. Software composed of many tiny functions is more difficult to understand because there is more context "off screen" to keep in your head.

A Cisco study of code reviews found that 200–400 LOC is the limit of how many LOC can be effectively reviewed per hour. Applying the findings of these studies suggests that neither functions nor patches/pull-request diffs should not exceed 200 LOC. FWIW, I have worked on commercial software that had functions many thousands of lines long! :)

I've seen my share of code bases and many times I've seen a long function, I've made a number of other observations:

The long function has a lot of comments that try to describe the different steps. The comments are often confusing because whoever wrote the comments couldn't quite understand themselves what is happening.

There is a significant frequency of bugs found in the functionality implemented or coordinated by that function.

The long function has poor code coverage.

For me, a long function is usually a code smell.

Yeah often the comments are the names of the inner functions that should be extracted out too.

Reply on news.ycombinator.com