Why code reviews are good, but not enough

image1.png

Code reviews are definitely needed and useful. This is an opportunity to transfer knowledge, training, control of the task, improve the quality and design of the code, and fix errors. Moreover, you can notice high-level errors associated with the used architecture and algorithms. In general, everything is fine, but people get tired quickly. Therefore, static analysis is an excellent supplement to reviews and helps to identify a variety of errors and misprints that are invisible to the eye. Let’s look at a good example on this topic.

Try to find the error in the function code taken from the library structopt:

static inline bool is_valid_number(const std::string &input) {
  if (is_binary_notation(input) ||
      is_hex_notation(input) ||
      is_octal_notation(input)) {
    return true;
  }

  if (input.empty()) {
    return false;
  }

  std::size_t i = 0, j = input.length() - 1;

  // Handling whitespaces
  while (i < input.length() && input[i] == ' ')
    i++;
  while (input[j] == ' ')
    j--;

  if (i > j)
    return false;

  // if string is of length 1 and the only
  // character is not a digit
  if (i == j && !(input[i] >= '0' && input[i] <= '9'))
    return false;

  // If the 1st char is not '+', '-', '.' or digit
  if (input[i] != '.' && input[i] != '+' && input[i] != '-' &&
      !(input[i] >= '0' && input[i] <= '9'))
    return false;

  // To check if a '.' or 'e' is found in given
  // string. We use this flag to make sure that
  // either of them appear only once.
  bool dot_or_exp = false;

  for (; i <= j; i++) {
    // If any of the char does not belong to
    // {digit, +, -, ., e}
    if (input[i] != 'e' && input[i] != '.' &&
        input[i] != '+' && input[i] != '-' &&
        !(input[i] >= '0' && input[i] <= '9'))
      return false;

    if (input[i] == '.') {
      // checks if the char 'e' has already
      // occurred before '.' If yes, return false;.
      if (dot_or_exp == true)
        return false;

      // If '.' is the last character.
      if (i + 1 > input.length())
        return false;

      // if '.' is not followed by a digit.
      if (!(input[i + 1] >= '0' && input[i + 1] <= '9'))
        return false;
    }

    else if (input[i] == 'e') {
      // set dot_or_exp = 1 when e is encountered.
      dot_or_exp = true;

      // if there is no digit before 'e'.
      if (!(input[i - 1] >= '0' && input[i - 1] <= '9'))
        return false;

      // If 'e' is the last Character
      if (i + 1 > input.length())
        return false;

      // if e is not followed either by
      // '+', '-' or a digit
      if (input[i + 1] != '+' && input[i + 1] != '-' &&
          (input[i + 1] >= '0' && input[i] <= '9'))
        return false;
    }
  }

  /* If the string skips all above cases, then
  it is numeric*/
  return true;
}

In order not to accidentally read the answer right away, I will add a picture.

image2.png

I don’t know if you found a mistake or not. Even if you find it, you will surely agree that finding such a typo is not easy. Moreover, you knew that there was an error in the function. If you do not know, then it is difficult to get you to carefully read and check all this code.

In such situations, a static code analyzer will perfectly complement the classic code review. The analyzer does not get tired and will thoroughly check the entire code. As a result, the PVS-Studio analyzer notices an anomaly in this function and issues a warning:

V560 A part of conditional expression is always false: input[i] <= '9'. structopt.hpp 1870 For those who did not notice the error, I will give an explanation. The most important thing:

else if (input[i] == 'e') {
  ....
  if (input[i + 1] != '+' && input[i + 1] != '-' &&
      (input[i + 1] >= '0' && input[i] <= '9'))
      return false;
}

The above condition checks that the i-th element is the letter ‘e’. Accordingly, the following check input[i] <= '9' doesn’t make sense. The result of the second check is always false, which is what the static analysis tool warns about. The reason for the error is simple: the person hurried and sealed himself, forgetting to write +1.

In fact, it turns out that the function does not complete its job of checking the correctness of the entered numbers. Correct option:

else if (input[i] == 'e') {
  ....
  if (input[i + 1] != '+' && input[i + 1] != '-' &&
      (input[i + 1] >= '0' && input[i + 1] <= '9'))
      return false;
}

An interesting observation. This error can be seen as a kind of “last line effect“. The mistake was made in the very last condition of the function. At the end, the programmer’s attention waned, and he made this subtle mistake.

If you like the article on the last line effect, then I recommend that you familiarize yourself with other similar observations: 0-1-2, memset, comparisons

Goodbye to everyone. I like those who found the error on their own.

Similar Posts

Leave a Reply

Your email address will not be published. Required fields are marked *