Upgrade to Pro — share decks privately, control downloads, hide ads and more …

How not to do a code review

dknx01
April 03, 2018

How not to do a code review

dknx01

April 03, 2018
Tweet

More Decks by dknx01

Other Decks in Programming

Transcript

  1. Who am I • Erik Witthauer (alias dknx01) • PHP

    developer for > 10 years • Twitter: @ewnx01 • Aka: ◦ need bad examples, I have some ◦ Mr. Anti-Pattern
  2. 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
  3. 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
  4. Repeat the same a lot • Writing “you forget the

    Docblock” on every method or variable • Summarize this
  5. 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
  6. 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
  7. 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
  8. 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)
  9. 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).
  10. 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
  11. 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
  12. 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
  13. 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
  14. Doing everything manually • Checking style, architecture or functions only

    manually • Us code sniffers • Writing tests • Use tools for checking architecture styles (e.g.Deptrac)
  15. 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