It depends on both the person and the environment.
In an environment that places stress on those who underperform, anything that calls attention to performance problems will be hated. Anyone that points out those problems will be disliked. Even the best people will fall to this stress.
In a more relaxed environment that isn't constantly counting points towards raises and promotions, people can relax and not worry about the problems that are raised. However, not everyone can manage this. Some people have just had too many jobs of the previous type, or other experiences like that. They just can't adjust.
IMO, it also means they have a much harder time fixing their own problems because they can't tolerate other people pointing out those problems.
As for code reviews, I always call it like I see it. It's my job to make sure the code will work and be secure. I'm not going to let things slip by that violate that, no matter how much I want that person to be happy. And I'll still point out matters of style as well, though I usually won't insist they be corrected, unless the person has repeatedly had the same problem. (My people are generally great, though, and will fix even my suggestions. So it's generally not a thing.)
You'd probably hate that last bit of my code reviews. If we were on a team, my advice would be to simply discuss that part with me in real time. We could decide together if it was worth the effort to make those changes. Async communications like PRs and emails simply aren't fit for that kind of discussion.
> In an environment that places stress on those who underperform, anything that calls attention to performance problems will be hated. Anyone that points out those problems will be disliked. Even the best people will fall to this stress.
Seems to me that this might be the problem rather than the code review itself!
> We could decide together if it was worth the effort to make those changes. Async communications like PRs and emails simply aren't fit for that kind of discussion.
Indeed. I've definitely found myself falling into a pattern of failing the review for serious issues with comments on the PR, but just approving the review and slacking the comments for small issues.
This is probably my biggest beef with the "culture" of software development nowadays.
Back in my day (hold on while I go grab my cane...) software developers would just flap their lips and sound would come out at each other.
Nowadays there seems to be this underlying assumption that actually talking to each other is a waste of time, that it should all be documented with no curation, and should always be in PR's, et al.
I'm all for documenting decisions, but I feel like it's not too much effort to swing by someone's office and have a discussion with them. At that point, maybe you don't even need to document if the updates are small issues. Is it really so all-encompassingly important?
But maybe I'm just old. Most of the time I'd rather talk to a human when I call into tech support too (unless it's something obviously automated like making a payment).
> I feel like it's not too much effort to swing by someone's office and have a discussion with them
Well there is a modern trend that's supposed to help with this too: open plan offices. No need to swing by their office, they're already right there (this has indeed been my working environment for most of my career).
It's all a bit different at the moment of course, but my team regularly video calls each other to discuss things that would take too long over text.
It also depends if everyone else pointed out 2 things and you pointed out 10. Letting other people go first means you only have to point out 5, which is still lopsided but not 'holy shit, man' lopsided.
>>> In an environment that places stress on those who underperform, anything that calls attention to performance problems will be hated.
This is so missing the point of development.
What do developers do? They write code and they ship code. (we could say that they deliver features or business value rather than code but that's not the point).
Code review is effectively preventing them to work. A bunch of comments on a PR, with no approval in sight, is very effectively blocking any progress on their work and making their project late.
I can't tell if the previous comment was ironic, but it seems to me that developers seeing things that way is a sign of immaturity or unprofessionalism.
Why must developers write and ship code? To deliver a certain goal / feature / business value. If all they're willing to do is writing and shipping code without considering how will that software work in the future, what problems it might have, how hard it will be to make changes to it, they're not being responsible enough about their work.
Software in general (except for throwaway code) is not supposed to work only NOW — it is expected to work for a long time, with an acceptable degree of quality, and with changing requirements.
If, to build a wall, it takes longer than just laying some bricks, it is because a solid wall serves many more purposes than some layers of bricks.
What does the business really want? If it is more or less worming code, or perfect code? Or somewhere in the vast continum between?
Code review is one tool in a large set to get closer to perfect. However if the business doesn't care to get there why are you doing it at all?
Of course if you care at all about quality code review is one of the better bang for buck tools and so maybe you need to change your culture around peer correction so it can be used. The choice is your companies.
Code review is not about perfection – it is about peer review, much like in science, recognizing that a single person might have overlooked a lot of important stuff, and recognizing that collaboration usually leads to higher quality work.
Of course the desired level of quality will be different at each company/team, and each team should adjust for its own reality.
In the same team there can be times for throwaway code with low standards, and times for high quality code with strict standards. It is part of the developers' responsibility to tell them apart and make contextualised choices.
In practice code review allows 1 or 2 people on the team to insist on their preferences throughout the project.
I once saw a 3-4 line fix turn into a rewrite of an entire page that took several full-time days to finish because the developer didn't like the original fix. I've seen a former DBA turned software dev insist on using nulls throughout the codebase rather than empty strings and ignore the argument that's not considered good practice as a software developer because they themselves are "now software developers".
And the rebuttal is always the same, "well you're just doing it wrong". Maybe, but it gets done wrong more often than right.
I'm not against code review per se, but the number of times I've seen silly and arbitrary changes because of one developer's sensibilities is way too high.
If you don't care about people overlooking things and otherwise turning in low quality work, then don't do code reviews.
In short your very words prove that you are wrong and code review is about getting to perfection. (along with types, formal proofs, tests, static analysis, and a few dozen other tools that are often used).
We've had a few people who thought their job was to close tickets. They'd write some code that fixed the problem, ignore all other problems (including ones they created while fixing the ticket) and call it done.
These people didn't last because that wasn't their job. Their job was to improve the product, and tickets were a way to explicitly telling them what their priorities are. Instead, they ended up creating more problems than they solved. Some of those problems we're still dealing with to this day.
Coding is something they do to improve the product, but it's not the only thing. They were expected to report other bugs they found, or just fix them, depending on the situation. And I'm not even talking "write up a full bug report with repro steps." Just a comment in the ticket would have been fine.
Code reviews are the least time-consuming thing that we do, and even if they took a whole day, they can start working on a new ticket until then.
The conflict I was talking about before is the difference between the goals of the company and the goals of the employee. The company wants a better product, in line with their vision for their product. The employee wants money and to advance their career. These aren't fundamentally incompatible, but some people's attitudes towards their career actually end up hampering their own goals.
The idea that "coders just code" is one of those ideas. That might work for junior developers, but mid and senior developers need a lot more skills.
In an environment that places stress on those who underperform, anything that calls attention to performance problems will be hated. Anyone that points out those problems will be disliked. Even the best people will fall to this stress.
In a more relaxed environment that isn't constantly counting points towards raises and promotions, people can relax and not worry about the problems that are raised. However, not everyone can manage this. Some people have just had too many jobs of the previous type, or other experiences like that. They just can't adjust.
IMO, it also means they have a much harder time fixing their own problems because they can't tolerate other people pointing out those problems.
As for code reviews, I always call it like I see it. It's my job to make sure the code will work and be secure. I'm not going to let things slip by that violate that, no matter how much I want that person to be happy. And I'll still point out matters of style as well, though I usually won't insist they be corrected, unless the person has repeatedly had the same problem. (My people are generally great, though, and will fix even my suggestions. So it's generally not a thing.)
You'd probably hate that last bit of my code reviews. If we were on a team, my advice would be to simply discuss that part with me in real time. We could decide together if it was worth the effort to make those changes. Async communications like PRs and emails simply aren't fit for that kind of discussion.