How not to do a code review
Disencourage by review
Slide 2
Slide 2 text
No content
Slide 3
Slide 3 text
Who am I
● Erik Witthauer (alias dknx01)
● PHP developer for > 10 years
● Twitter: @ewnx01
● Aka:
○ need bad examples, I have some
○ Mr. Anti-Pattern
Slide 4
Slide 4 text
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
Slide 5
Slide 5 text
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
Slide 6
Slide 6 text
Repeat the same a lot
● Writing “you forget the Docblock”
on every method or variable
● Summarize this
Slide 7
Slide 7 text
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
Slide 8
Slide 8 text
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
Slide 9
Slide 9 text
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
Slide 10
Slide 10 text
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)
Slide 11
Slide 11 text
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).
Slide 12
Slide 12 text
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
Slide 13
Slide 13 text
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
Slide 14
Slide 14 text
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
Slide 15
Slide 15 text
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
Slide 16
Slide 16 text
Doing everything manually
● Checking style, architecture or
functions only manually
● Us code sniffers
● Writing tests
● Use tools for checking
architecture styles (e.g.Deptrac)
Slide 17
Slide 17 text
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