Some anti-patterns

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) {
  // ... 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.


in build_some_map since neither of the parameters are expected/desired to ever be null, wouldn’t passing them by reference be the way to go ?


I agree with all of the above.

Another anti-pattern I notice (especially in all this old animation module code) is declaring all variables at the tops of functions instead of trying to maintain locality by keeping the declaration as close to the first use as possible. I think this is a holdover from ancient C rituals.

1 Like

Not if the file is in C! :upside_down_face:

it’s passing a std::map

Generally I do agree with what you’ve described.

But it is not really clear to me what would the outcome of all us agreeing be. Can’t say I am looking forward cleanup commits which converts all of the current C code to early returns as it could end up in an increased risk of temporary allocations leak. As part of a bigger code clarification it could work.

For the C++ code is something what we can easily manage even for simple and isolated cleanup patches.

I agree with you here. I was not thinking adding yet more work on everybody’s plate, and more about having increased certainty / direction during regular patch reviews.

1 Like

It is, and I’m glad it’s already mentioned in the Style Guide: Variable Scope :slight_smile: