Hello fellow devs,
While reviewing code, there are some patterns that I frequently stumble upon. To me, they are clear anti-patterns, but the fact that I see them so often suggests people have different ideas about this. I would like to have a discussion about two of those (from my perspective) anti-patterns.
The goal: This is not intended to add yet more work on everybody’s plate. It’s about having increased certainty and direction during regular patch reviews.
Disclaimer: The code I use here as example is just that: an example. I respect the authors of the code, and my aim is to collaborate and discuss to improve Blender. I also realise that there are sometimes good reasons to write code this way; in this post, I want to talk about the default, go-to way of doing things.
Too Defensive
There are quite a few bits of code of the following form:
void build_some_thing(const Main *bmain, Thing *r_thing)
{
if (bmain == nullptr || r_thing == nullptr) {
return;
}
// ... actually build the thing ...
}
To me this is dangerous code, as it hides potential bugs behind hard-to-diagnose behaviour. A NULL-check is of course fine, but then it should test for a valid use case of that particular function (return params can be optional, for example, but this function cannot do its work when it has no valid pointers).
To take a file importer as a more concrete example, if bmain == NULL, the entire importer cannot work. If you want to handle this situation, this should thus be done at the appropriate point in the code where the import can be aborted and an error message can be shown to the user. It is not something to handle deep in the code at the point where bmain is used to actually create some material. If it’s done in that way, the caller also has to handle cases where the material wasn’t added at all, even if that could only be caused by bugs that could be detected way earlier than that. Basically coding like this removes the trust that a function does what it should do.
In these situations my approach would be to either just use the pointer (and crash if it’s NULL), or use BLI_assert_msg() if I want to be more defensive / explicit.
Mixing precondition checks with “core functionality”
It’s often tempting to write code like this:
bool the_function(...) {
if (things_are_ok) { do the thing }
else { show error; return false; }
// do other things
return true;
}
Or even worse:
bool the_function(...) {
bool result = false;
if (things_are_ok) { do the thing; result = true; }
else { show error; }
// do other things, maybe also modifying result
return result;
}
In the above patterns, “doing the thing” is inside some conditional. When this pattern is applied repeatedly, it becomes harder and harder to find the normal flow of the code, as the core functionality of the function is nested further and further. It also separates the error handling from the error checking, making the unhappy flow harder to follow as well.
I would aways use an approach like this:
bool the_function(...) {
if (!things_are_ok) { show error; return false; }
// do the thing
// do other things
return true;
}
This has the added advantage that adding more precondition checks will just push the core of the function downwards, instead of nesting it further.
This also applies to loops, by the way, where the entire body of the loop is enclosed into some conditional. I’ve seen plenty of those. Especially when the body is more than two or three lines, I’d typically do if (!ok) continue; so that the bulk of the code sits outside of the condition.
I also believe that following my preferred approach lowers the cognitive complexity, which is seen as a good thing.
My question to you
As a general approach, do you agree with what I’ve described above? Of course there’s always cases in which other patterns are more suitable – I’m interested in the general approach, the go-to coding style, so to speak.
