Clever Geek Handbook

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 reveal a variety of errors and typos that are not noticeable 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 structopt library : 



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 have found it, you will surely agree that it is not easy to find such a typo. 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' does not 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 thought of as a variation of the " last line effect ". The error was made in the very last condition of the function. In the end, the programmer's attention waned, and he made this subtle mistake.





If you like the article about the effect of the last line, I recommend reading other similar observations: 0-1-2 , memset , comparisons .



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


More articles:


All Articles