4 ms·
If the function is called from 37 places, you need to refactor your code, but to answer your question on that point: it depends. DRY feels like the right answer
by PaulRobinson 1y ago
If the function is called from 37 places, you need to refactor your code, but to answer your question on that point: it depends. DRY feels like the right answer, but I think we'd have to review an actual code example to decide.
On examples where you're talking about a library function, I think you have to accept that as a library you're in a special place: you're on an ownership boundary. Data is moving across domains. You're moving across bounded contexts, in DDD-speak. So, no, you look after your own stuff.
EnterCriticalSection suggests a code path where strong validation on entry - including if conditions - makes sense, and it should be thought of as a domain boundary.
But when you're writing an application and your regular application functions have if statements, you can safely push them out. And within a library or a critical code section you can move the `if` up into the edges of it safely, and not down in the dregs. Manage your domain, don't make demands of other people's and within that domain move your control flow to the edge. Seems a reasonable piece of advice.
However, as ever, idioms are only that, and need to be evaluated in the real world by people who know what they're doing and who can make sensible decisions about that context.
- worik 1y ago> 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
- bognition 1y agoIf 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?
- kazinator 1y agoBecause then you are calling middleware_caching_auth_broker() from 37 places instead of authenticate(). Just the name has changed, not the 37.
- all2 1y agoBut 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.
- KPGv2 1y ago> 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.
- bognition 1y agoNo 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
- kazinator 1y agoThe 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.
- jovial_cavalier 1y agoPray 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?
- cakealert 1y agoThis 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.
- jovial_cavalier 1y agoSo 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.
- cakealert 1y agoIMO you should assert it's not null. There should never be a circumstance where you pass a null pointer to a function.
- kazinator 1y agoISO 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!
- kenjackson 1y agoRefactoring 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.
- CJefferson 1y agoI 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?