13. Table-style formatting

Fragment taken from the ReactOS project (open-source operating system compatible with Windows). The error is detected by the following PVS-Studio diagnostic: V560 A part of conditional expression is always true: 10035L.

void adns__querysend_tcp(adns_query qu, struct timeval now) {
  ...
  if (!(errno == EAGAIN || EWOULDBLOCK || 
        errno == EINTR || errno == ENOSPC ||
        errno == ENOBUFS || errno == ENOMEM)) {
  ...
}

Explanation

The code sample given above is small and you can easily spot the error in it. But when dealing with real-life code, bugs are often very hard to notice. When reading code like that, you tend to unconsciously skip blocks of similar comparisons and go on to the next fragment.

The reason why it happens has to do with the fact that conditions are poorly formatted and you don't feel like paying too much attention to them because it requires certain effort, and we assume that since the checks are similar, there are hardly any mistakes in the condition and everything should be fine.

One of the ways out is formatting the code as a table.

If you felt too lazy to search for an error in the code above, I'll tell you: "errno ==" is missing in one of the checks. It results in the condition always being true as the EWOULDBLOCK is not equal to zero.

Correct code

if (!(errno == EAGAIN || errno == EWOULDBLOCK || 
      errno == EINTR || errno == ENOSPC ||
      errno == ENOBUFS || errno == ENOMEM)) {

Recommendation

For a start, here's a version of this code formatted in the simplest "table" style. I don't like it actually.

if (!(errno == EAGAIN  || EWOULDBLOCK     || 
      errno == EINTR   || errno == ENOSPC ||
      errno == ENOBUFS || errno == ENOMEM)) {

It's better now, but not quite.

There are two reasons why I don't like this layout. First, the error is still not much visible; second, you have to insert too many spaces to align the code.

That's why we need to make two improvements in this formatting style. The first one is we need to use no more than one comparison per line: it makes errors easy to notice. For example:

a == 1 &&
b == 2 &&
c      &&
d == 3 &&

The second improvement is to write operators &&, ||, etc., in a more rational way, i.e. on the left instead of on the right.

See how tedious it is to align code by means of spaces:

x == a          &&
y == bbbbb      &&
z == cccccccccc &&

Writing operators on the left makes it much faster and easier:

   x == a
&& y == bbbbb
&& z == cccccccccc

The code looks a bit odd, but you'll get used to it very soon.

Let's combine these two improvements to write our code sample in the new style:

if (!(   errno == EAGAIN
      || EWOULDBLOCK
      || errno == EINTR
      || errno == ENOSPC
      || errno == ENOBUFS
      || errno == ENOMEM)) {

Yes, it's longer now - yet the error has become clearly seen, too.

I agree that it looks strange, but nevertheless I do recommend this technique. I've been using it myself for half a year now and enjoy it very much, so I'm confident about this recommendation.

I don't find it a problem at all that the code has become longer. I'd even write it in a way like this:

const bool error =    errno == EAGAIN
                   || errno == EWOULDBLOCK
                   || errno == EINTR
                   || errno == ENOSPC
                   || errno == ENOBUFS
                   || errno == ENOMEM;
if (!error) {

Feel disappointed with the code being too lengthy and cluttered? I agree. So let's make it a function!

static bool IsInterestingError(int errno)
{
  return    errno == EAGAIN
         || errno == EWOULDBLOCK
         || errno == EINTR
         || errno == ENOSPC
         || errno == ENOBUFS
         || errno == ENOMEM;
}
....
if (!IsInterestingError(errno)) {

You may think that I'm dramatizing things, being too much of a perfectionist. But I assure you that errors are very common in complex expressions, and I wouldn't ever bring them up weren't they 'so frequent. They are everywhere. And they are very difficult to notice.

Here's another example from WinDjView project:

inline bool IsValidChar(int c)
{
  return c == 0x9 || 0xA || c == 0xD || 
         c >= 0x20 && c <= 0xD7FF ||
         c >= 0xE000 && c <= 0xFFFD || 
         c >= 0x10000 && c <= 0x10FFFF;
}

The function consists of just a few lines, but it still has an error. The function always returns true. The reason, in the long run, has to do with poor formatting and programmers maintaining the code for many years being unwilling to read it carefully.

Let's refactor this code in the "table" style, I'd also add some parentheses:

inline bool IsValidChar(int c)
{
  return
       c == 0x9
    || 0xA
    || c == 0xD
    || (c >= 0x20    && c <= 0xD7FF)
    || (c >= 0xE000  && c <= 0xFFFD)
    || (c >= 0x10000 && c <= 0x10FFFF);
}

You don't have to format your code exactly the way I suggest. The aim of this post is to draw your attention to typos in "chaotically" written code. By arranging it in the "table" style, you can avoid lots of silly typos, and that's already great. So I hope this post will help you.

Note

Being completely honest, I have to warn you that "table" formatting may sometimes cause harm. Check this example:

inline 
void elxLuminocity(const PixelRGBi& iPixel,
                   LuminanceCell< PixelRGBi >& oCell)
{
  oCell._luminance = 2220*iPixel._red +
                     7067*iPixel._blue +
                     0713*iPixel._green;
  oCell._pixel = iPixel;
}

It's taken from the eLynx SDK project. The programmer wanted to align the code, so he added 0 before the value 713. Unfortunately, he forgot that 0 being the first digit in a number means that this number is octal.

An array of strings

I hope that the idea about the table formatting of the code is clear, but I feel like giving couple more examples. Let's have a look at one more case. By bringing it here, I am saying that the table formatting should be used not only with conditions, but also with other various constructions of a language.

The fragment is taken from Asterisk project. The error is detected by the following PVS-Studio diagnostic: V653 A suspicious string consisting of two parts is used for array initialization. It is possible that a comma is missing. Consider inspecting this literal: "KW_INCLUDES" "KW_JUMP".

static char *token_equivs1[] =
{
  ....
  "KW_IF",
  "KW_IGNOREPAT",
  "KW_INCLUDES"
  "KW_JUMP",
  "KW_MACRO",
  "KW_PATTERN",
  ....
};

There is a typo here - one comma is forgotten. As a result two strings that have completely different meaning are combined in one, i.e. we actually have:

  ....
  "KW_INCLUDESKW_JUMP",
  ....

The error could be avoided if the programmer used the table formatting. Then, if the comma is omitted, it will be easy to spot.

static char *token_equivs1[] =
{
  ....
  "KW_IF"        ,
  "KW_IGNOREPAT" ,
  "KW_INCLUDES"  ,
  "KW_JUMP"      ,
  "KW_MACRO"     ,
  "KW_PATTERN"   ,
  ....
};

Just like last time, pay attention, that if we put the delimiter to the right (a comma in this case), you have to add a lot of spaces, which is inconvenient. It is especially inconvenient if there is a new long line/phrase: we will have to reformat the entire table.

That's why I would again recommend formatting the table in the following way:

static char *token_equivs1[] =
{
  ....
  , "KW_IF"
  , "KW_IGNOREPAT"
  , "KW_INCLUDES"
  , "KW_JUMP"
  , "KW_MACRO"
  , "KW_PATTERN"
  ....
};

Now it's very easy to spot a missing comma and there is no need to use a lot of spaces - the code is beautiful and intuitive. Perhaps this way of formatting may seem unusual, but you quickly get used to it - try it yourself.

Finally, here is my short motto. As a rule, beautiful code is usually correct code.

results matching ""

    No results matching ""