Static analyzer nudges you to write clean code

Apr 16 2024

Author: Andrey Karpov

Static analyzers help not only detect errors and security flaws but also make code cleaner. Analyzers find redundant checks, duplicate actions, and other anomalies — they give the opportunity to make code simpler, nicer, and easier to read. Let's break it down with a real example of function refactoring.

Look at the C code fragment from the iSulad project.

/* container state to disk */
int container_state_to_disk(const container_t *cont)
{
    int ret = 0;

    if (cont == NULL) {
        return -1;
    }

    ret = container_save_container_state_config(cont);
    if (ret != 0) {
        return ret;
    }

    return ret;
}

At first, the code seems quite neat. However, it's redundant and makes us spend more time than necessary reviewing it.

I've found this code fragment due to the PVS-Studio analyzer warning:

V523 The 'then' statement is equivalent to the subsequent code fragment. container_unix.c 878

I mean this snippet:

if (ret != 0) {
  return ret;
}
return ret;

Indeed, no matter what the value of the ret variable is, the same action is performed.

It's not a bug. However, the analyzer warning makes us think about how clean the code is. And now we see how such "trivial things" can help with good refactoring.

We can try to shorten the bottom code chunk to a single return:

/* container state to disk */
int container_state_to_disk(const container_t *cont)
{
    int ret = 0;

    if (cont == NULL) {
        return -1;
    }

    ret = container_save_container_state_config(cont);
    return ret;
}

Let's keep enhancing the code. Declaring variables at the beginning of a function is an outdated and risky coding practice. It all has started with the languages, where developers have been required to declare all variables at the beginning of a function. However, pre-declaration isn't a nice thing. Instead, it increases the chance of making a mistake. For example, devs may start using a variable that has not yet been initialized.

So, it's recommended to declare a variable close to its use. Moreover, it's better to immediately initialize it at the point of declaration.

Here, the ret variable is also initialized when it's declared. But it's "the wrong initialization" and just doesn't make sense. Theoretically, it may protect against the error of using uninitialized variables. However, writing it in a way that eliminates even the possibility is better. So, it's recommended to combine the variable declaration and its initialization by a useful value.

In our case, it looks like that:

/* container state to disk */
int container_state_to_disk(const container_t *cont)
{
    if (cont == NULL) {
        return -1;
    }

    int ret = container_save_container_state_config(cont);
    return ret;
}

Here, we can see that the ret variable isn't needed at all. That's a reason to remove it:

/* container state to disk */
int container_state_to_disk(const container_t *cont)
{
    if (cont == NULL) {
        return -1;
    }

    return container_save_container_state_config(cont);
}

The code was simplified and shortened. Good.

"Objection!" someone might say, "Developers wrote the code fragment that can be further extended." Oh, come on. Stop writing code "just in case of its extension". It's unlikely that the extension will ever be needed! And if it happens, adding new code chunks isn't a very difficult task.

This picture is right on point here:

Let's move on. Pay attention to the comment for the function. It doesn't make any sense. It just repeats the function name.

/* container state to disk */
int container_state_to_disk(const container_t *cont)

The comment doesn't clarify anything, doesn't help anything. It's useless and should be deleted.

int container_state_to_disk(const container_t *cont)
{
    if (cont == NULL) {
        return -1;
    }

    return container_save_container_state_config(cont);
}

We have a simple and beautiful function as a "layer" that checks a pointer and passes control to another function.

It's that it? It turns out, it isn't. Let's look into the other called function:

static int container_save_container_state_config(const container_t *cont)
{
    int ret = 0;
    parser_error err = NULL;
    char *json_container_state = NULL;

    if (cont == NULL) {
        return -1;
    }

    container_state_lock(cont->state);
   ....
}

Wow! It appears that the function has a pointer check, allowing us to pass NULL safely.

So, the code is simplified more:

int container_state_to_disk(const container_t *cont)
{
    return container_save_container_state_config(cont);
}

What does that mean? It means that two functions are synonymous.

It would be better to leave a comment about it, rather than adding useless checks and actions to the function. As we found out, it's just junk code and only takes up space in memory.

It needs a comment like this to "make it better":

/* Currently, the container_state_to_disk function is the
   synonymous with container_save_container_state_config.
   We change it when we'll implement the feature for .......
*/
int container_state_to_disk(const container_t *cont)
{
    return container_save_container_state_config(cont);
}

What if there is no "when we'll implement"? We can simplify and shorten it even more using the macro:

#define container_state_to_disk container_save_container_state_config

The macro doesn't even need a comment because it's clear that these two functions are synonymous.

P.S. Actually, I'm not a big fan of macros. However, living without them in C is a difficult thing. Using them carefully doesn't harm or complicate the code. That's why I think it's quite appropriate to reduce everything to a macro.

Write simple code. Don't write redundant code "just in case". Thanks for your attention.

#Cpp