9

After we spent 4 years cleaning up the code and it is working OK again, my colleague started to get messy again. Ignoring Reviews with the usual phrase "I have more important tasks to do atm" means that reviews are only for show. An unused variable, so what? Copy/Paste, perfectly OK practice!

I'm responsible for code reviews in the team and while his code works it is messy and I fear we may be throwing away the last 4 years of cleanup efforts and ending up right back where we started.

How can I get this coworker to understand the importance of what I am trying to enforce or at the very least how can I ensure compliance?

|improve this question|||||
  • 9
    Who manages the adherence to your standards (i.e. if somebody is not keeping to the standard - who is responsible for getting them back on track)? And are you peers, or is this colleague junior to you (inc. are you their manager)? Also, are these standards well-defined and enforced, or are they just general "good practices" your department has informally tried to keep until now? – Bilkokuya Oct 2 '18 at 15:07
  • How do you want this to work out? Are you wanting this guy to conform to coding standards? Does the messy code keep breaking the builds and prevent you from working? How does everyone else feel about this? – user44108 Oct 2 '18 at 15:15
  • 2
    I am responsible for code rewievs... BUT he just ignores the dev process and commits on Master or merges it self... And im tired of playing cop after 2 years because of unwilingness to change. His point of view is that the code is already Garbage so what change does my garbage make? – Sangoku Oct 3 '18 at 8:34
  • 2
    @Sangoku the fact that he commits directly on master or merges his own pull requests is an important detail. You should put that in the question. – jcm Oct 4 '18 at 10:13
  • 2
    Some repository applications have a feature where you can protect Master so it can only be written via a pull request. Then, at the very least, this coworker will have to create a pull request to write to master, even if he just self-approves it. If this becomes a problem, you might also be able to set certain permissions so that only certain users (i.e. yourself) are allowed to merge by pull request to cut him off completely. I would do this in stages though, not all at once, because this is kind of the "nuclear option" as it were. – Ertai87 Oct 5 '18 at 20:12
10

Depends on how much authority you have in the company. In general, the way the code review process is supposed to work is that the code reviewer is supposed to be a "gatekeeper"; if the review isn't approved then the code doesn't get pushed to production. Even if you are a lower job title than your coworker, as the reviewer, you (are supposed to) have the power. If he says "no that's dumb I'm not going to fix my code", then say back to him "no, THAT'S dumb, I'm just going to block all your pull requests until you do what I say" (obviously not in those words, you can be more diplomatic about it, but that's the point to get across).

That said, it's possible that this coworker will just stop sending you code reviews and start sending them to someone he sees as less "nitpicky", as it were. In which case it's your job to make sure everyone on your team is as nitpicky as you are, and you should treat that as a challenge. Presumably, after 4 years of refactoring, nobody wants to do that again; explain to your boss and to your coworkers that in order to make sure that this doesn't happen again, there need to be tight code standards and everyone, including this other coworker, needs to follow them. Then, make it your boss's problem, and present it to him this way (again, diplomatically): Either he can back you up and make your coworker write good code, or he can spend another 4 years refactoring. He'll know the right choice to make.

|improve this answer|||||
3

You should put into place a system that enforces adherence to the coding standards.

There are tools that will check your code against your rules and block any commits that don't meet the standard.
In our development environment we use SonarQube, you can find plenty of alternatives if you look around.

If your code fails the configured quality gates then you can't commit.

This way it's not you being picky its the system. It also means code reviews can concentrate more on the functionality instead of looking for standards infringements.

|improve this answer|||||
1

Messy, unreadable code is a form of technical debt that only gets worse with time. You can do style linting and put in hard blocks to prevent code that fails linting from getting merged. But given you've already talked to your colleague and he is ignoring code reviews, the situation seems better handled by the tech lead or engineering manager.

|improve this answer|||||
  • I will just forward my problem to the Manager, we are a small group of 4 developers. There is no seniority, im longer here but because im lacking in the native language he is the lead on paper. But i do the design choices.... that is where the conflict arose... – Sangoku Oct 3 '18 at 8:32
  • Actually, I can’t ignore code reviews - because I cannot merge my changes into the main branch. Nobody can where I work. Only the reviewer can. – gnasher729 Oct 3 '18 at 15:28
  • That is what we tried to enforce.. but he just commits on the master when he knows it would take to long to review... the problem is the mentality not the tehnical solution. – Sangoku Oct 4 '18 at 7:25
  • 3
    @Sangoku In my product group, no one can commit to master, not even admins. All new code needs to be in a pull request, reviewed and tested before it's merged. Someone needs to pull this guy's commit privileges. – jcmack Oct 4 '18 at 13:57
  • 1
    @jcmack that would be the manager – Kilisi Oct 7 '18 at 1:27
0

This is less an issue with a particular person as it is an issue with programming in groups. If people don't have to, there will always be accidental slips and purposeful laziness on the part of some developers. Not to mention some group dynamics that can lead to certain team members being obnoxious with their sloppy code. There's always someone like this eventually.

To address things in the short-term I would advise taking this person aside and telling them privately and diplomatically that their code needs to be up to standard in order to pass code reviews. If they continue to fail at this you may need to speak with their direct supervisor about the wrench this is throwing in your process.

Since you are responsible for code reviews, I'm assuming you have a little room to implement changes to the review process. As jcmack mentioned, my current web-dev team also uses lint and yarn to great effect, ensuring coding conventions are followed not with your annoying coworkers complaining about indentation, but a program automatically rejecting things that don't fit into your defined ruleset. These tools make it mostly impossible for a branch to be merged if it doesn't pass the checks. This is obviously a more long-term and involved solution, but it has been very nice and saved a LOT of time not having to point out every trivial flaw that a computer could find much more easily. So you could likely sell it as beneficial to management and look like a superstar if you implemented it.

|improve this answer|||||
  • I wouldn’t call it laziness because the colleague seems to be quite willing to work. However, it may be an attitude where one worker wants to do only the things that are highly visible to management (new features) and doesn’t want to do less visible things (like cleaning up his mess, implementing things to not interfere with others). – gnasher729 Oct 4 '18 at 11:36

Not the answer you're looking for? Browse other questions tagged or ask your own question.