The Code is dark and full of errors

Code Reviews are already internalized in many teams as a tool to improve the quality of their code, although in many of them this causes more problems than benefits, more internal conflicts than consensus, more loss of time than results.

Here we talk about how Targaryen Consulting, a consultant of the Seven Kingdoms, agile and lover of craftsmanship, was the victim of a bad management of Code Reviews causing a great confrontation between Daenerys and Jon Snow, two developers, and how through an extensive review of how to work with Code Reviews and their good practices they will solve their conflicts ... Or not.

Transcript

1. The Code is dark and full of errors
2. Nicolás Patarino @npatarino
3. Disclaimer
4. The Night is dark and full of terrors
5. The Code is dark and full of errors
7. #CodeIsDark @npatarino Winterfell project
8. Petyr Baelish Project Manager
9. #CodeIsDark @npatarino Ramsay Bolton Quality Assurance
10. Daenerys Senior Full Stack Developer
11. #CodeIsDark @npatarino Walker White UX / UI
12. Jon Snow Junior Developer
13. #CodeIsDark @npatarino Winterfell project
14. #CodeIsDark @npatarino
15. #CodeIsDark @npatarino Jon Snow’s code try { obj = new URL(url); HttpURLConnection con = (HttpURLConnection) obj.openConnection(); con.setRequestMethod("GET"); BufferedReader in = new BufferedReader(new InputStreamReader(con.getInputStream())); String inputLine; StringBuffer response = new StringBuffer(); while ((inputLine = in .readLine()) != null) { response.append(inputLine); } in .close(); Type listType = new TypeToken < ArrayList < Dragon >> () {}.getType(); final List < Dragon > characters = new Gson().fromJson(response.toString(), listType); } catch (IOException e) { Log.e(TAG, e.getLocalizedMessage()); }
16. #CodeIsDark @npatarino Code Review comments This could be the worst code I’ve ever seen. I command you to restart the work from scratch You are an old developer that doesn’t know how to create good software without all your outdated shit. And you are the ugliest developer in the Seven Kingdoms.
17. #CodeIsDark @npatarino Code Review comments Respect me! I’m Daenerys Webstorm of the tech crew Targaryen, the First of Her Team, The Un-refactored, Queen of the Smells, the SOLID and the First Commit, Queen of Merge, Khaleesi of the Great Injector, Protector of the Realm™, Lady Regnant of the Seven Frameworks, Breaker of Method Chaining and Mother of Dagger
18. #CodeIsDark @npatarino Code Review comments Any man who must say “I’m the King” it’s not a true King
19. Ygritte Product Owner
20. #CodeIsDark @npatarino You know nothing Jon Snow
21. #CodeIsDark @npatarino
22. #CodeIsDark @npatarino Agile Coach
23. #CodeIsDark @npatarino Let’s start from the beginning
24. #CodeIsDark @npatarino Code review is systematic examination of computer source code
25. #CodeIsDark @npatarino Types of Code Review
26. #CodeIsDark @npatarino Formal Code Review • Printed copies of code • Multiple participants • Multiple phases ➢ Fagan inspection Planning Overview Preparation Meeting Rework Follow-up
27. #CodeIsDark @npatarino Lightweight Code Review • Pair programming • Over the shoulder • Email pass-around • Informal walkthrough • Group • Tool assisted (Pastebin, gists, specific tools)
28. #CodeIsDark @npatarino Tool Assisted • Pre Commit ◦ Social Network • Post Commit ◦ All we know ◦ Pull Request
29. #CodeIsDark @npatarino • Increase software quality • Find bugs before release • Discover the big picture • Improve design • Make code maintainable Goals
30. #CodeIsDark @npatarino • Share knowledge makes everyone better • Learn the value of constructive criticism • Create team foundations • Create Code Style Guidelines • Open your mind • Helps mentoring • Learn from teammates Advantages
31. #CodeIsDark @npatarino Consensus Meeting • Lightweight review • Post commit • Tool-Assisted: Upsource
32. #CodeIsDark @npatarino
33. #CodeIsDark @npatarino Types of reviewers
34. #CodeIsDark @npatarino Faceless man rule
35. #CodeIsDark @npatarino Assume the reviewer could give your name to Jaqen H’gar, so don’t make him get mad
36. #CodeIsDark @npatarino Types of reviewers
37. #CodeIsDark @npatarino Best Friend Forever
38. #CodeIsDark @npatarino • Doesn’t write comments to not offend • Doesn’t look very depth • Just approves to unlock the ticket
39. #CodeIsDark @npatarino Citadel Maesters
40. #CodeIsDark @npatarino • Everything need to be approved by them • They are the only authority for decisions
41. #CodeIsDark @npatarino Carefree
42. #CodeIsDark @npatarino • Approves to avoid to look the code • Approves after someone else approves first • Thinks Code Review isn’t helpful • Thinks Code Review is just a formality
43. #CodeIsDark @npatarino Son of the incest
44. #CodeIsDark @npatarino • Thinks he has all the truth • His solution it’s the best, always • Any change he suggest should be done • He is condescend to their peers
45. #CodeIsDark @npatarino The King’s hand
46. #CodeIsDark @npatarino • Cares about software quality • Knows he could learn from peers • Thinks his comments could help someone • Respect company, code and developers
47. #CodeIsDark @npatarino Who you wanna be?
48. #CodeIsDark @npatarino Planification
49. #CodeIsDark @npatarino Choose reviewer
50. #CodeIsDark @npatarino None
51. #CodeIsDark @npatarino Whoever developer wants
52. #CodeIsDark @npatarino Whoever developer wants
53. #CodeIsDark @npatarino Whoever developer wants
54. #CodeIsDark @npatarino Specialist
55. #CodeIsDark @npatarino Random
56. #CodeIsDark @npatarino Cross-Platform
57. #CodeIsDark @npatarino • Lightweight review • Post commit • Tool-Assisted: Upsource • Cross-Platform
58. #CodeIsDark @npatarino • Analyze the task ◦ How are you going to solve it? ◦ Stop starting, start finishing • Create sub-tasks for refactor ◦ Do I need to clean something up? • Ask for resources ◦ Images ◦ Translations Developer preparation
59. #CodeIsDark @npatarino Commits
60. #CodeIsDark @npatarino
61. #CodeIsDark @npatarino Jon Snow’s commit commit 070078158a5b3168ec515102a65284fhjf1958e6 Author: Jon Snow <snow@targaryenconsulting.com> Date: Mon Nov 27 15:43:21 2017 +0100 "Fix dragon's bug"
62. #CodeIsDark @npatarino Tyrion’s commit commit 070078158a5b3168ec515102a65284fhjf1958e6 Author: Tyrion<tyrion@targaryenconsulting.com> Date: Mon Nov 27 15:43:21 2017 +0100 Refill the dragon’s fire before it gets empty: The dragon's fire were filled up after the deposit it's empty. It causes that for a short time the dragon had no fire. Let’s fill it up earlier. * Add fire listener into a fire store to detect the changes in the deposit * Create a DepositAction with strategy pattern to decoupled the action of refill and others from the dragon itself. https://github.com/targaryenConsulting/ID-124
63. #CodeIsDark @npatarino Commits • Style and punctuation • What would you like to read? • Don’t allow the reviewers to guess • Metadata
64. #CodeIsDark @npatarino Top commits - Github commit 070078158a5b3168ec515102a65284fhjf1958e6 Author: Jon Snow <snow@targaryenconsulting.com> Date: Mon Nov 27 15:43:21 2017 +0100 "Initial commit"
65. #CodeIsDark @npatarino Top commits - Github commit 070078158a5b3168ec515102a65284fhjf1958e6 Author: Jon Snow <snow@targaryenconsulting.com> Date: Mon Nov 27 15:43:21 2017 +0100 "Update README.md"
66. #CodeIsDark @npatarino Top commits - Github commit 070078158a5b3168ec515102a65284fhjf1958e6 Author: Jon Snow <snow@targaryenconsulting.com> Date: Mon Nov 27 15:43:21 2017 +0100 "*** empty log message ***"
67. #CodeIsDark @npatarino Top commits - Github commit 070078158a5b3168ec515102a65284fhjf1958e6 Author: Jon Snow <snow@targaryenconsulting.com> Date: Mon Nov 27 15:43:21 2017 +0100 "Dummy"
68. #CodeIsDark @npatarino Top commits - Github commit 070078158a5b3168ec515102a65284fhjf1958e6 Author: Jon Snow <snow@targaryenconsulting.com> Date: Mon Nov 27 15:43:21 2017 +0100 "Fix"
69. #CodeIsDark @npatarino Top commits - Github commit 070078158a5b3168ec515102a65284fhjf1958e6 Author: Jon Snow <snow@targaryenconsulting.com> Date: Mon Nov 27 15:43:21 2017 +0100 "Tests"
70. #CodeIsDark @npatarino Change the history
71. #CodeIsDark @npatarino commit --amend / rebase -i git commit --amend -m "A updated commit message" git commit --amend --no-edit git rebase -i HEAD~3 reword f7f3f6d Change my name a bit squash 310154e Update README formatting and added blame pick a5f4a0d Add cat-file
72. #CodeIsDark @npatarino Create Code Review • Write a good title • Give a description explaining what you have done ◦ Remember good commits? • Could you provide some screenshots? • Ask for specific feedback • Explain how to test it • Do you have a Code Review template? • Could you review your own code before to release?
73. #CodeIsDark @npatarino Blockers
74. #CodeIsDark @npatarino Blockers • Review larger than 200 lines • Code doesn’t compile • Code doesn’t work • Tests are broken • Tests doesn’t prove the solution • Don’t follow the team’s current agreements
75. #CodeIsDark @npatarino Automation
76. #CodeIsDark @npatarino The most powerful tool we have as developers is automation. Scott Hanselman
77. #CodeIsDark @npatarino • Lint, Checkstyle, Inspectors… • Tests execution • Screenshot testing • Generate versions • APK for VROK Automation
78. #CodeIsDark @npatarino Review
79. #CodeIsDark @npatarino Preparation to review
80. #CodeIsDark @npatarino • Read the issue • How could you resolve it? • What problems could you have? • Check out the code ◦ git stash ◦ Second repo • And more important… To-Do list for reviewers
81. #CodeIsDark @npatarino Take a coffee
82. #CodeIsDark @npatarino What not to look at • Formatting and code style ◦ Automation • Don’t go further than diff ◦ Create a technical debt ◦ Don’t force Boy Scout Rule
83. #CodeIsDark @npatarino Technical Debt
84. #CodeIsDark @npatarino Technical Debt
85. #CodeIsDark @npatarino • Formatting and code style ◦ Automation • Don’t go further than diff ◦ Create a technical debt ◦ Don’t force Boy Scout Rule What not to look at • Performance ◦ Do we really need it? • Don’t look word by word ◦ Everyone has their own taste about code
86. #CodeIsDark @npatarino Don’t be a Nazi
87. #CodeIsDark @npatarino • What’s important to your company? • Aha effect • How it solves the problem? • Tests • Usability Code What to look at
88. #CodeIsDark @npatarino Usability Code
89. #CodeIsDark @npatarino • Understandable • Extensible • Maintainable Usability Code
90. #CodeIsDark @npatarino • Good data structures • Reuse instead of rewrite • Design Patterns • SOLID • Security problems • Keep your agreements • VROK Usability Code - Looking for
91. #CodeIsDark @npatarino Vrok
92. #CodeIsDark @npatarino Approval
93. #CodeIsDark @npatarino
94. #CodeIsDark @npatarino
95. #CodeIsDark @npatarino
96. #CodeIsDark @npatarino • Fail ◦ Needs to be adjusted before to going in • Passed ◦ Nothing needs to be done • Avoid to not give a Fail or Passed ◦ Go offline ◦ Technical Debt • Create a task to not merge without X passed Approval - Binary System
97. #CodeIsDark @npatarino • What if the comments infinitely increase? ◦ Go Offline ◦ Think. It’s really important your point? ◦ The code it’s gonna be damaged? When to stop
98. #CodeIsDark @npatarino Commenting
99. #CodeIsDark @npatarino Hate the code. Love the coder
100. #CodeIsDark @npatarino Aristotle Project
101. #CodeIsDark @npatarino The whole is greater than the sum of its parts Aristotle
102. #CodeIsDark @npatarino • Group Intelligence isn’t the average of the intelligence of the members of that group • Collective intelligence grows when all members have the same participation • Social Awareness Aristotle Project
103. #CodeIsDark @npatarino Psychology Safety A sense of confidence that the team will not embarrass, reject or punish someone for speaking up Amy Edmonson
104. #CodeIsDark @npatarino Empathy
105. #CodeIsDark @npatarino • See the world as others see it • Nonjudgmental • Understand another person’s feelings • Communicate your understanding of that person’s feelings back to them Empathy
106. #CodeIsDark @npatarino • Observation ◦ Facts no evaluation • Feelings • Needs ◦ Everything we do is because our needs • Request ◦ Free of demand Nonviolent Communication
107. #CodeIsDark @npatarino Humour
108. #CodeIsDark @npatarino Every decision about code reviews has to take into account that there is a lot of ego and pride involved both on the side of the developer and reviewer Ego
109. #CodeIsDark @npatarino ¿Qué es el ego? El argentino que todos llevamos dentro
110. #CodeIsDark @npatarino Hubo debate con mi nombre como Papa Como soy Argentino, pensaban que me pondría Jesús II
111. #CodeIsDark @npatarino
112. #CodeIsDark @npatarino The name of this variable is not clear. Don’t do the same always Cersei
113. #CodeIsDark @npatarino The name of this variable is not clear. Don’t do the same always Cersei
114. #CodeIsDark @npatarino The name of this variable is not clear. Do you mind to change it? Cersei
115. #CodeIsDark @npatarino Have you think what we are going to show to the user if we receive a null in this API call? Melisandre
116. #CodeIsDark @npatarino Have you think what we are going to show to the user if we receive a null in this API call? Melisandre
117. #CodeIsDark @npatarino I do like this solution a lot, the strategy pattern it’s pretty well used, but why you didn’t use a command pattern also? interface Command { fun execute() } class RefillDragonCommand(private val dragon: Dragon): Command { override fun execute() { dragon.refill() } } class CleanUpCommand(private val dragon: Dragon): Command { override fun execute() { dragon.cleanUp() } } Bronn
118. #CodeIsDark @npatarino I do like this solution a lot, the strategy pattern it’s pretty well used, but why you didn’t use a command pattern also? interface Command { fun execute() } class RefillDragonCommand(private val dragon: Dragon): Command { override fun execute() { dragon.refill() } } class CleanUpCommand(private val dragon: Dragon): Command { override fun execute() { dragon.cleanUp() } } Bronn
119. #CodeIsDark @npatarino What do you think if we add a command pattern to handle the actions? In TICKET-1234 we will have to add a new type of product and the command pattern could help us, that makes sense to you? Bronn
120. #CodeIsDark @npatarino You should have done the error handling. This Geolocation Service could send a NotGpsException and we should control it Ramsay Bolton
121. #CodeIsDark @npatarino You should have done the error handling. This Geolocation Service could send a NotGpsException and you should control it Ramsay Bolton
122. #CodeIsDark @npatarino I’ve noticed we don’t have error handling. This Geolocation Service could send a NotGpsException and we need to control it, do you mind? Ramsay Bolton
123. #CodeIsDark @npatarino
124. #CodeIsDark @npatarino I know this ticket it’s in code review a while and I’m adding a lot of comments, I don’t know this component very well, what do you think if we schedule a quick meeting? Maybe you can help me to understand why we need it, it’s ok to you? Tyrion
125. #CodeIsDark @npatarino I know this ticket it’s in code review for a while and I’m adding a lot of comments, I don’t know this component very well, what do you think if we schedule a quick meeting? Maybe you can help me to understand why we need it, it’s ok to you? Tyrion
126. #CodeIsDark @npatarino This if assumes the value of the type is going to be “24”, “36” or “76”. This is wrong, if the request come from a notification it also could be “88” or “96” Joffrey
127. #CodeIsDark @npatarino This if assumes the value of the type is going to be “24”, “36” or “76”. This is wrong, if the request come from a notification it also could be “88” or “96” Joffrey
128. #CodeIsDark @npatarino This if solves our problem in a very pragmatic way, I do like it, could this need to add a case for a request from notification? I remember the backend guys added new values (“88” or “96”) for this case Joffrey
129. #CodeIsDark @npatarino There aren't enough swear-words in the English language, so now I'll have to call you perkeleen vittupää just to express my disgust and frustration with this crap - Linus Torvalds
130. #CodeIsDark @npatarino Receiving feedback
131. #CodeIsDark @npatarino Step out of your comfort zone We invest a lot of time in our code, so submitting it to a someone for their critique can feel like putting part of our identity up for a critique
132. #CodeIsDark @npatarino • Be grateful for the feedback • You are not your code • Be open-minded • Written comments looks ruder • Breath 10 times before to reply • Think about their vocabulary Keep in mind
133. #CodeIsDark @npatarino Hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor, hodor... Hodor
134. #CodeIsDark @npatarino Hidden problems in Targaryen Consulting
135. #CodeIsDark @npatarino I just want to close my ticket
136. #CodeIsDark @npatarino I’ll wait till someone else approves to approve too
137. #CodeIsDark @npatarino She is a rockstar, I shouldn’t write comments to her
138. #CodeIsDark @npatarino Ugh! Another code review...
139. #CodeIsDark @npatarino I’ll do the Terms of Service review
140. #CodeIsDark @npatarino F***, Daenerys it’s my reviewer...
141. #CodeIsDark @npatarino Today I’ve had a bad day. Let’s do a Code Review
142. #CodeIsDark @npatarino Retrospective
143. #CodeIsDark @npatarino It’s not for everyone
144. #CodeIsDark @npatarino No place for ego
145. #CodeIsDark @npatarino No place for very sensitive
146. #CodeIsDark @npatarino Not for Schedule Driven Development
147. #CodeIsDark @npatarino It can break your team
149. #CodeIsDark @npatarino
150. #CodeIsDark @npatarino
151. Disclaimer
152. #CodeIsDark @npatarino Nobody is perfect,
153. #CodeIsDark @npatarino Nobody is perfect, but a Team can be
154. #CodeIsDark @npatarino I’m an imperfect part of a perfect team {api} Fran Pulido @FranPulidoM Victor de Francisco @vicdefran Jose del Pozo @Josedlpozo Kike Fau @KikeFau Miguel Olmedo @molmedoc Javier Aznar @JavierAznar1 Dani Jiménez @danibto Salvador Labajos @SalvatoreLabs Javier García @jargaval