Hacker News new | ask | show | jobs
by quantumhobbit 2864 days ago
I just left a job with a truly toxic code review culture.

- managers would swoop in and leave incorrect criticisms. Asserting plainly false things and in one case reviewing python code as though it was JavaScript.

- devs would make conflicting requirements for improvement. Dev A demands change A. Dev B wants change B. A and B are incompatible and neither dev will back down. Management of course refused to help settle the argument.

- change requests completely unrelated to the PR being reviewed would get crammed into the PR. So every small code change could balloon into weeks on refactoring work.

- Often times code review comments were made without ever reading the code being reviewed. Such as a comment of “Why aren’t you using library X?” sandwiched in between invocations of library X.

The culture was clearly rewarding people for shitting on others work instead of doing any work themselves. Getting even the simplest code changes past required endless meetings explaining the most basic facts about computers.

So glad to be gone from there.

5 comments

>devs would make conflicting requirements for improvement. Dev A demands change A. Dev B wants change B. A and B are incompatible and neither dev will back down. Management of course refused to help settle the argument.

This was a problem in my previous job. Options I thought of:

1. Get a moderator. This is what we did. He did not just settle disputes - he did more than that. But it's good to have at least this role.

2. Have a total of 4 reviewers review the disputed section. If there is a majority, change it. If it's a 2-2 split, just leave it as is. (Team did not accept this).

3. Disputed code (whether changed or not) should have comments saying this vs the alternative was discussed in a code review. Why? Because if there isn't consensus, over time, people are going to keep changing those lines of code back and forth (some developer who was not involved in the review will decide to refactor - rinse and repeat).

Some of these are really something else. Of some I have seen minor cases. "While you are changing this code, why don't you also..." is sometimes okay. Conflicting requests from reviewers, I have seen those happen due to neglect from the reviewer most in charge of the code. If they cared, they'd have used their technical competence, or rank if necessary, to set the direction.
Not sure if management is right for #2. There should be some kind of tech lead in place to resolve technical conflicts. Design by concensus doesn't work well.
Some of this stuff is bananas, and you're totally within your rights to say, "please read this PR; I am using this library / this is Python.". But if someone's asking me for a refactor, I say "let's make a separate ticket for that", and if multiple people ask me for conflicting things, it's totally fine to say "please figure this out and update this PR, or I'm ignoring both of you". I think you made a good call switching jobs, but sometimes a great job is worth dealing with some troublesome processes.
This has become the norm in the assembly-line culture of agile development? Last two gigs I heard about, this was baked into their process. Projects were delayed weeks or months; projects missed deadlines.