How not to do a code review

Transcript

1. How not to do a code review Disencourage by review

2. None

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