Downloaded 65 times
![118 Code Review in our Comp Let me help you out 2006, meet Froscon <?php if ($handle = opendir('.')) { while (false !== ($file = readdir($handle))) { if ($file != "." && $file != "..") { $files[] = $file; } } // closedir($handle); php closes on request end } if(!isset($files)) { die(); } ?>](https://image.slidesharecdn.com/code-review-in-practice-froscon-2011-draft0-130219050340-phpapp02/75/Code-review-in-practice-118-2048.jpg)
![128 Code Review in our Comp Growing I had Scalability Issues class __edorian { […] // Those darn Singletons private function __clone() {} }](https://image.slidesharecdn.com/code-review-in-practice-froscon-2011-draft0-130219050340-phpapp02/75/Code-review-in-practice-128-2048.jpg)
Code review in practice
- 1. Code Review in practice Froscon2011 Volker Dusch / @__edorian
- 2. 2 Introduction Me?
- 3. 3 Introduction Volker Dusch @__edorian
- 4. 4 Introduction PHP for around 9 years
- 5. 5 Introduction I‟m currently into TDD, CI, Clean Code and shipping …amongst other stuff
- 6. 6 Introduction Just go and buy those *Book covers used under fair use
- 7. 7 Introduction Ask questions at any time!
- 8. 8 Agenda We‟re gonna talk about Code Review
- 9. 9 Agenda Why do it?
- 10. 10 Agenda What types exist?
- 11. 11 Agenda How to do it?
- 12. 12 Agenda My story!
- 13. 13 Why do Code Review? Why do Code Review?
- 14. 14 Why do Code Review? Define „Code Review‟ 'Code Review' describes the systematic examination of source code
- 15. 15 Why do Code Review? Because code is written by Humans
- 16. 16 Why do Code Review? Because code is read by Humans … a lot
- 17. 17 Why do Code Review? It‟s easy to write code for machines … they will understand it …well they will execute it
- 18. 18 Why do Code Review? But Humans think differently of code
- 19. 19 Why do Code Review? Mandatory Code Review Joke Used under Fair Use: © Focus Shift
- 20. 20 Why do Code Review? So.. Code Review
- 21. 21 Why do Code Review? It‟s meant to improve
- 22. 22 Why do Code Review? Improve readability, maintainability and stability or your code ... so code quality
- 23. 23 Why do Code Review? Improve the knowledge of software developers
- 24. 24 Why do Code Review? achieved by…
- 25. 25 Why do Code Review? achieved by more people looking at the code before the customer experiences the changes
- 26. 26 Why do Code Review? achieved by finding more bugs during development
- 27. 27 Why do Code Review? achieved by making sure the code is understandable by humans
- 28. 28 Why do Code Review? achieved by bringing devs together to talk code leading to…
- 29. 29 Why do Code Review? achieved by easier team growing or team building …
- 30. 30 Why do Code Review? achieved by higher consistency formatting, architecture, …
- 31. 31 Why do Code Review? achieved by easier & more mentoring because training people rocks!
- 32. 32 Types of Code Review Types of Code Review
- 33. 33 Types of Code Review Over the shoulder do you have a moment? great! grab a chair
- 34. 34 Types of Code Review Over the shoulder This is what usually happens anyways doesn‟t matter what you call it
- 35. 35 Types of Code Review Over the shoulder People usually self organize Create Events?! Over the projector reviews
- 36. 36 Types of Code Review Pair Programming Code Review with 100% more real time than similar products
- 37. 37 Types of Code Review Pair Programming Instant feedback cycle Every step of the way
- 38. 38 Types of Code Review Automated Code Review computers can be so cruel
- 39. 39 Types of Code Review a.k.a. static code analysis phpmd, pDepend, phpcs, phpunit, phpcpd, phpdcd phploc, phpcb, pfff phantm, ppw I bet you couldn‟t read that during the presentation
- 40. 40 Types of Code Review phpqatools.org talk to people here! „nuff said
- 41. 41 Tools for Code Review Before we talk about tools
- 42. 42 Type of Code Review Amount of code to review?
- 43. 43 Type of Code Review Cycle based Every Iteration maybe?
- 44. 44 Type of Code Review Cycle based Get everyone to together
- 45. 45 Type of Code Review Cycle based It might take a while
- 46. 46 Type of Code Review Cycle based but you get the big picture
- 47. 47 Type of Code Review Feature based I think I‟m done with this What do you think?
- 48. 48 Type of Code Review Feature based Whenever something of value is done for the first time
- 49. 49 Type of Code Review Feature based Amout depends on feature / task sizes
- 50. 50 Type of Code Review Feature based Avg. 4 hours per feature? Around two reviews per day
- 51. 51 Type of Code Review Feature based Can be a short cylce
- 52. 52 Type of Code Review Feature based With big features / tasks you might run into troube That‟s 3 weeks old! I‟m not even sure I wrote it
- 53. 53 Type of Code Review Feature based Gives devs. a changes to get everything proper
- 54. 54 Type of Code Review Feature based Nice to make sure it really meets the businesses case
- 55. 55 Type of Code Review Commit based
- 56. 56 Type of Code Review Commit based Review every single checkin
- 57. 57 Type of Code Review Commit based just merges to master? „master‟ == „trunk‟ and those are feature based reviews I‟d say
- 58. 58 Type of Code Review Commit based Fast feedback
- 59. 59 Type of Code Review Commit based High traffic
- 60. 60 Types of Code Review Commit based Commit messages matter
- 61. 61 Types of Code Review Commit based Commit messages matter A LOT!
- 62. 62 Types of Code Review Commit based Tell people why a change was made If I want to know what it does I‟ll read the code
- 63. 63 Types of Code Review Commit based Make small commits As you should anyways
- 64. 64 Types of Code Review Commit based Small as in under 100 LOC What? You have classes bigger than that?
- 65. 65 Types of Code Review Commit based Small as in change 3-4 places at most You shouldn‟t need to touch everything. It‟s improper
- 66. 66 Types of Code Review Commit based But that just would be nice, it works anyways
- 67. 67 Types of Code Review Commit based Just don‟t review reformattings
- 68. 68 Types of Code Review Commit based We‟ll get back to that
- 69. 69 Tools for Code Review So let‟s talk Tools
- 70. 70 Tools for Code Review Review Board Open Source (MIT!) Eclipse Plugin Post Commit Review Discussions and so on
- 71. 71 Tools for Code Review Review Board Image used under fair use: http://www.reviewboard.org/screenshots/
- 72. 72 Tools for Code Review Review Board Image used under fair use: http://www.reviewboard.org/screenshots/
- 73. 73 Tools for Code Review Gerrit Open Source Requires git as scm Powerfull
- 74. 74 Tools for Code Review Gerrit Screenshots created from: https://review.source.android.com/
- 75. 75 Tools for Code Review Gerrit Screenshots created from: https://review.source.android.com/
- 76. 76 Tools for Code Review Fisheye / Crucible Commercial By Atlassian (JIRA) … if you use JIRA take a look
- 77. 77 Tools for Code Review SmartBear CodeCollaborator Commercial Review Board meets Enterprise Eclipse & Visual Studio Plugins
- 78. 78 Tools for Code Review CodeCollaborator Image under fair use from: http://smartbear.com/images/products/codecollaborator/ccollab-feature-sidebyside.png
- 79. 79 Tools for Code Review email pass around You had me at HELO
- 80. 80 Tools for Code Review scm sends out a mail for every commit or push or feature or whatever
- 81. 81 Tools for Code Review email pass around Is anyone here familiar with “mailing lists”
- 82. 82 Tools for Code Review email pass around Does your mail client have a “threaded view” button? All right, all we need
- 83. 83 Tools for Code Review email pass around Point being: Everyone has mail
- 84. 84 Tools for Code Review email pass around No additional tools That‟s also why IDE Plugins rock
- 85. 85 Tools for Code Review email pass around No interface learning
- 86. 86 Tools for Code Review email pass around Your process! Not the one of the tool But if a tool enforces a process it might not be a good tool
- 87. 87 Tools for Code Review email pass around Very fast cylce times 20 sec to 2 minutes per commit
- 88. 88 Tools for Code Review Enough with the tools already! Let‟s go!
- 89. 89 How to review code The first rule of Code Review is
- 90. 90 How to review code The first rule of code review
- 91. 91 How to review code The first rule of code review Well.. not really but it helps a lot!v
- 92. 92 How to review code The first rule of code review Get everyone involved!
- 93. 93 How to review code The first rule of code review No code is scared
- 94. 94 How to review code The first rule of code review Everyone gives feedback start with your juniors so they learn and don‟t just agree
- 95. 95 How to review code The first rule of code review Remember: It‟s about the code Not who wrote it
- 96. 96 How to review code What to look for?
- 97. 97 How to review code What to look for? Image used under CC-BY-ND http://creativecommons.org/licenses/by-nd/2.0/en/ Creator: Oliver Widder http://geekandpoke.typepad.com/geekandpoke/2010/11/how-to-make-a-good-code-review.html
- 98. 98 How to review code What to look for? It depends on the type of code review But let‟s get though my suggestions and we‟ll see
- 99. 99 How to review code What to look for? Does it implement the business case? Issue Tracking/Stories help a lot
- 100. 100 How to review code What to look for? Intent and purpose over actual words on paper or hard disk…
- 101. 101 How to review code What to look for? Does it fit into the applications architecture? My code always uses SpecialDbConnect7 because I wrote that class!
- 102. 102 How to review code What to look for? Overlooked edge cases? $date = new DateTime can throw an exception?! I <2 off by one errors
- 103. 103 How to review code What to look for? Is it tested? Is is maintainable? My class does everything we‟ll ever need, no need to worry about OO right?
- 104. 104 How to review code What to look for? Does it conform to coding rules?
- 105. 105 How to review code What to look for? Does it conform to coding rules? You seem to be doing a machines job. Do you want some help?
- 106. 106 How to review code What to look for? Let tools do what tools can do. They are better at some things and nobody gets mad at someone when they nag you a lot. Just make good rules
- 107. 107 How to review code What to look for? The things your rules can‟t catch.. you‟ll notice Promise
- 108. 108 How to review code What to look for? Is it going to confuse your users in unintended ways? Confusing them in intended ways is called major release Cookie lifetime now 5 minutes? Reddit says is more secure!!1
- 109. 109 How to review code What to look for? Is there a simpler way? "When debugging, novices insert corrective code; experts remove defective code" Richard Pattis
- 110. 110 How to review code What to look for? performance impacts? SQL is hard, at times Look out for everything that leaves PHP (io)
- 111. 111 How to review code What to look for? Duplicate functionality? The bigger your projects the more ways of achieving the same result?
- 112. 112 How to review code What to look for? And for the closer:
- 113. 113 How to review code What to look for? Is it easy to understand $important = getTRWTF(„daily‟);
- 114. 114 How to review code Easy to understand? Does it take you longer than a minute to grasp? Is it the code or the commit message that doesn‟t help?
- 115. 115 Questions so far? All right Story time
- 116. 116 STOP! STORYTIME! Soo… code review at our company
- 117. 117 Code Review in our Comp Do you remember 2006?
- 118. 118 Code Review in our Comp Let me help you out 2006, meet Froscon <?php if ($handle = opendir('.')) { while (false !== ($file = readdir($handle))) { if ($file != "." && $file != "..") { $files[] = $file; } } // closedir($handle); php closes on request end } if(!isset($files)) { die(); } ?>
- 119. 119 Code Review in our Comp How it started back then
- 120. 120 Code Review in our Comp How it started I was a working student
- 121. 121 Code Review in our Comp How it started One day we installed „WebSvn‟ Subversion repository browser
- 122. 122 Code Review in our Comp How it started And that came with a simple RSS Feed
- 123. 123 Code Review in our Comp How it started I started asking a lot of questions And nobody stopped me
- 124. 124 Code Review in our Comp How it started And even more questions At some point I started sending out one mail per dev per week with comments and questions about the code they commited
- 125. 125 Code Review in our Comp How it started At some day it „suddenly‟ was a part of my job I don‟t think I ever heard the term „Code Review‟ before that point. I was just asking ;)
- 126. 126 Code Review in our Comp How it started So there I was looking at feeds a lot
- 127. 127 Code Review in our Comp In the next Year our company grew. A lot!
- 128. 128 Code Review in our Comp Growing I had Scalability Issues class __edorian { […] // Those darn Singletons private function __clone() {} }
- 129. 129 Code Review in our Comp Growing So something needed to be done
- 130. 130 Code Review in our Comp Growing We already had collective code ownership
- 131. 131 Code Review in our Comp Growing At least nobody thought it was „his‟ code
- 132. 132 Code Review in our Comp Growing We also didn‟t want to stop with Code Review
- 133. 133 Code Review in our Comp Growing So everybody agreed to do peer-reviews We already talked a lot before but now we added per commit reviews
- 134. 134 Code Review in our Comp Growing I‟ve tried many tools
- 135. 135 Code Review in our Comp Growing Nothing worked out well I also didn‟t have a vision what exactly I was looking for. That didn‟t help
- 136. 136 Code Review in our Comp Growing Everything felt complicated and time intensive
- 137. 137 Code Review in our Comp Growing I was spending WAY to much time on reviews
- 138. 138 Code Review in our Comp Growing So we needed a solution
- 139. 139 Code Review in our Comp Getting it solved That was 9 month ago
- 140. 140 Code Review in our Comp Getting it solved Enter Qafoo
- 141. 141 Code Review in our Comp Getting it solved It took two days!
- 142. 142 Code Review in our Comp Getting it solved We discussed all possible solutions with the team and agreed that email pass-around would work
- 143. 143 Code Review in our Comp Getting it solved We implemented a svn-post-commit-hook using the “php-commit-hooks” from @korend svn://kore-nordmann.de/php-commit-hooks Also check out vcs_wrapper, a part of Arbit http://kore-nordmann.de/blog/vcs_wrapper_development.html We needed to extending one class to hack in your mail solution and since then I haven‟t touched that code, it just worked Well ok, we adjusted the “commiter <-> reviewer” mapping array many times and created special review circels for some projects
- 144. 144 Code Review in our Comp Getting it solved Then we created a workflow for our environment
- 145. 145 Code Review in our Comp Getting it solved Oh, environment
- 146. 146 Code Review in our Comp Getting it solved Exchange and Outlook
- 147. 147 Code Review in our Comp Workflow - Distribution list in exchange - commit hook mails there - Mails get marked as “you should review that” in the subject line using filter “ |fo|ba|etc| Repo Rev Commit” - Outlook marks those mails as important.( ! ) - Dev respons with status code (#ok, #note, #error) and an explanation - that‟s really really fast btw. - read, CTRL+L, type #ok, CTRL+ENTER, done - Continue mailing until the issue is resolved
- 148. 148 Code Review in our Comp It looks like this
- 149. 149 Code Review in our Comp One review cylce
- 150. 150 Code Review in our Comp Getting feedback
- 151. 151 Code Review in our Comp The commit
- 152. 152 Code Review in our Comp What we don‟t do
- 153. 153 Code Review in our Comp What we don’t do We don‟t review… “Cleanup commits” - We trust our tools “Buisiness case” - Takes to long. Trust devs “Maintainability” - For the older stuff “Performance Impact” - If it isn‟t obvious With those changes it works out quite nicely
- 154. 154 Your turn Froscon So far that‟s our Story Now I‟d like to hear yours
- 155. 155 Questions? Just a moment: Any open questions?
- 156. 156 Tell me! Do you already do code reviews? Share your exerience! What tools, what types, what works? Problems you ran into?
- 157. 157 Thanks a lot! Thank you for your time!