The static analyzer encourages you to write clean code
Static analyzers help not only to detect errors and security defects, but also to make code cleaner. By identifying unnecessary checks, duplicate actions, and other anomalies, you can make your code simpler, prettier, and easier to read. Let's look at this using a practical example of function refactoring.
Let's get straight to the point and start analyzing a piece of code in C taken 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 glance, this is quite neat code. The trouble is that it is redundant and forces you to spend more time on your study than it deserves.
I came across this piece of code thanks to a warning from the PVS-Studio analyzer:
V523 The 'then' statement is equivalent to the subsequent code fragment. container_unix.c 878
We are talking about this place:
if (ret != 0) {
return ret;
}
return ret;Indeed, regardless of what value the variable has retthe same action is performed.
This is not a bug. However, the analyzer warning makes you think about the cleanliness of the code and, as you will now see, “pulling the string” can lead to good refactoring.
First, let's reduce the bottom part of the code to one 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 not dwell on this and continue improving the code. Declaring variables at the beginning of a function is an outdated, harmful style of coding. It originates from languages where all variables were required to be declared at the beginning of a function. But there is nothing wonderful about their preliminary announcement; on the contrary, it increases the chance of making a mistake. For example, start using an uninitialized variable.
It is recommended to declare a variable as close as possible to where it will be used. Moreover, it is better to immediately initialize it at the declaration point.
Here the variable ret is also initialized when declared. But this is the “wrong initialization”, it simply does not make sense. In theory, it can protect against the error of using uninitialized variables. However, it is better to write in such a way that, in principle, such a possibility does not exist. For this purpose, it is recommended to combine the declaration of a variable with its initialization with a useful value.
In our case it looks like this:
/* 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;
}It becomes obvious that the variable ret not needed at all. This is a reason to get rid of 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);
}It turned out to simplify and shorten the code. Good.
Here it can be argued that the previous version of the code was written with the expectation of its further expansion. Oh, come on. No need to write code in reserve. There's a good chance you'll never need this extension! And if this happens, then there is nothing complicated in adding the code.
This picture is appropriate here:
Let's continue. Please note the function comment. It doesn't make any sense. This is a copy of the function name:
/* container state to disk */
int container_state_to_disk(const container_t *cont)The comment doesn't explain anything and doesn't help in any way. This is a meaningless entity that should be removed.
int container_state_to_disk(const container_t *cont)
{
if (cont == NULL) {
return -1;
}
return container_save_container_state_config(cont);
}Here we have a simple, beautiful layer function that checks the pointer and transfers control to another function.
That's all? It turns out not. Let's look at that 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);
....
}Ha! It turns out that this function itself has a pointer check in it, and it can be safely passed to it NULL.
Accordingly, the code is simplified even further:
int container_state_to_disk(const container_t *cont)
{
return container_save_container_state_config(cont);
}What does it mean? That these two functions are synonyms.
This should have been written in a comment and not created meaningless checks and actions in the function. As we found out, this was all junk code that does nothing and just takes up space.
What's missing is a comment like this:
/* На данный момент функция container_state_to_disk является
синонимом container_save_container_state_config.
Это изменится, когда будет реализовываться функционал .......
*/
int container_state_to_disk(const container_t *cont)
{
return container_save_container_state_config(cont);
}What if there is no “when will it be implemented”? Then the program text can be simplified and shortened even more using this macro:
#define container_state_to_disk container_save_container_state_configThis macro doesn't even need a comment. And it is clear that these two names are synonymous.
PS In general, I don't really like macros. But it’s hard to live without them in C, and their careful use does not spoil or complicate the code. Therefore, I considered it quite appropriate to ultimately reduce everything to a macro.
Write simple code. Don't write redundant code in reserve. Thank you for your attention.
If you want to share this article with an English-speaking audience, please use the translation link: Andrey Karpov. Static analyzer nudges you to write clean code.
