Code review... done well

Tips and best practices for reviewing code in your organisation

Transcript

1. CODE REVIEW… DONE WELL
   DEVFEST17 - NOVEMBER 2017
   WARREN GAVIN (@APOKRUPTO)
   

   View Slide

2. WHAT YOU WANTED
   WHAT YOU WROTE
   CODE REVIEW - @APOKRUPTO
   

   View Slide

3. IS THIS OK?
   CODE REVIEW - @APOKRUPTO
   if (myBool) {
   // ...
   }
   

   View Slide

4. IS THIS OK?
   CODE REVIEW - @APOKRUPTO
   float myBool = a - b;
   if (myBool) {
   // ...
   }
   

   View Slide

5. WE NEED TO IMPROVE OUR CODE
   CODE REVIEW - @APOKRUPTO
   

   View Slide

6. PEER CODE REVIEWS ARE THE
   SINGLE BIGGEST THING YOU
   CAN DO TO IMPROVE YOUR
   CODE
   JEFF ATWOOD
   CODINGHORROR.COM
   CODE REVIEW - @APOKRUPTO
   

   View Slide

7. View Slide

8. View Slide

9. procmail.c
   CODE REVIEW - @APOKRUPTO
   

   View Slide

10. procmail.c
    CODE REVIEW - @APOKRUPTO
    

    View Slide

11. A CAUTIONARY TALE
    CODE REVIEW - @APOKRUPTO
    

    View Slide

12. View Slide

13. View Slide

14. View Slide

15. TOTES LIED
    TO YOU
    

    View Slide

16. View Slide

17. SOME RULES
    CODE REVIEW - @APOKRUPTO
    

    View Slide

18. View Slide

19. View Slide

20. View Slide

21. View Slide

22. View Slide

23. View Slide

24. View Slide

25. View Slide

26. BEST PRACTICES
    CODE REVIEW - @APOKRUPTO
    

    View Slide

27. View Slide

28. Best Kept Secrets of Peer Code Review © 2013 SmartBear Software
    CODE REVIEW - @APOKRUPTO
    

    View Slide

29. REVIEW BETWEEN
    200-400 LOC
    CODE REVIEW - @APOKRUPTO
    

    View Slide

30. Best Kept Secrets of Peer Code Review © 2013 SmartBear Software
    CODE REVIEW - @APOKRUPTO
    

    View Slide

31. INSPECT 300-500 LOC
    PER HOUR
    CODE REVIEW - @APOKRUPTO
    

    View Slide

32. View Slide

33. NO MORE THAN 90
    MINUTES
    CODE REVIEW - @APOKRUPTO
    

    View Slide

34. AS THE AUTHOR, BE
    PREPARED
    CODE REVIEW - @APOKRUPTO
    

    View Slide

35. Best Kept Secrets of Peer Code Review © 2013 SmartBear Software
    CODE REVIEW - @APOKRUPTO
    

    View Slide

36. AS THE AUTHOR, BE
    PREPARED
    LIKE EXAM REVISION
    HOW OFTEN DO YOU WRITE CODE
    AND SAY “GOOD ENOUGH”?
    CODE REVIEW - @APOKRUPTO
    

    View Slide

37. AS THE AUTHOR, BE
    PREPARED
    LIKE EXAM REVISION
    HOW OFTEN DO YOU WRITE CODE
    AND SAY “GOOD ENOUGH”?
    CODE REVIEW - @APOKRUPTO
    

    View Slide

38. VERIFY FIXES
    CODE REVIEW - @APOKRUPTO
    

    View Slide

39. HAVE A BIG EGO
    CODE REVIEW - @APOKRUPTO
    

    View Slide

40. CODE REVIEW - @APOKRUPTO
    

    View Slide

41. AVOID UNINTENDED
    CONSEQUENCES
    CODE REVIEW - @APOKRUPTO
    

    View Slide

42. String identifier = “foo”;
    AVOID UNINTENDED
    CONSEQUENCES
    CODE REVIEW - @APOKRUPTO
    

    View Slide

43. AVOID UNINTENDED
    CONSEQUENCES
    PREFER ENUMS OVER STRINGS & INTS
    PREFER PROMOTED TYPES
    SEND STRICT, ACCEPT STRICT
    CODE REVIEW - @APOKRUPTO
    

    View Slide

44. AVOID UNINTENDED
    CONSEQUENCES
    PREFER ENUMS OVER STRINGS & INTS
    PREFER PROMOTED TYPES
    SEND STRICT, ACCEPT STRICT
    CODE REVIEW - @APOKRUPTO
    

    View Slide

45. AVOID UNINTENDED
    CONSEQUENCES
    PREFER ENUMS OVER STRINGS & INTS
    PREFER PROMOTED TYPES
    SEND STRICT, ACCEPT STRICT
    CODE REVIEW - @APOKRUPTO
    

    View Slide

46. // COMMENTS
    CODE REVIEW - @APOKRUPTO
    

    View Slide

47. PROS & CONS
    // COMMENTS
    CODE REVIEW - @APOKRUPTO
    

    View Slide

48. YOU CAN OVERDO IT
    /* Now bump the count */
    (*argc)++;
    CODE REVIEW - @APOKRUPTO wireshark
    

    View Slide

49. IS THIS SELF DOCUMENTING?
    class Fibonacci {
    // ...
    }
    CODE REVIEW - @APOKRUPTO
    

    View Slide

50. IS THIS SELF DOCUMENTING?
    class Cauchy {
    // ...
    }
    CODE REVIEW - @APOKRUPTO
    

    View Slide

51. View Slide

52. IF YOU FIND YOURSELF DOING
    THE SAME THING MORE THAN
    THREE TIMES, WRITE A SCRIPT
    CODE REVIEW - @APOKRUPTO
    

    View Slide

53. STATIC ANALYSIS FINDS:
    CODE REVIEW - @APOKRUPTO
    

    View Slide

54. STATIC ANALYSIS FINDS:
    CODE REVIEW - @APOKRUPTO
    CODE STYLE INFRACTIONS
    

    View Slide

55. STATIC ANALYSIS FINDS:
    CODE REVIEW - @APOKRUPTO
    CODE STYLE INFRACTIONS
    COMPLEX CODE
    

    View Slide

56. STATIC ANALYSIS FINDS:
    CODE REVIEW - @APOKRUPTO
    CODE STYLE INFRACTIONS
    COMPLEX CODE
    DUPLICATION
    

    View Slide

57. STATIC ANALYSIS FINDS:
    CODE REVIEW - @APOKRUPTO
    CODE STYLE INFRACTIONS
    COMPLEX CODE
    DUPLICATION
    POTENTIAL SECURITY HOLES
    

    View Slide

58. WHICH IS CORRECT?
    if (doFoo) {
    foo();
    } else {
    bar();
    }
    if (doFoo)
    {
    foo();
    }
    else

    {
    bar();
    }
    if (doFoo)
    foo();
    else
    bar();
    CODE REVIEW - @APOKRUPTO
    

    View Slide

59. WHICH IS RIGHT?
    if (doFoo) {
    foo();
    } else {
    bar();
    }
    if (doFoo)
    {
    foo();
    }
    else

    {
    bar();
    }
    if (doFoo)
    foo();
    else
    bar();
    CODE REVIEW - @APOKRUPTO
    

    View Slide

60. if ((err = SSLHashSHA1.update(&hashCtx, &clientRandom)) != 0)
    goto fail;
    if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
    goto fail;
    if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    goto fail;
    goto fail;
    if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
    goto fail;
    CODE REVIEW - @APOKRUPTO
    

    View Slide

61. COMPLEXITY
    CODE REVIEW - @APOKRUPTO
    

    View Slide

62. COMPLEXITY
    CODE REVIEW - @APOKRUPTO
    > 10 MUST REVIEW
    

    View Slide

63. COMPLEXITY
    CODE REVIEW - @APOKRUPTO
    > 10 MUST REVIEW
    > 20 MUST CHANGE
    

    View Slide

64. COMPLEXITY
    CODE REVIEW - @APOKRUPTO
    > 10 MUST REVIEW
    > 20 MUST CHANGE
    > 50 FATAL
    

    View Slide

65. COMPLEXITY
    CODE REVIEW - @APOKRUPTO
    

    View Slide

66. COMPLEXITY
    CODE REVIEW - @APOKRUPTO
    SSLHANDSHAKE.C
    

    View Slide

67. =================================================
    !!!! WARNINGS (CCN > 15 OR ARGUMENTS > 100 OR LENGTH > 1000) !!!!
    ================================================
    NLOC CCN TOKEN PARAM LENGTH LOCATION
    ------------------------------------------------
    48 18 287 2 55 SSLUPDATEHANDSHAKEMACS@81-136@SSLHANDSHAKE.C
    73 16 488 2 87 SSLPROCESSHANDSHAKERECORD@139-226@SSLHANDSHAKE.C
    108 22 799 2 146 DTLSPROCESSHANDSHAKERECORD@229-375@SSLHANDSHAKE.C
    96 39 513 2 110 SSLPROCESSHANDSHAKEMESSAGE@404-514@SSLHANDSHAKE.C
    400 137 2183 2 477 SSLADVANCEHANDSHAKE@546-1023@SSLHANDSHAKE.C
    43 18 136 1 43 HDSKSTATETOSTR@1289-1332@SSLHANDSHAKE.C
    

    View Slide

68. View Slide

69. CODE REVIEW - @APOKRUPTO
    

    View Slide

70. RECAP
    CODE REVIEW - @APOKRUPTO
    

    View Slide

71. RECAP
    CODE REVIEW - @APOKRUPTO
    REVIEW BETWEEN 200-400 LOC
    

    View Slide

72. RECAP
    CODE REVIEW - @APOKRUPTO
    REVIEW BETWEEN 200-400 LOC
    INSPECT 300-500 LOC PER HOUR
    

    View Slide

73. RECAP
    CODE REVIEW - @APOKRUPTO
    REVIEW BETWEEN 200-400 LOC
    INSPECT 300-500 LOC PER HOUR
    NO MORE THAN 90 MINUTES
    

    View Slide

74. RECAP
    CODE REVIEW - @APOKRUPTO
    BE PREPARED
    

    View Slide

75. RECAP
    CODE REVIEW - @APOKRUPTO
    BE PREPARED
    HAVE A (BIG) EGO
    

    View Slide

76. RECAP
    CODE REVIEW - @APOKRUPTO
    BE PREPARED
    HAVE A (BIG) EGO
    ADOPT CODING GUIDELINES/STYLE
    

    View Slide

77. RECAP
    CODE REVIEW - @APOKRUPTO
    AVOID UNINTENDED CONSEQUENCES
    

    View Slide

78. RECAP
    CODE REVIEW - @APOKRUPTO
    AVOID UNINTENDED CONSEQUENCES
    THINK ABOUT YOUR COMMENTS
    

    View Slide

79. RECAP
    CODE REVIEW - @APOKRUPTO
    AVOID UNINTENDED CONSEQUENCES
    THINK ABOUT YOUR COMMENTS
    AUTOMATE THE AUTOMATABLE
    

    View Slide

80. READING LIST
    CODE REVIEW - @APOKRUPTO
    IN PRAISE OF SUPERFICIAL BEAUTY
    HTTPS://CORTE.SI/POSTS/CODE/READING-CODE.HTML
    CODE REVIEWS: JUST DO IT
    HTTPS://BLOG.CODINGHORROR.COM/CODE-REVIEWS-JUST-DO-IT/
    BEST KEPT SECRETS OF PEER CODE REVIEW
    HTTPS://SMARTBEAR.COM/SMARTBEAR/MEDIA/PDFS/BEST-KEPT-SECRETS-OF-PEER-CODE-REVIEW.PDF
    11 PROVEN PRACTICES FOR MORE EFFECTIVE, EFFICIENT PEER CODE REVIEW
    HTTPS://WWW.IBM.COM/DEVELOPERWORKS/RATIONAL/LIBRARY/11-PROVEN-PRACTICES-FOR-PEER-REVIEW/
    PROCMAIL.C
    HTTPS://OPENSOURCE.APPLE.COM/SOURCE/PROCMAIL/PROCMAIL-1.2/PROCMAIL/SRC/PROCMAIL.C
    THE MYTH OF THE COST OF DEFECT
    HTTP://THKLEIN.COM/EN_US/COST-OF-DEFECT/
    A SHORT HISTORY OF THE COST PER DEFECT METRIC
    HTTP://WWW.IFPUG.ORG/DOCUMENTS/JONES-COSTPERDEFECTMETRICVERSION4.PDF
    HTTP://LMGTFY.COM/?Q=CODE+REVIEW
    

    View Slide

81. OBLIGATORY THANK YOU
    SLIDE
    CODE REVIEW - @APOKRUPTO
    

    View Slide