Code review... done well

A84ff6526157fd1d165bfde743367382?s=47 Apokrupto
November 04, 2017

Code review... done well

Tips and best practices for reviewing code in your organisation

A84ff6526157fd1d165bfde743367382?s=128

Apokrupto

November 04, 2017
Tweet

Transcript

  1. CODE REVIEW… DONE WELL DEVFEST17 - NOVEMBER 2017 WARREN GAVIN

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

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

    // ... }
  4. IS THIS OK? CODE REVIEW - @APOKRUPTO float myBool =

    a - b; if (myBool) { // ... }
  5. WE NEED TO IMPROVE OUR CODE CODE REVIEW - @APOKRUPTO

  6. PEER CODE REVIEWS ARE THE SINGLE BIGGEST THING YOU CAN

    DO TO IMPROVE YOUR CODE JEFF ATWOOD CODINGHORROR.COM CODE REVIEW - @APOKRUPTO
  7. None
  8. None
  9. procmail.c CODE REVIEW - @APOKRUPTO

  10. procmail.c CODE REVIEW - @APOKRUPTO

  11. A CAUTIONARY TALE CODE REVIEW - @APOKRUPTO

  12. None
  13. None
  14. None
  15. TOTES LIED TO YOU

  16. None
  17. SOME RULES CODE REVIEW - @APOKRUPTO

  18. None
  19. None
  20. None
  21. None
  22. None
  23. None
  24. None
  25. None
  26. BEST PRACTICES CODE REVIEW - @APOKRUPTO

  27. None
  28. Best Kept Secrets of Peer Code Review © 2013 SmartBear

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

  30. Best Kept Secrets of Peer Code Review © 2013 SmartBear

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

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

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

  35. Best Kept Secrets of Peer Code Review © 2013 SmartBear

    Software CODE REVIEW - @APOKRUPTO
  36. AS THE AUTHOR, BE PREPARED LIKE EXAM REVISION HOW OFTEN

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

    DO YOU WRITE CODE AND SAY “GOOD ENOUGH”? CODE REVIEW - @APOKRUPTO
  38. VERIFY FIXES CODE REVIEW - @APOKRUPTO

  39. HAVE A BIG EGO CODE REVIEW - @APOKRUPTO

  40. CODE REVIEW - @APOKRUPTO

  41. AVOID UNINTENDED CONSEQUENCES CODE REVIEW - @APOKRUPTO

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

    @APOKRUPTO
  43. AVOID UNINTENDED CONSEQUENCES PREFER ENUMS OVER STRINGS & INTS PREFER

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

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

    PROMOTED TYPES SEND STRICT, ACCEPT STRICT CODE REVIEW - @APOKRUPTO
  46. // COMMENTS CODE REVIEW - @APOKRUPTO

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

  48. YOU CAN OVERDO IT /* Now bump the count */

    (*argc)++; CODE REVIEW - @APOKRUPTO wireshark
  49. IS THIS SELF DOCUMENTING? class Fibonacci { // ... }

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

    CODE REVIEW - @APOKRUPTO
  51. None
  52. IF YOU FIND YOURSELF DOING THE SAME THING MORE THAN

    THREE TIMES, WRITE A SCRIPT CODE REVIEW - @APOKRUPTO
  53. STATIC ANALYSIS FINDS: CODE REVIEW - @APOKRUPTO

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

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

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

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

    COMPLEX CODE DUPLICATION POTENTIAL SECURITY HOLES
  58. WHICH IS CORRECT? if (doFoo) { foo(); } else {

    bar(); } if (doFoo) { foo(); } else
 { bar(); } if (doFoo) foo(); else bar(); CODE REVIEW - @APOKRUPTO
  59. WHICH IS RIGHT? if (doFoo) { foo(); } else {

    bar(); } if (doFoo) { foo(); } else
 { bar(); } if (doFoo) foo(); else bar(); CODE REVIEW - @APOKRUPTO
  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
  61. COMPLEXITY CODE REVIEW - @APOKRUPTO

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

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

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

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

  66. COMPLEXITY CODE REVIEW - @APOKRUPTO SSLHANDSHAKE.C

  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
  68. None
  69. CODE REVIEW - @APOKRUPTO

  70. RECAP CODE REVIEW - @APOKRUPTO

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

  72. RECAP CODE REVIEW - @APOKRUPTO REVIEW BETWEEN 200-400 LOC INSPECT

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

    300-500 LOC PER HOUR NO MORE THAN 90 MINUTES
  74. RECAP CODE REVIEW - @APOKRUPTO BE PREPARED

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

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

    EGO ADOPT CODING GUIDELINES/STYLE
  77. RECAP CODE REVIEW - @APOKRUPTO AVOID UNINTENDED CONSEQUENCES

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

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

    YOUR COMMENTS AUTOMATE THE AUTOMATABLE
  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
  81. OBLIGATORY THANK YOU SLIDE CODE REVIEW - @APOKRUPTO