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.<p>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.<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 "future issues" we'll face).<p>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.
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.<p>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.<p>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".
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.
Reject it.<p>I've done it a few times, there was a fuss, there was a discussion, it got sorted out.<p>At the same time I've had a couple rejected (due to not adopting the reviewer's preferred code style). Same thing happened.
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.<p>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.
I have definitely worked with this type of programmer before.<p>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.<p>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.<p>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.<p>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.<p>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.
When you say "ignorance and arrogance", my gut suggestion is to pull the plug now.<p>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.
'We' is a bit vague here. Are you this guy's boss?<p>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.<p>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.
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?<p>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.<p>[1] <a href="https://www.industriallogic.com/blog/modern-agile/" rel="nofollow">https://www.industriallogic.com/blog/modern-agile/</a>
Fire him now, <i>before</i> he creates the minefield in your codebase.<p>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.
I've seen situations where someone with experience acted a certain way that sounded wrong - only to find out he was right all along.<p>It's impossible to judge this situation without clear examples of code.
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.
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.
~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.
> We worry he won't be around in an year or so, when things start to break.<p>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.