HN Explorer

Ask HN: How do you deal with 2000 line functions?

26 points by pinkunicorn ↗ HN
I'm working with an archaic code base and I'm stuck at trying to add new business logic into a 2000 line function that is absolutely crappily written with nothing but if() conditions in it. Given such a scenario, how do I go about adding a new functionality to this mess? Should I tell my manager that the function needs to be refactored and take time to refactor it?

Edit: Removed offensive word

71 comments

[ 4.0 ms ] story [ 150 ms ] thread
PaulHoule
Yes, just don't use the word "shit".

The obvious refactoring is to split the function up into smaller pieces, even if you can split it into two 1500 line functions that is a win.

pinkunicorn
Thanks, that came out of frustration from looking at similar long-sized functions for an extended period of time.
Nadya
Er... am I misunderstanding or is everyone else?

Tell your manager that you want to refactor it. Just don't tell your manager "it's shit".

The "don't say it's shit" was when speaking to the manager... not using it on your thread. Unless I'm the one misunderstanding the statement...

PaulHoule
Both, but more to the boss. I can't stand the whole "Get Shit Done" thing, the way that word gets used on HN is like a universal qualifier, i.e.

ForAll(x): Shit(x)

josephmx
Is swearing on HN banned now?
hamiltonkibbe
Apparently not, I personally find the phrase "2000 line function" offensive but that part isn't censored.
tinco
No, but a large part of the community is in the US where many people (not all) take offense to even mild swear words (in contrast to for example the UK where even politicians won't hesitate to call someone a wazzock).
sharemywin
you probably need a ton of unit tests for this thing.
mod
You test the shit out of it, then re-write it ensuring tests pass, then add your new code.

It's probably easier to do that than to muck around in the 2000-line version hoping you don't screw something else up.

pinkunicorn
Makes sense. So step #1, get 100% coverage for that thing.
NDizzle
Yep, this is the only way to handle these beasts. Don't touch anything. Make a ton of tests. Spend some time validating those tests. Get someone to write off that those tests are sufficient. Start refactoring!
falcolas
For a function with 2,000 lines of code, we have to be honest with ourselves and accept that testing will never be sufficient; if there's 2,000 lines of code, you can bet good money that there's global state manipulation as well.

Making the assumption that anybody is capable of sufficiently testing such functions and subsequently re-writing them will only introduce new bugs and old regressions.

Seems harsh, but I've had to work with a few such monstrosities. Global state galore.

seanwilson
I've been here too. If possible, try and pull someone in who's familiar with the code to hopefully help you test and simplify it.

2000 lines of code is a massive amount of behaviour to understand. There's a minuscule chance you can infer all that behaviour from reading the code unfortunately.

johnmaguire2013
Global state is one of the trickiest problems we are trying to solve at my workplace. Our entire, 10 year old, PHP codebase relies on a single static class (and now some supporting static classes) aptly called "Meta". What does it do? Meta things.

It's basically an abstraction over the entire database layer, that has probably over 30 toggles that denote where and how it should fetch data, given a table name and a function call.

And every database call in the entire project is dependent on it.

NDizzle
That's why I mentioned getting a write off on the changes. If several peers, project managers, etc write off that they expect the behavior to be one of these things, covered by these other tests, and it's not, well... Everyone just got educated if and when it breaks!
aeneasreid
Test if you can, but I agree. Thinking unit testing will solve the problem can be incredibly naive especially if the code has lots of global state. Sometimes, just setting up all this state for a test is equivalent to rewriting the application. If the rest of the application is written like this, chances are this function makes a ton of 2k line calls of its own. Unit testing just might make sure you aren't doing something catastrophically dumb.
awayaccount53
The other trick once you have tests is to change the code to be easier to work with without changing its behavior. Prefer doing this in small steps that you can be confident about. Once the code is in a form amenable to change, which usually means a change in one place doesn't have non-obvious behavior in other places, bugfixes can be applied.
[dead] s73v3r
Unless it's going to take a really long time, don't tell the boss it needs to be refactored; just do it.
kpil
Pft, 2000 lines :-)

What you do is that you:

A) Realize that there might be years of hidden bugs, but also fixes to complex problems, speedups, kludges, weird changes to requirements, etc, hiding in the code, so even really creative unit-test might not cover real-world edge-cases.

B) Take as small bit as you can, and just refactor out the smelly code into smaller parts. Often a good name around smelly code helps a long way.

D) Iterate. Until good enough.But not more... Maybe you have more important things to do than rewriting ugly code that works.

There is a big risk that since you probably won't understand everything that goes on, and why - you will break something important.

falcolas
Slowly and carefully, like you're defusing a malfunctioning clockwork bomb.

1) Understand at a very fundamental level what it does (there are probably multiple things).

2) Understand at a very fundamental level what it does (this is really the hard part)

3) Document your hard-earned understanding (tests, ironically, do not work well for this, since there are no fundamental units to test at, and there likely is a metric ton of shared state)

4) Refactor it into something easier to understand and test.

I've had to work with a similar 5,000 line C function at the heart of a DSL parser; literally the core of the entire business. Changes were made very slowly, and with a lot of consideration. Refactoring is still underway 6 years later.

wnevets
I would add landmarks and do small refactoring within the function until I'm confident in my ability to start splitting it up into smaller functions. This should happen naturally as you make changes and add new features. However if you're not in the function enough to make this happen then it's probably not worth refactoring to the business.
ndesaulniers
1. Unit tests. Otherwise modifying old code is full of potentially unforeseen landmines.

2. Read the function a few times to get a sense of what it's doing.

3. Break the function into blocks of statements that are performing a related task.

4. Factor those out into smaller functions.

5. Recursively repeat 3-5 until you're satisfied with the new functions' line count.

> Should I tell my manager that the function needs to be refactored and take time to refactor it?

It's absolutely worth mentioning. Time estimations are one of the harder tasks of Software Engineering. Being up front with your manager can help better set expectations.

afarrell
Unit tests might be hard on a function like this and you might have to admit that your tests are more integrationey than you'd like.
rrauenza
I would add assertions in as well as you iterate. It will validate whether you understand the flow of information through the function.
phpnode
Absolutely this. Not only do assertions make it easier to validate your understanding, they capture and preserve that understanding for future developers (including yourself, 6 weeks from now).

Assertions are comments which can always be trusted and which never get out of date. It baffles me that there's so much reluctance to use them (especially in dynamic languages).

bdavisx
There's no way to add unit tests to a multi-thousand line method. There's almost no way something that long was written in a testable manner.

You can probably add some tests around the inputs/outputs, depending on the structure. For example you might have to check the database before/after and other things like that. But it won't be "unit" tests in the conventional sense. More like Integration tests, because something that long should not only be multiple methods, but multiple classes.

davidw
Indeed.com, Craigslist and maybe Linkedin.
rbrogan
You could start by writing comments. There is little chance of any bugs coming from that. If there are any tests available then you could work on understanding the tests and then step through the code in the debugger. Hope that helps.
frozenport
Where a function ends and where a function starts is somewhat artificial. For example, if this were BASIC it would be just be a label.

I would read the thing a few times and figure out exactly where to add the functionality. Refactor second, if time permits. If the thing is 2k long clearly, refactoring it isn't a priority.

tinco
Don't tell you manager anything, just do it. If someone tells you to add functionality to a 2000 line function, it takes extra time. If you want the company to benefit a little bit more from the time you spend on grokking it (by saving the next poor soul some time) by splitting it up.

Go to the part that you suspect you need to modify, identify a block of code that together forms something you can give a name, i.e. "initialize_doodad_struct", "validate_widget_params", pull it out put it in a function. Now look at all identifiers that are referenced or declared in your block of code, do a ack or ctrl+f through the 2000-N line function to 100% verify that they're not referenced anywhere. If they aren't you can safely encapsulate them in your new function (if you're working in a static language you probably could just right-mouse-click-extract to do all this).

Then immediately verify that the code still works. Run it, ideally in unit tests but most certainly also in real world(-like) scenarios. Do this for every small step. If your codebase takes 2 hours to compile, instead make a commit. Make a commit for every single 3-10 line extraction you do. (do it anyway, it'll look good and everyone will love you)

And then repeat until either the part you wanted to modify is clear to you, or there's nothing more to extract ;)

By the way, I'm a Ruby programmer mainly, and in Ruby any function over 10 lines is frowned upon. I know that in some programming language communities (looking at you C and C++!) a 200 line function doesn't raise any eyebrows, and they'll sometimes even scold you for extracting a function if it doesn't 'add' anything. I think this is because in C every function pollutes the global namespace, so there's a little more cost associated with it.

radicalbyte
If you write C/C++ then you probably need fast code. Function calls - unless inlined - have a small cost. The cost is really rather small (think memory writes) and can be negligible depending on where the values are stored.

That is of course rarely the reason. From what I've experienced it's usually that the developer is uncomfortable with abstraction (at least that was my reason many moons ago).

joshka
If speed of algorithm / function is a concern on particular functions, write a test that measures this. Refactor as much as you like until you hit the limit of readability vs adequate speed.
azth
> and in Ruby any function over 10 lines is frowned upon

Citation required.

tinco
Sorry you're right. It's 5 lines:

https://robots.thoughtbot.com/sandi-metz-rules-for-developer...

azth
For certain web apps, maybe that can work. Not for anything slightly more complex though.
odonnellryan
Needing to be longer than five lines? What's your limit?

I can see some view functions being a bit more, but that's boilerplate.

A function should do one thing. It's really, really hard to do one thing in more than a few lines.

azth
Just take a look at any OS/graphics/machine learning/etc. codebase.
odonnellryan
Okay, I have.

I haven't seen any code that could not be refactored to be clearer and shorter. Not being a dick, but five lines is a lot.

davimack
I would actually approach this differently. I'd go back to whomever the authority is with regards to what the function is supposed to do, or to the spec, and then rewrite it from scratch. This won't ensure that it behaves as it does now but with a little bit added - it will ensure that it behaves as per the specification / user requirement. If the function is that huge, with just if's, it's very likely that it's buggy and that those bugs haven't been addressed. They won't be addressed if you duplicate the functionality in a refactor unless you can get a handle on the purpose of the thing to begin with. Scrapping it and rewriting it is not a bad thing - frequently, you'll make something much tighter if you do that.

(note: would be happy for you to have left the "offensive" word in - 2,000 lines of code for a single function screams for a refactor, and says that the original dev had no clue how to write good code. if the code makes you want to cuss, well....)

joshka
-1 on this. 2000 lines shows that there's a good history of edge cases that are potentially relied on by various other parts of the application or system. Much of the time, the code is the spec. Writing tests for this function will allow you to rewrite an equivalent better function safely, as well as discover functionality that is hidden / ambiguous from the spec, and bugs that may trip you up in the second iteration (whether generated via refactoring or rewriting).
tahssa
Do a re-write. If your manager says no to the re-write then add another if condition to handle the current business logic and, at the same time, look for a new job (which is probably what the last guy did).
azeirah
Something I found to be really useful is to remove all statements inside the code, so you'll be left with large skeleton structures. Look at it from a distance, (small font), see some patterns. Can you split it up?

getting a feel for the structure of the function helps me understand it.

Ex:

function someFunction () {

    if () {
        for () {
            if () {

            } else {

            }
        }

        if () {
            for () {
            
            }
        }
    }

}
pinkunicorn
Ah I do this with if() block folding in Vim.

In this case, the developer actually has return statements inside some if blocks. A general point that has stood out from all comments is to first understand how the function achieves what the function is trying to achieve.

odonnellryan
A return point inside an if statement is a great place to do a refactor. Refactor anything inside that if into its own function, write a test, and see what happens :)
al2o3cr
To expand on that, something I sometimes find useful with giant functions with too many 'ifs' is to pull out the code that runs for each of the possibilities. Frequently I'll discover that there are actually several different overlapping functions (possibly with some common parts) mashed together with the differences wrapped in 'if'.

Ferinstance:

  def giant_method
    if thingy1?
      do_stuff
    else
      do_something_else
    end
    do_common_stuff
    if thingy1?
      finish_up_for_thingy1
    else
      finish_up
    end
  end
It's easy to spot that there are really two paths through the method (depending on what thingy1? returns) in this simplified form, but rewriting it will make it clearer even in a 2k line function:

  def giant_method
    if thingy1?
      do_stuff
      do_common_stuff
      finish_up_for_thingy1
    else
      do_something_else
      do_common_stuff
      finish_up
    end
  end
This is also useful even when the 'if' clauses aren't identical - even then, there are frequently implied relationships (for instance, if one checks if something is positive and the other checks for zero) that can simplify things.
timonoko
I remember linear lists of 1000+ if-statements. These were "state machines", popular form of real-time multitasking 50 years ago. Nothing wrong with that. You might make an analyzer which recognizes longer series of events and impossible states, but why bother. This is a programming paradigm, best you can do is to go with the flow.
agentultra
I've had to do this once. They don't teach you managing code like this! A friend gave me a copy of Working Effectively With Legacy Code[0] which helped me.

The gist of it: a strong suite of integration and unit tests. Isolate small code paths into logical units and test for equivalency.

[0] http://www.amazon.com/Working-Effectively-Legacy-Michael-Fea...

lectrick
1) Cover code with tests 2) Refactor. Martin Fowler wrote a famous book on refactoring patterns that are guaranteed to merely transform the code and not break functionality. 3) Rerun tests, repeat till everything passes
zerr
Do not touch it.
alkonaut
I think making small opportunistic fixes is best. Do what you can do without massively delaying your work every time you need to work in that area of the code. That is, leave the code better than you found it, each time.

If you add the new logic this time and also split it into four 500-line methods and maybe a few extra tests, you have done quite a lot and it is likely doable within the time frame of the work you are doing.

But

If there are zero tests and you aren't confident that you can create those tests then I'd bring it up with management. Maybe they'll say that changes in this functionality will be very rare after this small change and that it's very well tested manually and by customers - then that's it. Not all code has to be made concise and elegant, it's better to focus your effort on the code that regularly needs changing. However, in this case I'd ask management to consider not changing it at all, or make sure it will be very well tested manually after your changes).

panamafrank
quit, get a job working in a newer more modern language or just a company working on 'green field' projects. I worked on legacy code bases for ~3 years as a junior grunt and it contributed to some kind of PTSD-lite disorder.

or read the 'ol refactoring book...