Hacker News Reader

Refactoring due to being called more than N times seems very function dependent. As the prior author noted, I’d expect to call a lock function in some programs a lot. Likewise, memcpy. In fact I’d argue that well factored functionality is often called at many different call sites.

I can't imagine a large program where no function is useful enough to be called more than 37 times. Memory allocation? Printing? Adding a member to a list? Writing to a file?

I'm guessing you mean something else, or do you feel useful functions can't be called many times in the same program?

Pray tell, how many places is appropriate to call the same function? Is 5 too many? How about 6? When I hit 7, I have to refactor everything, right?

This only applies to a situation where you have a function that requires dynamic checks for preconditions. I would suggest that such a function (or how it's being used) is likely a blight already, but tolerable with very few call sites. In which case checking at the call site is the right move. And as you continue to abuse the function perhaps the code duplication will prompt you to reconsider what you are doing.

So if a function dereferences a pointer, it doesn't make sense to check that it's not null inside the function?

Unless there's an actual performance implication, this is all purely a matter of taste. This is the kind of broadly true, narrowly false stuff that causes people to destroy codebases. "I can't write it this way, because I have to push ifs up and fors down!!!" It's a totally fake requirement, and it imposes a fake constraint on the design.

IMO you should assert it's not null. There should never be a circumstance where you pass a null pointer to a function.

ISO C allows:

  free(NULL); // convenient no-op, does nothing

  fflush(NULL); // flush all streams; done implicitly on normal exit

  time(NULL); // don't store time_t into a location, just return it

  strtol(text, NULL, 10);  // not interested in pointer to first garbage char

  setbuf(stream, NULL);  // allocate a buffer for stream

  realloc(NULL, size); // behave like malloc(size)

and others. More examples in POSIX and other APIs:

  sigprocmask(SIG_UNBLOCK, these_sigs, NULL); // not interested in previous signal mask

  CreateEventA(NULL, FALSE, FALSE, NULL); // no security attributes, no name

Reality just ran over your opinion, oops!

I'm sorry. Are you claiming the people who designed those functions made good choices? They altered the behavior of the function considerably for a single input value that is more likely to be a bug than not.

Yes. I will probably continue to make similar choices of the future.

If there is a performance implication of moving the if into the callers or not, you can do it with an inline function.

  static inline int function(blob *ptr, int arg)
  {
     if (ptr == NULL)
       return ERR_NULL;
     return real_function(ptr, arg);
  }

Just like that, we effectively moved the if statement into 37 callers, where the compiler may be smart enough to hoist it out of a for loop when it sees that the pointer is never changed in the loop body, or to eliminate it entirely when it sees that the pointer cannot be null.

You don't need an explicit rule, you just need to be smarter than than the average mid-curve tries-too-hard-to-feel-right hn poster and realize when you're repeating a calling convention too much.

> If the function is called from 37 places, you need to refactor your code,

Really?

I do not have to think hard before I have a counter exampl: authentication

I call authenticate() is some form from every API

All 37 of them

If you are explicitly calling authenticate() for each api, you’re doing it “wrong”. At that point you want implied authentication not explicit authentication. Why not move it to some middleware that gets called in every api call?

Because then you are calling middleware_caching_auth_broker() from 37 places instead of authenticate(). Just the name has changed, not the 37.

But that's ok because the calls are hidden from the programmer.

I'm not sure if my response is serious or tongue-in-cheek. Maybe a bit of both.

> Because then you are calling middleware_caching_auth_broker() from 37 places

No you aren't. You aren't really calling it from anywhere. The framework you're using, which you aren't writing, is calling the registered middleware.

The topic here is complexity for the code structure because it's called from 37 different places. A registered middleware doesn't run into that issue because it doesn't get called anywhere that "code structure complexity" matters.

Your reasoning is isomorphic to "I'm calling a bit shift millions of times because I have written some code in a programming language." Technically true but not what we're talking about here.

That sounds like the programming equivalent to thinking that food just comes from the grocery store.

And going to a grocery store instead of 37 individual farmers…?

Is fine, but don’t pretend the farmers don’t exist and aren’t part of the system.

No that’s not how this works. You register the middleware with your web framework and it gets called as part of all web requests before the request hits your endpoints. This allows you to trust that authentication has been called for all api calls

The strongest interpretation of the remark is not that you need to refactor because you have a function called 37 times (which is likely a good thing) but rather that if you think you need to move an if statement into or out of it, you face refactoring.