How not to do a code review

Transcript

1. How not to do a code review Disencourage by review
3. Who am I • Erik Witthauer (alias dknx01) • PHP developer for > 10 years • Twitter: @ewnx01 • Aka: ◦ need bad examples, I have some ◦ Mr. Anti-Pattern
4. Opinion as fact • Must be abstract/static • Interface is needed • Naming: use fetch… instead of find… • Use pattern XYZ because I like it • No variable is used outside this method, so it could be static • Interface is better for exchange it later • We use fetch in other classes, for consistency use it here too • Explain why the pattern is better here
5. Tool as personal • Only the reviewer can mark something as done • Add label “xyz” so I can see if something changed • Discuss the usage of labels with the team • Don't use the review tool as your personal Todo list, management tool • Asked if you can block/negative influence other team members
6. Repeat the same a lot • Writing “you forget the Docblock” on every method or variable • Summarize this
7. Use your role/title • I want it this way because I'm your team lead/boss/lead developer/head of xyz/senior developer • You're just a junior, listen to me • I'm not listen to what you say, because you're a junior/normal developer • Listen to the author • Asked why he thinks it's a good way • Say what can happen or what consequences it has • Talk to the author
8. Lead by personal opinion/feeling • I'm very strict or not listen to the author because I don't like the author • I'm not looking too close, because I trust/know the author • I ignore that you're not following the style guide, because I don't want to discuss with you • Treat everyone the same way • Just look at the code • Rules must be followed (or discussed again) -> team rules
9. All in one • If you're at this point, please do this or that too, even it is not part of the ticket • Clean up all occurrences of bad naming in this file • Suggest a following ticket for clean up • Stick to the original ticket
10. Make new requirements • If we change this we must also change XYZ in our application • I think the ticket should also include xyz • Suggest a following ticket for the other parts, maybe as a blocker • Stick to the original ticket • Suggest to stop the ticket and put in to “Not ready” again ( and learn from it)
11. Assume something • Did you even test your code? • Next time test it before the MR/PR. • I think it is right. It’s very complex. • Can you tell me how you tested it, so I can reproduce it? • Can you explain what id does? It’s complex and I don’t understand it (right).
12. Ignoring toxic behaviour from high performers • Ignoring bad writing (code style) just because someone is very productive • Talk to the high performer • Remind him he is working in a team and the other must also work with it
13. Communicate via comments only • Writing a comment for everything, even small thinks • writing long or a lot of comments • try other communications: chat, telephone, face-to-face • limit the numbers of comments
14. Comment but not doing the review • Commenting to express your opinion but not answering to questions or do a full review • Ask yourself if you really will do the full review • Answering all questions to your comments
15. Treat example as a rule • In the example of our coding style guide we've not a blank line here, so remove your (aka. Tragende Leerzeile) • Check if the example follows the rule, they can be incorrect • Treat examples as examples not rules
16. Doing everything manually • Checking style, architecture or functions only manually • Us code sniffers • Writing tests • Use tools for checking architecture styles (e.g.Deptrac)
17. Never forget • You work in a team • Be friendly, never rude • Encourage others to learn / look at some new (and you too) • Treat the author as equal • It's just a review not a judgement
18. Some (might be) helpful links • https://medium.com/@sandya.sankarram/unlearning-toxic-behaviors-in-a- code-review-culture-b7c295452a3c • https://speakerdeck.com/dknx01/how-not-to-do-a-code-review/edit • http://verraes.net/2013/10/pre-merge-code-reviews/
19. Thank you Questions? Discussions?
20. "Dies ist ein sehr bedeutendes Zitat." – Ein Experte