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.
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".
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").
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
It's impossible to judge this situation without clear examples of code.
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.
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.
