while (2): horror code | Just Another Computer Science Blog

Horror Code - Copying & Pasting Errors

The following piece of code contains a trivial error.

        int v;
        char buf[11], *p;
        memset(buf, 0, sizeof(buf));

        /* Fill buf with a string */

        v = strtoul(buf, &p, 16);
        p = NULL;      // <--- This is wrong!!
        if (buf == p) {
                fprintf(stderr, "format error <%s>\n", buf);
                return -1;
        }

        /* Some other code */

        memset(buf, 0, sizeof(buf));

        /* Fill buf with a string */

        v = strtoul(buf, &p, 16);
        p = NULL;      // <--- This is wrong!!
        if (buf == p) {
                fprintf(stderr, "format error <%s>\n", buf);
                return -1;
        }

The content inside the if's will never be executed, because buf is statically allocated hence it will never be NULL. Of course the issue is the row "p = NULL;". Inside p there should be the pointer to the first invalid character (i.e. not an hexadecimal digit) inside buf. So the condition should evaluate to true if the string does not contain any valid hex number.

This is a trivial error, but the thing that makes it funny is that it has been repeated twice in few rows, probably because of an unfortunate copy & paste operation.

I'm a big fan of the Ctrl+C Ctrl+V sequence, but I know that it can make you lose more time than what you save by not retyping the same thing.

In this particular case, the error is quite subtle and it can remain unnoticed for years (as actually it did). This is because strtoul() returns zero whenever the string doesn't start with an hexadecimal digit. But zero may be a valid value for further operations.

My suggestion is to always check twice when you copy and paste some code: you can easily replicate unnoticed errors.

horror code Tuesday, June 16, 2015 No comments

Horror Code - Loop And Re-loop

Some time ago, a colleague of mine told me to look at a function. It was something similar to this:

void foo(struct bar array[], unsigned int count)
{
        /* some initialization code */

        for (int i = 0; i < count; i++) {
                /* 30 rows of code
                   doing something with array[i]*/
        }

        for (int i = 0; i < count; i++) {
                /* other 20 rows of code
                   doing something with array[i]*/
        }

        /* some cleanup code */
}

At first, I thought that, in the first loop, some data needed by the second loop were calculated. But after a closer look, I found that this was not the case. Furthermore, I saw that the first five or six rows of both loops were the same.

The explanation is that the second loop has been added years later the first by a different developer that didn't want to waste time in understanding what the first loop did. You may not like it, but it works, unless you have performance issues. Personally, I think there are funnier ways to make two loops in a row.

horror code, optimization needed, working life Friday, April 17, 2015 No comments

Horror Code: Paid By The Number Of Rows

The first thing I've thought is: "I'm missing something". But after few seconds the doubt that the author of the following code is paid for the number of lines of code come to my mind. Judge yourself:

struct data_node *node = calloc(1, sizeof(*node));
if (node) {
        node->data = NULL;
        node->info = NULL;
        node->next = NULL;
}
comm->data_list = node;

Since node isn't used anywhere else, those six lines are exactly equivalent to the following:

comm->data_list = calloc(1, sizeof(*(comm->data_list)));

The function calloc returns a pointer to a memory area that is guarantee to be blank, so there is non need to assign NULL to the members of the structure.

Checking the value returned by calloc is a good thing. It may be that there is no more memory available so that error should be handled. But in this case the author didn't take any action in case of failure.

Use a temporary variable may be useful in some situations, but this is not one of those.

In conclusion, the code is formally correct: it doesn't have bugs or cause harms and maybe the compiler can optimize it. But the point is that redundant and useless code can lead to difficulties in reading and understanding besides subtle hard-to-find bugs.

Image by goopy mart licensed under CreativeCommons by-nc-sa 2.0

C, horror code Friday, November 28, 2014 No comments

Horror Code - Why?

while (x >= 0) {
        x--;
        y--;
}

I've only a question: why?

horror code, optimization needed Thursday, October 09, 2014 No comments

Horror Code: the Matryoshka Functions

Image by Fanghong and Gnomz007

Some years ago, when I was a Windows developer, the maintenance of a big project has been assigned to me. My job was to fix a couple of minor bugs and add some new functions. The problem was that the creator (and previous maintainer) have resigned years before, leaving absolutely no documentation.

The only thing I could do was to look the code and read the few comments. At some point, I found a call to the function GetName(). Changing the name returned by that function was one of the purpose of my job, so I looked for the implementation. And this is what I have found:

CString GetName()
  {
    return GetValue("Name");
  }

Pretty useless, don't you think? OK, let's see the implementation of GetValue():

#define MAIN_APP   "Main"

CString GetValue(CString szField)
  {
    return GetField(MAIN_APP, szField);
  }

Is this a joke? Well, let's see where the story ended:

#define MAIN_INI   "main.ini"

CString GetField(CString szSection, CString szField)
  {
    char szResult[100];

    GetPrivateProfileString(
        (LPCTSTR) szSection,
        (LPCTSTR) szField,
        "App",
        szResult,
        100,
        MAIN_INI
    );

    return szResult;
  }

Surprised? Now I'm sure you are wondering why someone had done this. I'm still wondering too.

horror code, personal Sunday, August 10, 2014 No comments

Horror Code: the Impossible Function

This is one of the less readable functions that I've found in my life. I've removed any reference to structures and variable names, according to the Italian law about privacy ;-)

I don't want to add any other comment. Enjoy!

static int my_function( /* Parameters */ )
{
  /* Some code */
  int j;
  for(j = 0; j < 2; j++) {
    /* Some code */
    if (j == 0) {
      /* Some code */
    } else {
      /* Some code */
    }
    /* Some code */
    if ( /* Condition */ ) {
      if ( /* Condition */ ) {
        /* Some code */
        if ( /* Condition */ ) {
          /* Some code */
          if ( /* Condition */ ) {
            /* Some code */
            if ( /* Condition */ ) {
              int i;
              /* Some code */
              for(i = 0; i < length; i++) {
                /* Some code */
                if ( /* Condition */ ) {
                  /* Some code */
                  if ( /* Condition */ ) {
                    if ( /* Condition */ ) {
                      /* Some code */
                      if ( /* Condition */ ) {
                        /* Some code */
                        if ( /* Condition */ ) {
                          /* Some code */
                          if ( /* Condition */ ) {
                            /* Some code */
                          }
                          /* Some code */
                          if ( /* Condition */ ) {
                            /* Some code */
                          } else {
                            /* Some code */
                          }
                        } else {
                            /* Some code */
                        }
                        /* Some code */
                      } else {
                        /* Some code */
                      }
                      /* Some code */
                    } else {
                      /* Some code */
                    }
                  } else {
                    /* Some code */
                  }
                } else {
                  /* Some code */
                }
              }
            } else {
              /* Some code */
            }
          } else {
            /* Some code */
          }
        } else {
          /* Some code */
        }
        /* Some code */
      } else {
        /* Some code */
      }
      /* Some code */
    }
  }
  /* Some code */
  if ( /* Condition */ ) {
    /* Some code */
  } else {
    /* Some code */
  }
  /* Some code */
  return 0;
}

The real function has 113 rows and most of the code in the else's is made of debug prints. The innermost code, the only one that actually does something, is indented 13 (thirteen) times.

Wasn't there a clearer way to write that function?