
Ask HN: How to deal with refusal to change code during reviews? - skeptic2718
This particular programmer has 7-10 years of industry experience in various areas but lacks knowledge about this particular language&#x2F;framework and doesn&#x27;t have a lot of experience in day-to-day infrastructure operations.<p>He will usually dismiss 60-80% of the feedback as not being important, not having an impact, or will flat out say &quot;I will keep the code as-is&quot; without giving a reason.<p>We want to be an welcoming place and let people learn on the job but we worry that our codebase is becoming a minefield (someone started keeping a document with &quot;future issues&quot; we&#x27;ll face).<p>This isn&#x27;t simply a junior programmer not knowing better. It&#x27;s seems to be a mix of ignorance and arrogance. We worry he won&#x27;t be around in an year or so, when things start to break.
======
bsvalley
It's not just about "you should code like that because that's how we code
here" type of problems. That makes you, the arrogant person in the story. You
should create a list of bugs generated by his code. If the list is significant
AND if you believe these bugs could be avoided by incorporating %100 of your
code review feedback, then setup a 1-1 quality meeting with him and go over
the list. Tell him how this could be avoided in the future and that the goal
is to stay away from bugs and to focus on fun things. Don't use "you should",
use "we should", act like it's a team effort.

If he doesn't accept your invite, or doesn't want to cooperate during this
meeting, resend an invite and include your manager and QA and all the other
engineers. Update your list of bugs and add bugs from the whole team not only
his bugs. Then make it a real team exercise. The problem will raise during
this meeting and everyone will clearly see that this dude is a bug-generator.

It is a long process, but that's how you do it professionally. What do you
earn from this big effort? People will thank you for taking initiatives and to
raise the bar in terms of quality. In other words, you'll get a bonus ;) He
will regret not listening to you when he had a chance to fix his mess
"secretly".

~~~
fnbr
Also, "you should code like that because that's how we code here" isn't a good
reason. If you don't have a good rationale for the changes, I would consider
not making them.

~~~
wallstprog
On the contrary, that is a very good reason, and sometimes the only one that
matters.

In any group of "n" developers there are likely to be "n+m" different ways to
do the same thing. This increases the "cognitive load" to understand others'
code (a fancy way of saying it makes the code hard to understand).

Personal preference is not a valid reason to eschew standards, even
(especially) de-facto ones.

If there is a "better" way to accomplish a particular task, then the entire
team should adopt it (and go back and change existing code to conform to the
"better" approach). If that's not worth doing, then leave well-enough alone.
(Another way of saying "If it ain't broke, don't fix it").

~~~
mbrameld
Right, but simply saying, "that's the way we do it here" doesn't communicate
all that. Hence, "that's the way we do it here" is not a good reason. "That's
the way we do it here, and here is why we do it like that..." is how it should
be phrased.

~~~
wallstprog
Agreed -- if you can't explain why "that's the way we do it here", then that's
a problem.

------
tetek
Write a clear guideline, and ask team to follow it. During the code review,
you only ask to stick to guidelines (that everyone agreed on). This way
programmers won't feel like your subjective opinion is affecting their work.

~~~
debacle
This is solving a people problem with a process. That doesn't work very well,
in the long run.

~~~
transitorykris
This can be done without it being a burdensome process or weaponized against
an individual. Developing a team agreement with the team can be a powerful
tool. It's important the team itself is the author, that everyone either
agrees with a proposal or will go along with the rest of the team (if they
won't, then its on them to form a counter-proposal), and that when the
environment changes the team can revise it.

------
jjgreen
Reject it.

I've done it a few times, there was a fuss, there was a discussion, it got
sorted out.

At the same time I've had a couple rejected (due to not adopting the
reviewer's preferred code style). Same thing happened.

~~~
wichcraft
I agree. I would flat out say "I won't accept this change".

~~~
euyyn
Agreed too; that's how it is at Google. Code reviews aren't "hey reviewer,
take a look at this code so you understand it and rubber-stamp it".

As a reviewer you're the gatekeeper, and no code is submitted unless the
author and all reviewers are happy. Happy as in, if you approved the change
and the author leaves tomorrow, you're the maintainer of that code. No one
should be rude in the review, but the understanding needs to be that the power
lies on the reviewer to hold their approval indefinitely until they're happy
with the code. That's a cost for the author and costs almost nothing to the
reviewer, so as an author you learn very fast to not waste time discussing
whether something "is not important enough": You apply all suggestions you
don't disagree with, and explain your disagreement about those that really
matter.

------
galdosdi
It depends on how much skin in the game you have as a reviewer. If you have to
or will have to maintain the same codebase (sounds like in your case you do),
then you have every moral right to reject the code and refuse to ship it. If
you're literally not allowed to reject the code, escalate to your common
manager. If it goes wrong later, at least you're on record objecting, and this
could also be a signal to get a new job if your manager can't even handle
this.

And if you don't have skin in the game (ie, process just requires "someone" to
review it even if not on the same team), then whatever, give them the advice
and let them burn themselves if they wish.

------
tedmiston
I have definitely worked with this type of programmer before.

One thing to can do is quantify a code quality metric, for example, test
coverage percentage. Then, get the team to agree that all feature branches
must maintain or improve that metric to be merged. Maybe treat hot fixes
differently depending on the situation.

However, this doesn't necessarily solve the issue if, for example, the
implementation is just overly complex, not well designed, or not well
decomposed for the problem being solved. There are code quality services that
address some of these, but perhaps we need a little more detail on the
specifics of your situation.

Unfortunately sometimes I've just had to merge code that's lower than ideal
quality, and then hold the engineer who wrote it responsible for fixing it
when it breaks.

Most engineers are smart enough to admit when they made a bad assumption or a
better solution becomes more obvious in retrospect. Sometimes people just
learn at different paces or can be afraid of trying new paradigms inconsistent
with how they've been thinking for a while.

As long as they learn eventually, your team is improving, but if they're not
interested in learning better solutions, then maybe there are two different
cultures in mind. Some devs value shipping faster over writing better code,
and having your team on the same page is helpful.

------
jowiar
When you say "ignorance and arrogance", my gut suggestion is to pull the plug
now.

But one important note regarding ignorance: Is the incentive structure at your
workplace such that programmers have the freedom to learn on company time? Or
will they be dinged in a performance review because they haven't shipped
features. If you value "getting it done right", you need to communicate that
it's not going to come at your employees personal expense.

------
rwallace
'We' is a bit vague here. Are you this guy's boss?

If not, you shouldn't start a feud with a peer. It doesn't do anyone any
favors. Get on with your job and let everyone else get on with theirs.

If so, decide whether the issues in question are really that important. If
they aren't, cross them off the list of rules to follow. If they are, order
him to do things the company way. If he refuses, fire him for insubordination.

------
transitorykris
It's hard to understand what someone is really thinking and feeling. In the
past I've found a lot of issues like can be traced back to safety, consider
making safety a prerequisite [1]. Have you tried pair programming?
Prioritizing the resolution of bugs over new feature work?

Try proposing some small experiments for the team to try over the course of a
couple weeks that can pull your coworker into the team.

[1] [https://www.industriallogic.com/blog/modern-
agile/](https://www.industriallogic.com/blog/modern-agile/)

------
AnimalMuppet
Fire him now, _before_ he creates the minefield in your codebase.

I mean, try to fix him first. But if he refuses to be fixed, fire him. He'll
be toxic to the codebase, and he'll be toxic to the culture.

~~~
brangalinafoeva
True dat. He could easily become toxic. Give him a chance, but if he's the
type that rejects reason (they are around), just let him go for damage
limitation.

------
zepolen
I've seen situations where someone with experience acted a certain way that
sounded wrong - only to find out he was right all along.

It's impossible to judge this situation without clear examples of code.

~~~
euyyn
"This isn't important", "this doesn't have an impact" and "I won't change it"
without giving a reason are always wrong.

But the code review policy in OP's company looks weak: It's the onus of the
author to convince the reviewer, not the other way around.

A good version of the first two responses could look like: "That would have a
small impact because of X and Y, and it doesn't seem to me that the extra
complexity in the code would be worth it". If the reviewer isn't convinced and
you still don't agree, propose to open an issue to measure the actual impact,
so the current change isn't blocked.

------
camhenlin
I like to work in refactoring during code reviews. How much longer does it
take you to show the person how the code should actually look (i.e., rewrite
it for them on the fly during the review?) than it does to point out every
little thing that they need to change? Probably not much. Then the changes
that need to be made are already done and nobody can ignore them. This will
also allow you to reason about the changes while you type them up.

------
I_am_neo
Does this person work alone, while also being responsible for the changes that
are bureaucratically and technically important to the 'system'? That shouldn't
have happened, never leave us to ourselves for extended periods. Next time
place a pair in that position of power because when you only answer to
yourself there are no opposing views.

------
throwme_1980
~This is why teams fails, i have posted something similar few weeks ago,
unless he has extensive domain knowledge or your boss wants this guy to be
there for some weird reason, i'd say get rid now, everyone is replaceable.

------
NonEUCitizen
Are you sure it's not a case of junior programmers writing up useless feedback
and dismissing an experienced programmer's judgement?

------
atmosx
> We worry he won't be around in an year or so, when things start to break.

I don't understand what this has to do with anything. It's more like you want
someone to blame when (and if) shit hits the fan. I get that office politics
are equally important and we have to learn to successfully navigate through
them, but seriously it is NOT cool to think and act like this... If you don't
like the PR/MR don't merge it, stand your ground for the greater good.

------
LeicaLatte
Accept his PR. Now fix the code yourself with all your review comments in a
new PR. Test. Commit. Checkmate.

------
brangalinafoeva
Some people you just can't coach. Let them go if they aren't team players.

