Ask HN: How to deal with refusal to change code during reviews?

28 points by skeptic2718 9 years ago | 32 comments

This particular programmer has 7-10 years of industry experience in various areas but lacks knowledge about this particular language/framework and doesn't have a lot of experience in day-to-day infrastructure operations.

He will usually dismiss 60-80% of the feedback as not being important, not having an impact, or will flat out say "I will keep the code as-is" without giving a reason.

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 "future issues" we'll face).

This isn't simply a junior programmer not knowing better. It's seems to be a mix of ignorance and arrogance. We worry he won't be around in an year or so, when things start to break.

bsvalley 9 years ago |

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".

dlubarov 9 years ago | |

IMO you shouldn't try to prove that his code is bad and the feedback is good. Even if his code is perfect and the feedback is bad, he should still respond to the feedback with a justification for his approach. If his way is really better, then he can educate his peers about it.

fnbr 9 years ago | |

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 9 years ago | | |

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").

tetek 9 years ago |

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.

StillBored 9 years ago | |

Its more than guidelines. Frankly the guidelines are arbitrary, and people's ability to find flaws and apply them just as arbitrary. What seems to happen quite frequently is that the guidelines just end up being used as expedient ways to say "I'm busy, go away" or "I don't like your patch, but can't find anything wrong with it" or any number of other non productive things. Reformatting code by hand is tedious, and every second typing "add a space here" in the code review subtracts from a second of understanding and analyzing the functionality of the code.

Which is why if you have coding guidelines they should be enforced by automated tools. Either the code passes the tool or it doesn't, no on is allowed to nitpick brace placement, spacing, or whatever. Use something like clang-format or whatnot and stick to it.

debacle 9 years ago | |

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

transitorykris 9 years ago | | |

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.

startupdiscuss 9 years ago | | |

I think part of the issue is that when we, innocent readers on Hacker News, read something like this, we only know part of the story.

So we can't tell which party is the problem, and what the issue is and we end up second guessing the person asking the question.

The advice invariably tends towards finding an objective standard so that the person asking the question can check that they aren't the problem person.

JamesBarney 9 years ago | | |

Training code reviews into a process does work VERY well in the long run. See Code Complete.

jjgreen 9 years ago |

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 9 years ago | |

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

euyyn 9 years ago | | |

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.

tedmiston 9 years ago | |

This is why it's helpful to have a consistent code style across the team, or at least across the codebase, so that personal opinions in either direction don't cause too much conflict in code review. Some companies value low quality but running code now over better quality code tomorrow.

galdosdi 9 years ago |

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 9 years ago |

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 9 years ago |

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 9 years ago |

'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 9 years ago |

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/

AnimalMuppet 9 years ago |

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 9 years ago | |

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 9 years ago |

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 9 years ago | |

"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 9 years ago |

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 9 years ago |

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 9 years ago |

~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 9 years ago |

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

atmosx 9 years ago |

> 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 9 years ago |

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

brangalinafoeva 9 years ago |

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