{"id":414168,"date":"2024-06-29T23:36:13","date_gmt":"2024-06-29T23:36:13","guid":{"rendered":"http:\/\/savepearlharbor.com\/?p=414168"},"modified":"-0001-11-30T00:00:00","modified_gmt":"2024-06-29T23:36:13","slug":"","status":"publish","type":"post","link":"https:\/\/savepearlharbor.com\/?p=414168","title":{"rendered":"Why code reviews are good, but not enough<\/span>"},"content":{"rendered":"
<\/div>\n
\n
\n
\n
\n
\"image1.png\"<\/div>\n

Code reviews are definitely necessary and useful. It’s a way to impart knowledge, educate, control a task, improve code quality and formatting, fix bugs. Moreover, you can notice high-level errors related to the architecture and algorithms used. So it’s a must-have practice, except that people get tired quickly. Therefore, static analysis perfectly complements reviews and helps to detect a variety of inconspicuous errors and typos. Let’s look at a decent example on this topic.
<\/a>
Try to find an error in the code of a function taken from the structopt<\/a> library:<\/p>\n

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; }<\/code><\/pre>\n
  To avoid accidentally reading the answer right away, I’ll add a picture.<\/p>\n
\"image2.png\"<\/div>\n
  I don’t know if you found the error or not. Even if you found it, I’m sure you’ll agree that it’s not easy to find such a typo. Moreover, you knew that there was an error in the function. If you hadn’t known, it would have been hard to make you read and check all this code carefully.<\/p>\n
  In such cases, a static code analyzer will perfectly complement the classic code review. The analyzer doesn’t get tired and will thoroughly check all the code. As a result, the PVS-Studio analyzer notices an anomaly in this function and issues a warning:<\/p>\n
  V560<\/a> A part of conditional expression is always false: input[i] <= ‘9’. structopt.hpp 1870<\/p>\n
  For those who didn’t notice the error, I will give an explanation. Here’s the main part:<\/p>\n
else if (input[i] == 'e') {   ....   if (input[i + 1] != '+' && input[i + 1] != '-' &&       (input[i + 1] >= '0' && input[i] <= '9'))       return false; }<\/code><\/pre>\n
  The above condition checks that the i-th element is the letter ‘e’. Accordingly, the following check input[i] <= ‘9’<\/i> doesn’t make sense. The result of the second check is always false<\/i>, which is what the static analysis tool warns you about. The reason for the error is simple: the person was hasty and made a typo, forgetting to write +1.<\/p>\n
  In fact, it turns out that the function doesn’t check the correctness of the entered numbers as expected. Correct version:<\/p>\n
else if (input[i] == 'e') {   ....   if (input[i + 1] != '+' && input[i + 1] != '-' &&       (input[i + 1] >= '0' && input[i + 1] <= '9'))       return false; }<\/code><\/pre>\n
  Here’s an interesting fact.<\/b> This error can be considered as a kind of the «last line effect<\/a>» one. An error was made in the last condition of the function. By the end of this snippet, the programmer’s attention weakened, and they made this barely noticeable mistake.<\/p>\n
  <\/p>\n
<\/div>\n
<\/a>
  If you like the article about the last line effect, I recommend reading about other similar ideas: 0-1-2<\/a>, memset<\/a>, comparisons<\/a>.<\/p>\n
  Bye everyone. Kudos to those who found the bug themselves.<\/p><\/div>\n<\/div>\n<\/div>\n
<\/div>\n

 \u0441\u0441\u044b\u043b\u043a\u0430 \u043d\u0430 \u043e\u0440\u0438\u0433\u0438\u043d\u0430\u043b \u0441\u0442\u0430\u0442\u044c\u0438  https:\/\/habr.com\/ru\/articles\/520258\/<\/a><\/p>\n","protected":false},"excerpt":{"rendered":"
<\/div>\n
\n
\n
\n
\n
\"image1.png\"<\/div>\n
  Code reviews are definitely necessary and useful. It’s a way to impart knowledge, educate, control a task, improve code quality and formatting, fix bugs. Moreover, you can notice high-level errors related to the architecture and algorithms used. So it’s a must-have practice, except that people get tired quickly. Therefore, static analysis perfectly complements reviews and helps to detect a variety of inconspicuous errors and typos. Let’s look at a decent example on this topic.  <\/p>\n","protected":false},"author":1,"featured_media":0,"comment_status":"open","ping_status":"open","sticky":false,"template":"","format":"standard","meta":{"footnotes":""},"categories":[],"tags":[],"class_list":["post-414168","post","type-post","status-publish","format-standard","hentry"],"_links":{"self":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/posts\/414168","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/users\/1"}],"replies":[{"embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Fcomments&post=414168"}],"version-history":[{"count":0,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/posts\/414168\/revisions"}],"wp:attachment":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Fmedia&parent=414168"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Fcategories&post=414168"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Ftags&post=414168"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}