
Ask HN: How do you do code reviews without getting silly over little things? - nobody271
I&#x27;ve had good experiences with code reviews where someone corrected my logic, saw an easier way to do something I was doing, stopped me from being lazy, etc. I&#x27;ve also had a lot of dumb code reviews where I used a variable name that conflicted with one of ten conflicting standards or I did something in five lines instead of one because it was easier to read.<p>The problem I see is once code reviews are in place and you have a big enough team you get code review specialists who don&#x27;t add any value but create an awful lot of work. I&#x27;ve literally seen companies consider any change made in a code review to be a bug fixed so you get folks who pride themselves on flagging things in code review even though there is zero, and often negative, substance to the flag.<p>How do you get the good parts of a code review without the bad parts?
======
deanmoriarty
Do you work at my company? :)

I don't know how to solve it, but it's insane. Especially for teams with high
churning (typical in Silicon Valley where everybody constantly leaves for
better opportunities), the code review culture changes from month to month
depending on who's the latest "super picky specialist" on the team, with
terrible dips in productivity.

The managers are not too involved with the code review process themselves and
they pat themselves in the back just for seeing "very active and healthy code
reviews", as measured in terms of review comments.

The problem is that code that 100% functionally works, with proper tests and
good readability, gets blocked for weeks by __completely__ unproductive people
who don't do anything other than driving a personal agenda of demanding
abstract levels of refactorings. Everyone in software should have learned at
this point that your fancy abstractions don't provide any benefit, since they
won't solve your future problems anyway. Just make sure you refactor where you
would otherwise be facing physical duplication of code/logic, and you'll be
100% good.

I've been victim of this myself lately: I'm known for writing code that is as
simple as it can be, while maintaining functionality, scalability and correct
handling of corner cases: I don't do abstract interfaces for the sake of it, I
couldn't care less about factories, ...

Sadly, it's forcing me to likely look for opportunities out of my current
company.

------
msencenb
Nitpicky code standards should be the realm of automated sniffers and fixers.

Besides that there is no magic wand here. You have to do the work to create a
good culture of code reviews in your team. This is not science, this is your
engineering lead promoting good review practices and cracking down on harmful
ones.

------
slindsey
My experience with code reviews is that they always became adversarial. I
tried to implement code reviews in my group many times and it didn't provide a
lot of value.

We finally did it by using GitLab and requiring that every merge request to
master must be reviewed by two team members. This provided code review on
every piece of code instead of only on select pieces. GitLab allows us to
enforce it easily, but the process can be done anywhere.

The nit-picky code reviewer gets overwhelmed defending his point of view and
starts being a little more selective. The lazy programmer gets overwhelmed
with constant changes and actually starts paying more attention. Somewhere
along the way it has actually balanced out.

Discussions have allowed us to identify poor practices, new techniques, and
implement some standards.

~~~
nobody271
Where I saw it be successful it was a pretty good team actually. Like pretty
much everyone on the team was a reasonable person. The way they did it was
before you check into any branch (other than master) you need to get a code
review from one other person. So you'd go into the chat room, ask for the
review and either get a :shipit or an email/notification on github about
whatever was flagged. That worked pretty good but idk if the main reason it
worked is because it was a reasonable group of people.

------
brodouevencode
Great question and thank you for making the rest of us think about it because
it can become unnecessary or burdensome when used inappropriately.

I personally eschew the trivial or otherwise stupid company standards in favor
of more language/community set standards (think PEP8). When I do criticize I
will lead more with “what was your reasoning behind <this block of code>”
because you get a more honest response and sometimes you’ll get the author to
admit to “oh I was just being lazy there” or “do you have a better way of
writing that”. Code reviews are a necessary evil but how you approach them can
change the outcome.

------
seanwilson
> I've literally seen companies consider any change made in a code review to
> be a bug fixed so you get folks who pride themselves on flagging things in
> code review even though there is zero, and often negative, substance to the
> flag.

Do you not push back when this happens? You should be allowed to ignore some
review comments. I think marking comments with priorities help (e.g. must fix
vs could fix). When the reviewer knows you'll be reviewing their work as well
this can help stop them being unfair.

~~~
nobody271
Do I push back? If I care, yes. But I'm asking from the point of view of a
team, not an individual on the team.

How do you decide that something must be fixed?

~~~
seanwilson
Just some ideas:

\- Have a code review wiki document you can refer to for clear cut cases of
things you have to fix without debate (e.g. naming convention issues) and
things that are more subjective and don't have to be fixed (e.g. there can be
more than one return per function).

\- Code review suggestions should come with priority levels. Ignoring "nice to
fix" problems shouldn't be a big deal if they're too time consuming and have
little merit. This also helps draw attention to people that flag low merit
issues as high priority.

\- Use linter tools and autoformaters to stop people wasting time with certain
nitpicks.

\- Ongoing team discussions and training with concrete examples of which code
review nitpicks aren't good uses of time might help.

If you've got team members saying nitpicks are high priority and making no
considerations about how much time certain fixes are going to take then it's a
training and experience problem you're going to have to address.

Can you give some examples of code review feedback you're not finding useful?

