Hacker News Reader

Comment by m0zg | Hacker News Reader

My personal rule is: if a simple (<=10LOC) function is used fewer than 3 times in code, it's not really a function, it should be inlined. Only noobs avoid copy&paste at all costs. There's time and place for it. Consider that code is written once but read multiple times, and it's a lot harder to read it than to write it, in particular if there are endless chains of function calls, as you commonly see in e.g. Java and the like.

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.

View on HN

No need to be adversarial, we're talking about coding styles.

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.

Of course that should be

    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.

I agree that the pointer to mgr should've been const, that's a mistake on my part. On the other hand "const"ing the local integer is a bit overkill IMO (I do think that languages like Rust are right to default to const, but doing the same thing in C adds a lot of noise IMO).

From the article (and follows my experience):

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

Where did I say anything about relying on anything "indiscriminately"? Avoiding repetition at all cost is definitely a mistake. Avoiding repetition where it's justifiable could be a sensible choice. Though in your case I'd just say:

  u32 machine_id = readl(mgr->regs + OFF_MACHINE_ID) & 0xff;

and be done with it.

>Where did I say anything about relying on anything "indiscriminately"?

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.

   #define machine_id() (readl(mgr->regs + OFF_MACHINE_ID) & 0xff)

Please no. C-style macros are too leaky.

So your example isn't germane at all to the comment you replied to. The reason is that the function you quoted is an API call, it crosses a boundary in the code base. As such it separates two concerns. And actually reduces dependency. (Whatever function calls get_machine_id() shouldn't know anything about how that bit of sausage get made.)

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.

>It's always a matter of balance

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.

It’s a lot easier to read one function that calls 10 functions that are named what they do than to read one long 300 line function.

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.

Hard disagree on this. I read quite a bit of code for a living and one of my single biggest complaints about most codebases are trivial functions (or useless objects).

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.

My favorite rule is orthogonal to length. Functions should ideally be at one level of abstraction. If you are doing 100 things at a single level of abstraction, and it flows logically, then by all means, keep it togeather. But if in a high level protocol specification you, say, concatenate strings and describe the details of fetching from files etc, then it might be better to isolate those.

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

Yes, absolutely! A hundred line function can be trivial, and a oneliner can be complex. It has nothing to do with function length but how much complexity and dependencies they isolate.

What you say about specifications certainly rings true. In those cases the reader's preferred abstraction level is pretty much given.

> If your code does n operations in sequence, just list those operations. Don't hide them off somewhere.

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.

Here's a counterargument to that: you have to read and understand those "trivial" functions only once and just assume they are correct and do what they do when you keep encountering calls to them.

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

Speaking of IDEs, a cool feature would be automatic read-only inlining for short functions. Could probably help understanding what's going on in code with lots of calls.

Usually I’m not just reading other people’s code to understand it, I’m stepping through it while running it. In that case, if it is broken up by function, I get to put watches on interesting variables and step over functions and see the data change. If I see something interesting I can step into the function. With one big blob of code, I have to set breakpoints to skip over the non interesting parts.

"Inlining functions is not a readability thing."

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.

Even with using a switch statement, how would it be less readable to put each part of the statement in a function?

Imagine there are two or three key variables, and a bunch of repeated blocks of code that modify some or all of the variables. (That sounds messy but it’s the kind of thing that could easily come up in an embedded controller.)

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.

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.

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.

Good explanation. Thanks?

> Inlining functions is not a readability thing.

Inlining for readability is precisely what the linked article is about.

If you are changing global state, and each function has an order dependency, I could see that. But it seems like even then his general takeaway is not to inline functions but not to change global state and use a more functional style of programming.

I think the takeaway is both. Avoid state changes and use a functional style wherever possible. But, when you do need to make state changes, keep it inline rather than hiding it behind function calls. Better to have one long function, sectioned off by comments, than to have state changes with subtle order dependencies in several different functions.

I would go one step further. Don’t section them off with just comments, use nested scoping blocks. At least most modern IDEs then can collapse the sections and you still have the concept of local variables.

Or use a state monad to distinguish between steps that mutate state and functions that don't, making your order dependencies visible and easy to work with...

And this is how we end up with classes like AbstractSingletonProxyFactoryBean

No, just the opposite. Languages with inadequate composition facilities forcing overly specialised single-purpose classes lead to AbstractSingletonProxyFactoryBean. Small composable abstractions are how you avoid it.

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 it calls 10 functions, then the equivalent "inlined" function written under my rule is at most 100 lines long, which means it fits on one page easily, and it's almost certainly easier to read than one where you have to follow 10 different function calls. Function naming usually sucks. As they say it's one of the hardest problems in computer science. If your code is 10 lines or less, it should be self-evident (perhaps with a short comment) as to what it does.

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.

> If it calls 10 functions, then the equivalent "inlined" function written under my rule is at most 100 lines long

Only if those 10 functions weren't already inlined themselves.

But then they won't be 10 lines or less anymore.

Yeah, but if you have A() calls B() calls C(), and they're all a few lines long, how do you decide which ones to inline? If you just do it as you're writing, don't you end up missing inlining opportunities further ahead just because you've already inlined before, and therefore the resulting function is too big to be further inlined?

>> how do you decide which ones to inline

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.

Here's a counterargument to that: a developer wants to do exactly what's done in those 10 LOCs, but there's no function encapsulating that so the dev in question can't intuitively, or through the documentation, find a reusable piece of code that will save him/her 9 LOCs. So the dev goes ahead and writes the exact same logic one more time. And because it's almost impossible to find an identical piece of logic in a large codebase, you now you have 4 places where that logic is being used and will likely never be replaced with a single function call.

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.

Just because you have some helper function somewhere in your codebase doesn't mean somebody else is going to find it. It's not uncommon that you end up with multiple versions of the same helper function.

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.

> doesn't mean somebody else is going to find it

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.

actually curious: in a codebase worked on by multiple devs, how do you acquire the hint that those few lines you are about to write have been made in a reusable function by someone? Is there some naming convention, a fuzzy search, or some other technical thing? Or is it a email/standup "look at my functions" social thing? And what about functions that 95% but not exactly satisfy your requirements? What is a practical way to sync up multiple devs on small helper functions?

Documentation, Ctrl+Shift+F (on most editors) or Ctrl+T (symbol based search on VSCode) and you try looking for something that "looks like it". It's not hard, it's just that most don't do it because they are lazy and think their use case is "too special".

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.

You put them into a "common" or "helpers" file or module and you do code reviews in the team.

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.

I mostly follow the 0, 1, N rule myself. The second time the code is needed I start looking into making the snippet a function and there's no doubt of doing that on the third account.

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.

Obviously people are fairly opinionated about this.

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.

Good code is like a good book, it's written to be read. A lot of people don't get that. Small single-use functions are like footnotes.

... and maximizing entropy makes for a terrible read. Good code, like child poetry, should be allowed to rhyme.

Fully agree with you.

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?

>code is written once but read multiple times

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.

> Only noobs avoid copy&paste at all costs.

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.

I think the mistake that people make with DRY is that they apply it at the textual level. They think that if they see the same particular sequence of characters in multiple places that they've violated the principle and need to refactor.

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?

The ops team care. When they'll want you to switch to another logging output config, you'll answer one hour later saying "done". Two days later, you'll fix another two or three of those config. One week later, you'll fix the final one.

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.

Rate of change of a particular piece of code is a factor too, given. In my experience logging boilerplate has a very low rate of change, so I figured it was a good example, but I guess everyones experience is different.

That you took the time to edit your comment to say you don't care about downvotes is proof that you do care.

It is ok to care, btw. You don't need to dismiss other people who don't agree with you though.

The counterpoint to this is that if you find a bug, it has to be fixed in multiple different places, and you have to remember where those are, and that they exist.

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.

I am not sure the downvotes are from people who disagree with you. I think more likely they are from people who take issue with the way you said it. Just food for thought...

Unless, of course, these 10 lines are an invariant-preserving data manipulation code. If you inline that, then good luck ever needing to change that invariant.

Amen I wish I could up vote this 100 times - a modern compiler shouldn't need you to inline a function for speed.

couldn't you just "static inline ...." and let the compiler inline it? He is talking about C....

ah, scarface74 said the same thing :)

I feel lucky that I learned about functions late in the game. I used to write php in long long blocks of code.

When I learned about functions I was very reticent to use them and very seldomly did.

Reply on news.ycombinator.com