John Carmack on Inlined Code (2014)
number-none.com
number-none.com
Edit: I don't even care about the downvotes. This is hard won wisdom. If you don't see it, that's your problem, not mine.
Inlining functions is not a readability thing. It’s for performance when needed. In C, you can give a compiler hint to online a function and get the best of both worlds.
Sometimes it is for readability. For example we have code that serializes commands sent to a device. It used be built with a command pattern which was hard to debug. We have replaced it with a huge switch statement and now the code is much easier to understand and refactor. Obviously this approach often doesn't work but there are cases where inlining makes code easier to read.
If you turn all those code blocks into functions, it’s no longer easy to visually inspect the code to see all the places where variable X is modified. It might not be a net win for readibility.
Personally, I wouldn’t define a function purely to contain a repeated block of code; I only do it if the code is related, i.e. all operating on the same data.
In my first example, before trying to wrap things in functions, I’d try to figure out if some of those variables can be tied together into a single object with a nice clear interface.
This goes back to not modifying global (or class global) variables and using a functional style. If you need to pass four or five variables put them in your languages equivalent of a pass by value struct.
In the case of my favorite IDE, Extract Method and creating a struct/class from a parameter list is an automated, guaranteed safe refactor.
Inlining for readability is precisely what the linked article is about.
Zygohistoric prepromorphism has the same kind of meme status as AbstractSingletonProxyFactoryBean, but the difference is that a zygohistoric prepromorphism is a one-liner you can compose together yourself out of the functions that the library in question makes available, whereas AbstractSingletonProxyFactoryBean is a class in its own right that the library in question has to define.
If your code does n operations in sequence, just list those operations. Don't hide them off somewhere.
Code that reads linearly is by far the easiest to read. At least when you are familiar with the language and idioms. It's like the difference between a novel and a choose-your-adventure book.
Modern IDEs help, but there's only so much they can do.
Of course, what exactly a level of abstraction depends on how you model, and personal opinion. But I find it a more useful starting point then "line count".
What you say about specifications certainly rings true. In those cases the reader's preferred abstraction level is pretty much given.
In principle I agree. In practice, if your function needs to do more than say five things one after another then something has gone wrong. If you were sending a human a set of instructions on how to do something, that's the point where you'd start grouping them into logical "meta-steps" (e.g. prepare the ingredients, make the broth, prepare the meat, finish cooking the dish).
Chopping a 12-step function into 3 4-step functions without regard for what each step actually does will make it less readable, just like doing the same to a recipe, or putting paragraph breaks and full stops into a passage at random. But a single function that does a long sequence of things is a code smell in the same way as an overly long sentence; it's well worth looking for a way to logically group these steps into a structure that will help the reader understand.
> Modern IDEs help, but there's only so much they can do.
Exactly. In my experience (YMMV) 100% of the time the complaints about using "too many functions" (or similar) come from people who use poor tools or don't really know how to use them.
Also, as an aside, inlining "for performance" with the inline keyword doesn't always inline. You have to use a GCC hint to force it.
Only if those 10 functions weren't already inlined themselves.
Use whichever setup maximizes readability. Lift code out into functions if it's a logical, reasonably self contained block used 3 or more times. Use your best judgment, in other words. If you require me to jump around the file (or worse, files, plural) to read your code, make sure there's a good reason for all this cognitive overhead.
I agree with you 100% - code reuse is far too overvalued and copy and pasting has a bad name.
If something "fits" a function then yes, by all means lift it as a function. But not all long snippets of code make sense when lifted out of the scope - there's no reason to wrap those.
Most of all, forcing all code to fit "merely calling functions style" is an antipattern that can make the code less understandable.
The problem is many people don't know how to read code properly: they try to read all the details at once, instead of surfing at the top level(s) and going deeper only when they actually need. Such people are the bad developers who write bad code bacause they cannot read.
My personal mantra with copy&past while programming is: if your mouse gave you a little electric shock on pasting, would you paste it anyway?
When I learned about functions I was very reticent to use them and very seldomly did.
I actually sometimes write very short helper functions that are actually meant to improve readability by abstracting some details and making the code more self-documenting.
For instance I have this code in my current application (a device driver):
static unsigned get_machine_id(struct manager *mgr)
{
u32 conf = readl(mgr->regs + OFF_MACHINE_ID);
return conf & 0xff;
}
It's called twice in the entire code. By you rule I ought to manually inline it but I think it'd actually make the code worse, not better. It's always a matter of balance, writing good code is an art.>Only noobs avoid copy&paste at all costs
Only noobs rely on rules of thumb indiscriminately.
u32 machine_id = readl(mgr->regs + OFF_MACHINE_ID) & 0xff;
and be done with it. #define machine_id() (readl(mgr->regs + OFF_MACHINE_ID) & 0xff)Well maybe I read a little too much into your comment but I interpreted it as "if you have small functions called only a small number of times in your code then you're a noob". I attempted to provide a counter-example.
I'm sure that you didn't mean it quite as literally but there probably are a bunch of young, inexperienced coders browsing this thread and I think these rules of thumb can be pretty counterproductive when they reach impressionable minds. Then you end up with people cargo-culting weird ad-hoc rules without even understanding the reasoning behind them.
>Avoiding repetition at all cost is definitely a mistake. Avoiding repetition where it's justifiable could be a sensible choice.
I completely agree on that.
const uint32_t conf = read1(mgr->regs + OFF_MACHINE_ID);
and the 'mgr' argument should be const:ed, too. :) That makes it much more clear that this kind helper is just reading something out, and not poking stuff around. Always const all the things.Have you even bothered checking if your compiler inlines this? It sure looks like a very likely candidate.
"Const parameters and const functions are helpful in avoiding side effect related bugs, but the functions are still susceptible to changes in the global execution environment. Trying to make more parameters and functions const is a good exercise, and often ends in casting it away in frustration at some point. That frustration is usually due to finding all sorts of places that state could be modified that weren't immediately obvious -- places for bugs to breed. "
OP is talking about about tying disparate parts of code together via a function call. This creates dependency. Creating dependencies without a really good reason is BAD.
Depends on your language and environment, too.
I'm more comfortable inlining C# web code than unsafe device driver code.
But remember when you inline, you can comment around code and use more expressive variable names too. It's the same logical abstraction and similar levels of readability, just accomplished with different language features.
I rather see it at the behavioural level. Important domain logic ought to have a single implementation to avoid discrepencies that can put the system in an inconsistent state. For example your e-commerce platform really ought to just have one way of calculating the total cost of an order.
But, if you've copy pasted the boilerplate code that sets up your logging subsystem in a dozen different places, who cares?
It's annoying to everyone.
If it's not config, it's library versions, it's a change in a "boilerplate" argument, ...
Code duplication makes it hard to update all the code that should be "the same". Everyone involved in actually having code updated every now and then cares about it.
I'm familiar with the expression but it's not really written once, it's written and iterated upon. If I change something in a function that's called twice, I can be confident it will be the same change both places it's called: there's no programmatic way of verifying the change is the same if I copy and pasted, there I have to compare text. Humans aren't good at that.
So in Carmack's example, what if someone adds side effects to the pure, inlined code which don't break anything at the time? They could cause issues when someone later assumes the inlined code is pure. Whereas, if you have pure function calls, this is all done for you by a machine designed for it.
The reason I do it is having several small functions gives the program a custom language. So I can effectively start programming in my own semantic terms even if I was writing C. Building high-level constructs is easier in high-level languages, of course, but you'll always have to start with the lowest building blocks and even that helps regardless of the language!
The small functions will be marked for inlining and practically also inlined anyway so there's no performance cost. But it's immensely more practical to hypothetically write:
const size_t len = get_data_size(fragment);
rather than: const size_t len = ((struct packet_header *)&fragment->databuf[4])->sz + fragment->offset;
The latter one is easy too because you know the code but you will have forgotten a year later.I write java (or try to) in such a way that the major function calls logical, sensible smaller steps, and in such a way that it is readable. If there are, generally speaking, three things a method needs to do before it returns, I don't do two of them in separate methods and then dive into the minutiae of the third right there.
So I'm afraid I don't really see it. Modularisation and encapsulation can help comprehension.
It is ok to care, btw. You don't need to dismiss other people who don't agree with you though.
Had you put that logic into a nice little function the first time, maybe with a bit of documentation, it would be reusable, better tested (because more people use it) and will save 9 LOCs every single time that logic will be used.
But, hey, I'm a noob so what do I know.
And even if you did reuse that code, you might end up with more work because while you can you use it in two places, the third place that also uses it now has slightly different requirements. Do you add a bool parameter to that function? Now you have two code paths in the function, is it still tested as well as you thought it would? Or you could inline it...
You can't predict the future. It's not worth thinking along the lines of "but maybe somebody might use it later". Only if they really do is it time to act. Don't go looking for abstractions. Let them come to you.
You either failed to document it / name it properly or the other dev was too lazy to find it. Making 10 different variations of the same thing is purely a mistake, not a good reason to justify not encapsulating code into reusable chunks.
> Now you have two code paths in the function
But the whole point here is that you don't need to know how those code paths are implemented, only that they work the way they are supposed to.
> You can't predict the future.
Just because you can't predict the future doesn't mean you shouldn't prepare for it.
Even if you're not completely up to date on what's already there you'll probably bump into it when trying to write your helper in the same place.
It's not a silver bullet, of course, but don't let perfect be the enemy of the good.
It's the same exact thing as not bothering looking for a decent library that does what you want and instead write it from scratch. Laziness. Pure laziness. (Unless there's a very valid reason not to do it)
Since I use Django Rest Framework an example comes to mind: finding out that serializers give you the chance to define `validate_{field_name}` methods rather than put all the logic into `validate`. This allows the API to return field based errors rather than a generic error message. But even experienced devs often don't know about this feature. They go by intuition and can't be bothered to spend a few more mins looking at the docs.
If only that were true. I've seen plenty of people with 10-20 years of experience do this too. You know those coding standards that absolutely forbid functions longer than X lines? They weren't written by noobs. They were written by old-timers who assume authority based on mere longevity.
The fact is that there are tradeoffs. The "giant function" approach does create risk that code will be copied and tweaked incorrectly, or violate the assumptions under which the original was correct. This risk can be ameliorated with asserts and/or good tests, but it never goes away entirely. OTOH, the "splatter" approach can be disastrous for subsequent navigation and reading of the code, and the kicker is that it doesn't eliminate the copy-pasta problem at all. I've had to fix many bugs caused by copying between many small functions, followed by either not updating them consistently or someone calling the wrong one. Generally I don't like it, but that's also a bit language-specific. In languages that have explicit guard/context constructs or objects with RAII there's little excuse. In Plain Old C function nesting is a valid way to deal with the cascading-error-cleanup problem. It's still not the approach I prefer, but I'm not enough of a jerk to deride others as noobs for using it.
As stated in the beginning of the article, these thoughts were from 2007 (not 2014).
Aside, but can we all agree that this is what good technical leadership looks like? It's not some arbitrary law passed down about superficial stuff, it's a thought provoking _discussion_ about code quality. It specifically doesn't argue for a one-size-fits-all appraoch, but rather encourages people to consider the situation on a case by case basis. It also includes a concrete list of actions.
Readability IMHO.
Contrast -
// This function does three high level things
// doHighlevelThing3 is only called once
func () {
doHighlevelThing1()
doHighlevelThing2()
doHighlevelThing3()
}
func doHighlevelThing3 () {
doLowlevelStuff1()
doLowlevelStuff2()
doLowlevelStuff3()
doLowlevelStuff4()
doSomeBitshifting()
writeSomeMemoryLocations()
doLowlevelStuff5()
doLowlevelStuff6()
}
With - // This function does three high level things
func () {
doHighlevelThing1()
doHighlevelThing2()
doLowlevelStuff1()
doLowlevelStuff2()
doLowlevelStuff3()
doLowlevelStuff4()
doSomeBitshifting()
writeSomeMemoryLocations()
doLowlevelStuff5()
doLowlevelStuff6()
}
To me, the second one loses the readability of the first. There are three main actions func takes, but in the second example we lose the clarity on that. If we bring in all three HighLevel funcs into the low level one, we can end up with something hard to follow and digest, and the intent gets lost.Whether that gets inlined for performance reasons is entirely up to the compiler, IMHO, and if I have very strong feelings I can probably hint to the compiler (making it static in C, private in Java, whatever mechanism in other languages) that it can do what it likes in terms of optimisation.
The second one makes it clear that this function changes the state of something by writing to certain memory locations, the first one doesn't. To me that makes the second one more readable.
Seriously? So all state changes need to be made in 'main'?
Pretend there was no explicit state change in there, does that change your mind?
What if doHighlevelThing3 is actually called "changeTheThing", and the actual write to the memory location is simply the low-level implementation of that?
That's a really weird objection (to me, I acknowledge this is all style and I am opinionated).
I think all things considered, readability should generally take a back seat to correctness. I mean, the code is readable, yay!, but the game crashes randomly...not so yay...
Also, he does offer a suggestion to help address your concern:
"Using large comment blocks inside the major function to delimit the minor functions is a good idea for quick scanning, and often enclosing it in a bare braced section to scope the local variables and allow editor collapsing of the section is useful."
Finally, note that he's only suggestion that you CONSIDER it, which was my original point. It's hard to say his suggestions are wrong, when his suggestions are merely that you consider it.
I think that lack of readability is a sure path to lack of correctness, personally. Carmack may have had the luxury of working with consistently great engineers... I do not :)
I'm not saying his suggestions are wrong, merely that there are times when encapsulation makes sense. Code can be readable and correct.
BigFunction()
{
LittleFunction1(); // 50 lines
LittleFunction2(); // 50 lines
LittleFunction3(); // 50 lines
LittleFunction4(); // 50 lines
LittleFunction5(); // 50 lines
}
Which complies with the ill-advised policy, but is often not any more readable, and could be more bug prone. Does LittleFunction4() change some state variable that LittleFunction5() depends on? Who knows, better go find the source and check it. Yuck!If we pick a more realistic
RenderThePage()
{
FetchStateFromBackend()
ProcessState()
RenderTemplate()
ReturnHTMLToUser()
}
I would find that a lot easier to follow than jamming all of that code into a single massive function that did all those different things. (Not to mention, I'd find it easier to test all those things in isolation, too).To take a completely contrarian position to that being put forth in this thread, I often find it clearer to pull a single hard-to-follow line of code into a function, just to encapsulate it behind a simpler named interface.
($a, $b, $c, $d, ..., $x, $y, $z) = GetData();
PrintResult($a, $b, $c, $d, ..., $x, $y, $z);
The variables actually had reasonable names, but there were at least 20 of them. He just splitted the code at some random point and all variables that needed to be used in the second half became return values and arguments, respectively.I've done basically the same. It's as good a place as any to start getting a handle on the problem, as long as you don't leave it there.
In this example it is not clear whether the reader may need to see the content of multiple functions simultaneously to understand what's going on. Or it may depend on the circumstances whether they do. So if you have to make the choice between inlining or not there will always be pros and cons.
An optimal solution gives you the best of both worlds: have an IDE that lets you inline-expand function calls (read-only), optionally replacing parameters with arguments, and simple commands to 1) actually inline the code to make locally unique changes, 2) edit the function in-place to make global changes.
My argument is that if you've named the functions correctly, and abstracted at the appropriate level, then you shouldn't need to know what's going on in each function to understand the top level. And, if you've separated your concerns well, then the implementation of one function should not depend on state in the other function.
If on the other hand you have global state being manipulated in each of those functions, then sure, it's going to be confusing. Don't do that?
Note that my position is actually closer to what Carmack is arguing for;
> The real enemy addressed by inlining is unexpected dependency and mutation of state, which functional programming solves more directly and completely. However, if you are going to make a lot of state changes, having them all happen inline does have advantages; you should be made constantly aware of the full horror of what you are doing. When it gets to be too much to take, figure out how to factor blocks out into pure functions (and don.t let them slide back into impurity!).
He's saying that pure functions are better than inlining, because they get the readability benefits of splitting your code up, while also avoiding the problems of mutating state that's shared between multiple peer functions.
Some gems from the article were - "I'm not going to issue any mandates, but I want everyone to seriously consider some of these issues" - "[what I'm proposing is different] so I am going to try and make a clear case for it" - (at the very end)"Discussion?"
Of course it would have been even closer to the type system if he wrote that also variables should be used only once, but now Rust showed us how many advantages it can have.
It would be also interesting if a function in Rust would have to be cloned to be used multiple times (as it's an expensive operation for inlining), but of course it would have UX disadvantages in the programming language.
I'm not a type system expert, but I'm sure some of the theories that work on unique types work on non-function values and function values at the same time.
But you were correct. There is a sense of uniqueness that could be captured in a linear type system. If we want the function's computation run exactly once per frame, you might imagine a linear value created at the top of the game loop, threaded down to the callsite where the function consumes it. Alternatively, the function could emit the linear value, which then it gets returned all the way back out to something which explicitly consumes it at the end of the loop. Or maybe you encode this mechanism into the type system or as a language feature some other way, but the point is, it's not the function itself that gets used once -- it's an input or output parameter, combined with some sort of access control that guarantees only the outer game loop code can create or destroy such a value.
(By the way, Swift is especially great for writing “Style C”:
label: do { /* code here */ }
It’s self-commenting and you can break back to label: at any time, which IIRC will skip the rest of the block.)
Goodhart's law is an adage named after economist Charles Goodhart, which has been phrased by Marilyn Strathern as: "When a measure becomes a target, it ceases to be a good measure."
"To sum up:
If a function is only called from a single place, consider inlining it.
If a function is called from multiple places, see if it is possible to arrange for the work to be done in a single place, perhaps with flags, and inline that.
If there are multiple versions of a function, consider making a single function with more, possibly defaulted, parameters.
If the work is close to purely functional, with few references to global state, try to make it completely functional.
Try to use const on both parameters and functions when the function really must be used in multiple places.
Minimize control flow complexity and "area under ifs", favoring consistent execution paths and times over "optimally" avoiding unnecessary work."
What I found particularly interesting was: "I now strongly encourage explicit loops for everything, and hope the compiler unrolls it properly". I suppose looking at the generated listing code will always be the final arbiter in whether the compiler does so or not for tiny loops.
But if you work for instance on MS Word your goals will change 2-4x a year, you will have a list of hundreds of goals, your code might be reused in MS Excel without people even telling you, etc. In such kind of situation it is much, MUCH more important to encapsulate everything in classes and methods and functions (methods changing states, functions not) in a way that it can be treated as a black box. So in that kind of situation you are rather building lego blocks and hope that they can be connected easily enough to build the house that the little child, which is your boss, wants. In that case your lego blocks each need to be very testable and very connectable.
And the truth is that most of us live somewhat in the middle. We have a clear, highest goal that needs to achieved asap, while at the same time having loads of competing medium-priority goals that change all the time. So, while John's insides might be awesome by itself if you haven't thought about this topic before, please don't go full steam in that direction for a few years now. The best result is usually a little unclear and in the middle between two extremes.
I keep that quote in the back of my mind every day while working. I think it has the largest influence on how I write code now.
That's perhaps the main issue: crucial and unexpected state change hidden in functions.
Functions should do a single thing and should have explicit names.
We've all encountered functions with innocuous names a la "printSomething" that actually also changed internal state by stealth...
For a long loop no branch back is clearly silly. Is unrolling and inlining part of it worthwhile? How does it help or hider readability, unnecessary operation identification, (eg repeated intialisation) and bug identification? Does it have a tension with execution time? These answers would likely be different in 2019. SIMD intrinsics anyone because compilers can't do it well at all. Function with a descriptive name that is called or just write it all right there probably with a comment right where the function call would have been and right where it would have returned..?
We're talking low level optimisation or he wouldn't be having the discussion in terms of c++ etc. Duff is frequent touchstone is such a discussion although not mandatory by any means.
(He said trying to get a little of such discussion going ;-)
You've not explained anything, just 2 really negative posts of little productive use to anyone. No need to even post that kind of thing, right? Contributes nothing to a discussion. Explaining your point of view, might.