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)
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
9. procmail.c CODE REVIEW - @APOKRUPTO
10. procmail.c CODE REVIEW - @APOKRUPTO
11. A CAUTIONARY TALE CODE REVIEW - @APOKRUPTO
15. TOTES LIED TO YOU
17. SOME RULES CODE REVIEW - @APOKRUPTO
26. BEST PRACTICES CODE REVIEW - @APOKRUPTO
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
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
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
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