
Tips for reviewing code you don’t like - kesor
https://developers.redhat.com/blog/2019/07/08/10-tips-for-reviewing-code-you-dont-like/
======
wickerman
First rule about code reviewing is that you don't tell someone how you
would've written it, but rather try to point out pros/cons of current vs
proposed approach. It's as easy as that. Every time I see a piece of code I
don't like I ask myself: why do I not like it?

I've gotten PR reviews with people literally writing "I don't like this" even
after I wrote a very detailed technical argument about my reasoning. My only
approach to that is to remind them that I really don't care about their
personal preferences, because no discussion will come out of them addressing
none of my technical arguments and instead relying on what their personal
preference is like.

~~~
organsnyder
Agreed. In these cases, the reviewer should always aim for learning as much as
teaching (if not more so).

I've gone into code reviews where my reaction began with, "ugh, this is
terrible", but evolved into, "well, I probably would have done it differently,
but this approach handles an edge case I hadn't considered".

~~~
makapuf
Completely agree with all of this. Some say be hard with the code, soft with
the coder. Also don't nitpick and be nice. Simple.

~~~
filoleg
In cases like this, I explicitly preface it with "nit: <text here>", explain
how I would do it myself instead and why, and then (also explicitly) say that
this is more of a preference thing and that they should feel free to leave it
as is. It might turn out they like my approach more and change the code, but
if not, it is still totally cool with me, as they learned of just one more way
to do the same thing that might come useful later.

------
TheCapn
I'm kind of shocked at how many recommendations are merely, "have tact and
don't be a dick to your coworkers." How many code reviews are shitslinging
mudfests where you guys work??

Now admittedly I _mostly_ work on projects where I'm the sole developer so
structured code reviews aren't part of my daily routine, but during the period
where I managed interns we'd do code reviews before every merge which mostly
involved sitting down and going through the functions. I'd point out
deficiencies and work to teach the student developers new
tools/libraries/resources they weren't aware of. At no point did we exchange
shit like "This change sucks", "Can’t you see that this is obviously wrong?",
or "This is stupid code written by a stupid person."... all examples from an
article which is also toting the "Avoid hyperbole" mantra. Even a single one
of those snide remarks would be grounds for discipline or potentially
termination if it were consistent.

My tip? Separate your ego from your code. If you can't discuss criticism
objectively and professionally you shouldn't be involved in a professional
career. Hostile workplaces are shit to be part of and I will never stand for
that among my peers.

~~~
civilian
After 2 years of developing at a scrappy design&dev shop, I got a new job that
was a big step up at a hip SAAS company. I was in my mid-20s and I still
hadn't worked through a lot of my anxiety issues, and I started getting code
review from these two intimidating brothers-- one an engineering manager and
one a principal dev. Their review style was very terse, and they called out
_every instance_ of a syntax 'mistake' I made, and it just made me feel like
absolute shit.

It wasn't just me, several of my coworkers felt the same about just getting
hammered on by them.

This has kind of permanently changed my approach to code review. I always call
out the things I like in a batch of code. I acknowledge that there are some
code styles preferences that are arbitrary, but that I have a preference in
and it's good to have a consistent codebase. If the person is working for me,
or if a coworker took a nasty task that nobody wanted, I thank them for the
code with the LGTM.

~~~
QuercusMax
This sounds like a case where having an agreed-upon code style can remove a
lot of conflict. Even if you personally don't prefer a particular way of doing
things, if it's in the style guide, then there shouldn't be any argument.

Same things go for automatic code-formatters. If every file has to be
formatted according to the canonical formatter, then you remove tons of
chances for nitpicking. Your code doesn't match what the formatter outputs?
You can't send your code out for review.

That way it's not personal; you're not saying "my way is best", you're saying,
"this is OUR way".

------
srfilipek
> 9\. Say please

I actually don't like this. Maybe it's just me, but I always read these as
being condescending, written by someone who thinks that I won't take a
suggestion without unnecessary pleasantries and/or begging, or they're
frustrated and trying to mask it.

> “Could you please align these variable definitions so they’re easier to
> read?”

> “Could you align these variable definitions so they’re easier to read?”

I like the latter.

~~~
Double_a_92
Agree. I'm not trying to do the reviewer a favor, nor are they asking one from
me.

Even if it's some personal style thing, I don't perceive "Could you...?" as
rude or anything.

If anything an exaggerated please could seem more insulting, as in "Could you
please just do this simple thing ffs!?"

~~~
nroach
Tolerance for and expectation of "please" can be cultural. OS code may receive
contributions from global contributors.

Anecdotally, in my team which spans individuals from each continent except
Africa and the poles, US english speakers are the least likely to use
'please', whereas our APAC (asia-pacific) contributors use it liberally.

~~~
wolco
Canadians will use it liberally along with thanks.

~~~
sverige
And sorry.

------
ravenstine
My biggest problem with code reviews isn't so much the phrasing(although it is
definitely important), but the fact that they almost never has a "done"
criterion.

In my opinion, if new code has nothing objectively wrong with it(no security
flaws, no unacceptable design patterns, no foreseen bugs, etc.), then the
author has the prerogative to merge the code. Reviews shouldn't go on in
perpetuity because different reviewers would all have written something
differently than the author submitting the code. I've experienced this
phenomenon at every company I've worked for at one point or another, and it's
basically a way for Waterfall to sneak its way back in to an Agile process.
(not that I'm an advocate of Agile) It doesn't happen too often, but I find it
more frustrating than just about anything else.

Beyond some reasonable adjustments to be made in the first wave or 2 of review
comments, any other non-critical changes should be separated into tickets for
the next sprint or simply be labeled as DIY or "do it yourself". If you want
something done, but there's no actual standard for doing it on your team, then
ordering people to write things your way via code review is really just a form
of _backseat coding_.

My code is never perfect, but that doesn't mean I want to change variable
names, swap out libraries, or use different approaches merely because someone
else prefers them that way. You think the variables should have different
names? Wonderful! _Go do it yourself_. I'm not trying to be rude, but I've got
_other shit to do_ , and the software already works beautifully.

~~~
fogetti
I completely agree.

Many times when someone asks me to change something without pointing to a
previously agreed-on coding standard that all team members approved before, I
just call it bullshit and let the PR sit there indefinitely while I get to my
next task. If someone is interested why the PR is not released yet, I always
point out that some people wanted to have changes but they were lazy to make
those changes themselves even though the code base was accessible for the
criticizer too. And I never fail to mention that I am already working on
something else so going back is just the waste of my time and my time is as
precious as the criticizer's.

Of course I am not this arrogant when there are agreed-on coding standards and
the reviewer points out mistakes based on those standards (but that almost
never happens). I am more than happy to make changes in that case.

So to sum it up: pull requests have become the prefect tool for workplace
bullying.

~~~
sfink
My interpretation of your comment is that if someone applies their experience
and expertise during a code review, and comes up with issues or suggestions
that are not part of a written standard, then you will assume that your
evaluation (maybe of the merits of the case, or perhaps just the importance)
is more worthwhile/accurate than theirs.

Also, your preferred approach is to passive aggressively let the PR sit until
external pressures build enough to force it through.

Do I have that right? If so, I personally find that to be a deeply problematic
approach. Though I can imagine situations where it would be defensible and
even optimal -- but those situations would involve working with idiots and/or
control freaks. In which case my preferred approach would be to find a
different place to work.

~~~
fogetti
Well you got it right. I was surrounded by control freaks. Today I am the
happiest man on earth because I don't have to put up with the idiocy of code
reviews anymore.

~~~
xpe
I’m curious: what sorts of things does your team do instead of code reviews?

Does your team have norms around code quality? How are these maintained?

What happens when a commit needs improvement? Does the team discuss it
somehow? Does someone fix it with a follow up commit?

~~~
fogetti
Pair programming, live coding sessions and study sessions. When we start
implementing something we define the definition of done and the acceptance
criteria. When the code passes linting and meets the acceptance criteria it is
accepted. If it doesn't then it's clear why not. There is no such thing as a
commit needs improvement. It either passes or fails based on the above.

------
pkamb
The problem I have with most similar articles/comments is that the only two
options for Code Review now seem to be:

\- point out strict violations of the team's enshrined code style guidelines

\- accept any and all code as-is, so long as there are no "bugs" or
performance issues

Which, at that point, why are we even doing PRs? Unit test and lint...

When I receive a Code Review I want to know about the function that reduces my
10 lines to 1. I want someone to notice that my git mistake introduced a bunch
of unneeded diff noise. I want to change variable names that are confusing to
the first reviewer, because that means they'll be confusing to me when I read
the code again next month.

Give me the development "human factors" in the code review. Some of that will
be subjective and that's ok.

~~~
JoeAltmaier
I know the pain - but its no use reducing it to a dichotomy (All or Nothing).

The strong reactions are due to pedants who block the source base doorway and
pick at everything going by. It can sink a project. So folks push back, say
things like "Only comment if its a real bug!" and so on.

There's lots of middle ground in a company source process. For instance, point
out real bugs if present. But for anything else, comment and approve,
respecting the contributing Engineer to do what they can in the time
available.

------
JamesBarney
I feel like code reviews comments should be of several types. 1\. There is a
bug in this code(most important) 2\. This violates the norms on the project
3\. This is less maintainable because it can cause a future bug(two pieces of
code in different that need to be consistent) 4\. I personally had trouble
understanding this code, here's what took me a while to understand. Would you
mind adding a comment or temp variable to speed this up. 5\. Here's a cleaner
way to do it(totally in writers court whether or not they implement this
unless they're a jr. dev.

I make a lot less code review comments than I used to because I've seen too
many bugs introduced by fairly innocuous looking code review changes and the
amount of bikeshedding that can go into code reviews comments that will likely
never materially effect the application.

~~~
goostavos
>code review comments that will likely never materially effect the application

I think that is actually a good bar for a CR comment. Issue of maintainability
affect the application. Bugs affect the application. A lack of general good
software architecture has long term effects on the application.

All other comments I either tag on an "as an option, you could..." or force
myself to shut down the "that's not how I would have done it!" voice in my
head and just leave no comment.

On the receiving end, few things are more obnoxious than "deck chair
rearranging" comments which have no bearing on performance, maintainability,
style, or correctness, but just boil down to you satisfying the benign
preferences of whoever is looking at your code.

------
Traster
This seems like a pretty good list, I managed a team previously where they
would make these mistakes in pretty much every code review - and the result of
one engineer being nasty during a code review was very quickly a culture where
code reviews were an opportunity to settle scores and showboat rather than an
actual functional process.

A lot of this advice could be boiled down to "Would you make this comment on
reddit, or would you say it to your colleague's face?"

The only thing I would add is:

>If a submission is too large to be reasonably reviewed, it is okay to let the
submitter know right away. Keep moving forward.

Doesn't seem right to me - or atleast the focus really should be making sure
that no one submits code for review that's too big for review, and that means
early feedback about a feature getting too complex and breaking it down. It's
much easier to go into a problem saying "I'm going to do X, it's going to be
huge, so I'll do A then B then C and submit them separately". If a submission
is too big to reivew it's probably non-trivial to go back and break up so that
can be quite annoying.

~~~
singron
The number 1 reason why I actually try to make code reviews small is because
the few times I made big code reviews, they just sat there. They get a few
comments here and there, but the reviewers seem reluctant to review them at
all, or just keep doing partial reads without the confidence to approve the
whole thing.

I even found the relationship non-linear. I.e. if you can break up a big
change into smaller changes that have more overall code, you can still
probably get those reviewed quicker.

I probably would have learned the lesson faster if reviewers had just rejected
the changes for being too big, or at least a "next time you could have split
it this way".

------
alexandercrohde
Honestly, in my opinion, the fact that this type of thing can even happen
suggests the format of code-review has a problem. I propose it's better to:

1\. Sit at the desk of the person and pair-code review. Helps avoid basic
misunderstadnings, and also is more private that a permanent written record
where people may feel defensive about their choices. Also it's easier for a
lot of people to be rude through the internet.

2\. Write in small notes if any must-address concerns arise and are agreed-
upon (e.g. "Security?", "duplicates function x")

If your devs can't interact face-to-face, then I think you've got major
cultural problems or hired the wrong devs.

~~~
smcameron
Or you're working on an open source project.

~~~
organsnyder
Maybe a shared-screen video call, then? Same for non-colocated colleagues.

~~~
Crinus
This makes it much harder for people to review stuff since they'll need to
schedule for such reviews. Normally reviews happen asynchronously whenever
people find time and often code review tools allow for writing a review piece-
by-piece (that is, your progress is saved on the server that hosts the review)
instead of all-in-one.

Introducing any form of friction makes reviews less desirable for everyone
involved.

~~~
organsnyder
Agreed. But for complicated reviews, a high-bandwidth synchronous meeting can
be useful. Perhaps start out asynchronous, with the reviewer requesting a live
meeting/call if they feel it's necessary? That's always worked well for me.

------
gwbas1c
I review a LOT of code, and honestly, I didn't find this article helpful. It's
common sense on how to behave in a civil manner, but it doesn't really help
with having effective code reviews.

Code reviews accomplish a few things:

\- They are primarily a _discussion_ about code changes. ("Why did you do
this?"

\- They help onboard new members of the team, both in terms of how to work
most effectively with the code, and with style. (No style guide is 100%
exhaustive, "We normally name variables for that fooBar, not barFoo.")

\- They help team members learn from each other. ("That's a cool new function
in the new version of XYZ library!")

\- They help enforce good engineering practice. ("I'm not merging that until
you write tests.")

\- They help make code more readable. ("Please add a comment here, and rename
that method to DoSomething.")

\- They also quickly identify people who shouldn't be part of the team.
Everyone sees that someone moves slowly in a pull request, or that someone
resists cleaning up sloppy code.

The thing with people who resist cleaning up their sloppy code is that it
shows to management. This happens when everyone else in the team gets their
pull requests merged quickly, and one person's pull requests pile up with lots
of minor comments.

~~~
efitz
> It’s common sense on how to behave in a civil manner

I am frequently surprised by how lacking many technical professionals are in
EQ/soft skills/civility -often I observe people who apparently believe that
their skill is such that they don’t need to treat their peers with respect.
This isn’t anomalous; it’s widespread.

[Conjecture/opinion] I believe that many people who enter software development
and related fields are high on the autism scale- eg aspergers, etc., and that
many such people are unaware of how badly that they’re treating others. It’s
not “common sense” for many.

~~~
wolfgke
> I am frequently surprised by how lacking many technical professionals are in
> EQ/soft skills/civility -often I observe people who apparently believe that
> their skill is such that they don’t need to treat their peers with respect.
> This isn’t anomalous; it’s widespread.

I personally think that terseness is actually polite since it wastes a lot
less time for both sides.

------
tawat987
I disagree with the basic premise of the article that maintainers are obliged
to merge code changes by default.

One reason is that the maintenance burden falls on more than just the author,
so there needs to be some consideration on whether it is maintainable and
whether the people involved are willing and able to maintain it.

Particularly if the change adds a public API or behavior, it may have to be
maintained for the life of the codebase.

Also, some projects have very high reliability expectations and you have to
assume that code is broken until proven otherwise. I won't merge code until
the author has demonstrated via tests, comments and clearly-written code that
the change does as expected and doesn't introduce potential issues.

The attitude in the article isn't particularly conducive to the long-term
health and quality of a project, since you get a bunch of authors ramming
through their changes based on their agenda and end up with an inconsistent
mess that nobody wants to maintain.

~~~
m463
I was mentally thinking about refuting your argument, but then I started
wondering about systemd (which I do not like, although I do have concrete
criticisms).

------
reallydontask
I use to hate receiving comments like this (the bad ones) yet I use to give
them myself almost without realising it. Obviously, I was right to give them
and wrong to receive them ;->

It took me a while to realise that the same message can be delivered in a far
nicer manner, which generally leads to a lot less conflict.

Now it almost comes naturally to write _nice_ comments that deliver the same
message, namely: the code could be improved

------
MattyRad
> 1\. Rephrase your objection as a question

Careful with this one. In my experience, you aren't fooling anyone when you
ask someone a question to which you yourself know the answer. It can quickly
come off as condescending.

One helpful practice that we have is that we make a distinction between
blocking and non-blocking comments. Nonblocking comments are desired changes,
but aren't required by the submitter to fix (formatting, code simplification,
naming, etc). This steers people in the right direction without preventing
them from getting work done. Blocking changes are critical things which must
be resolved before the code gets merged (security risks, performance concerns,
complexity concerns).

And of course, every comment should be followed with a suggestion for how to
resolve it (even if it's meeting in person). "This is bad" obviously doesn't
fly.

~~~
watwut
What helps here is to then actually listen to answer and try to learn from it.
As in, it becomes less condescending when the answer is listened to and
actively acted and reacted upon in follow up conversation.

------
raxxorrax
I have started to love these "hey programmers, here are some tips for being a
nice person for once"-articles.

I haven't been in too many code reviews, but my experience is that it is often
taken personally if developers aren't friends with each other. I vastly prefer
unit and integration tests to measure code quality.

I can also understand the reluctance of maintainers to include code of a
different style. Here the tips from the article probably help, but I think it
happens often enough that code is rejected for non-technical issues.

~~~
gtirloni
Yes, as much as possible should be automated by tests and style guides but
they don't cover everything.

I'm not aware of automated ways to validate architectural soundness and other
abstract features.

------
tomxor
> It’s not a political or emotional argument

For FOSS at least, ultimately maintainers and authors are not beholden to
anyone who wants to submit code, it's perfectly ok to say NO even for
political or emotional reasons... so if not in the direct context of a PR then
where?

I think heated and unconstructive PRs probably arise as a result of pushy and
entitled submitters, after a certain point authors will inevitably get fed up
and abandon diplomatic approaches.

------
agapon
I think that this is a very good article on code reviews:
[https://jml.io/pages/your-code-sucks-and-i-hate-
you.html](https://jml.io/pages/your-code-sucks-and-i-hate-you.html)

------
spicyusername
I think the spirit of these is applicable to any situation where multiple
parties need to come to consensus - i.e. try to focus on positive, respectful,
inclusive, and objective modes of communication.

------
raverbashing
I'll just say that if you think Linus' comments are bad, there are much more
(needlessly) rude people in LKML.

Linux might be incisive but he's often right and it's usually about an
impactful issue. Some other people are just gratuitously more rude and/or
obnoxious.

~~~
titzer
This is why it doesn't matter if you are right are not, if the attitude that
comes from the top is rude and obnoxious, then other "less right" people will
follow suit. The culture then degrades as people generally emulate the
behavior of elites. The example of behavior should be set at the top. Being
technically right (this time) is no license to train others how to be an ass.

~~~
Ididntdothis
In all fairness Linus often makes pretty detailed comments with a lot of
patience. He only blows up from time to time. There aren’t many tech leaders
who have the capacity to understand issues and then explain them.

------
fjfaase
I have noticed that when doing a code review, I ask a lot of why questions. I
want to understand why the coder has chosen for the given solution and if any
other solutions were taking into consideration. I also want to know if the
coder things that all use cases are covered. I find that I often happy with
the answers, when I realize that the coder thought about the solution.

Usually, there are multiple ways to implement something and that there is no
clear choice which is better. Trying to make into a black/white problem, is
not the best way to deal with the inherently grayness of software engineering.
Make a fuss about coding standards, for example, is often not productive.

------
docker_up
First thing before any code review is that your team/company needs to have a
coding standard. This is the first rule of engagement and eliminates most
problems. Without a coding standard, code reviews quickly devolve into crap.

Once you have a coding standard, then the reviewer needs to differentiate
between opinion and fact. If you don't like the variable name but it conforms
to the coding standard then it's not a valid review comment. The coding
standard should give some guidance as to what a variable name or function name
should be.

If there's a bug or if there are potential for issues then that's what I focus
on.

------
stunt
I found three different reasons for people doing a bad job at code review.

\- Not having the required soft skills to do it. \- Cultural differences.
Because, What is considered offensive, rude, or negative in many cultures,
might be just direct in some cultures. But I do believe cultural awareness is
also a soft skill. \- Having a bad mentor/peer/environment. Many junior
developers write reviews with the same style as they receive reviews.

~~~
pbhjpbhj
Comment review: you have to put a line space [extra blank line] in order for
HN not to collapse the text into a single line.

\- first

\- second

instead of

-first -second

[https://ibb.co/jLtZTFS](https://ibb.co/jLtZTFS) \- image shows editor pre-
submission ;o)

HN should use [limited] markdown or something IMO.

~~~
m463
I wish there was a way for a bullet list. Then again, simple text might mean
better minimalist communication.

------
kazinator
This article is complete garbage. :)

We absolutely must consider the non-technical aspects of a change, and it
behooves us to consider them before other aspects.

Changes are often driven by new requirements. If we approve the change, we are
approving the adoption of those requirements.

There isn't always a technical reason for such an adoption other than "someone
wants it that way".

Based on this article, I mustn't reject, for instance, GPL-ed code being added
to a BSD-licensed program. There is no _technical_ argument against it. We can
just switch everything to the GPL license and march forward; what's the
problem? Can we put religion aside and just discuss the code?

There also no technical reason why a compiler shouldn't have a --tetris option
for playing a little game on the text console. No wait, I didn't say anything
about text; we can just link in SDL and have it graphical! Anyway, can we just
discuss the best way to do this, and not emotional debates about whether or
not to do it?

So, "someone wants it that way"? Whooptee doo, so effin' what? Let them fork
the code.

------
Double_a_92
The most annoying thing is when the reviewer doesn't recognise the complexity
of some problem, and just insists on doing it in some "simple" way that
doesn't solve the whole problem.

"Oh you spent 2 weeks figuring this solution out? Well here's my 2 minutes of
naïve ideas..."

~~~
lolc
When the reviewer can't understand why the complexity is needed, I add a
comment explaining it. This is a very frequent response by me: "Yeah, it's not
obvious why the simple approach doesn't work. I've added a comment."

------
watwut
The most infuriating code rewiev was the one where I ended up with code that I
considered worst then my original one.

The second most infuriating one was series where I had to rewrite code because
rewiever did not knew syntax I used amd claimed it is difficult to read. Three
weeks later he was forcing me to use that same syntax - because he found some
blog where he learned it so it became necessary. Like what the hell.

------
thanatos519
I was expecting a way to receive "tips" (small cash payments) for reviewing
code ... :/

~~~
civilian
lol, I was expecting to receive advice I wouldn't like for reviewing code.

------
lmm
Most of the advice seems to amount to watering down one's criticism. If we do
that, won't the result be accepting less-good code into our projects? Is that
the outcome we want?

~~~
gnozzle
I didn't read any of these as watering down. Which ones do you think dilute
the message?

~~~
LanceH
Exactly. I seems like most of these are jumping off points for starting a
conversation.

I may be wrong but I read the article as a senior reviewing a junior's code,
in which case the big win is not just getting better code in now, but getting
a better developer. Starting the conversation instead of jumping all over them
is important.

Even when reviewing a peer, while more terse we tend to phrase things in
questions, "Shouldn't this be in a lib instead?" Frequently it's oversight on
their part, but sometimes they have their reasons and I learn something in
return.

