Hacker News Reader

Comment by simias | Hacker News Reader

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.

View on HN

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.

Reply on news.ycombinator.com