> In fact, I’d argue that the code author, by virtue of their time and effort, deserves the chance to see their proposals in action. They’ve earned the opportunity to prove their detractors wrong.
That sounds so whiny.
Code review doesn't comment on the coder. It comments on the code. Other people who aren't the whiny coder know how well it matches the expectations of surrounding code, how the design will age, what a newcomer to the code next year will need to know to maintain it. They will point out missing test cases. No one is forcing the code author to even write code, that is their choice.
I had a teammate that would take problems away and work on them for a couple of weeks. Come back with a hairball, with obvious design issues like interfaces depending on concrete classes and byzantine relationships. Very tough to usefully feed back on. Go away for longer. After several months of that kind of progress, rather than review more I just rewrote the entire subsystem in 2 weeks. A dev spending time on a system is measuring their time, not their effectiveness.
That sounds simply wrong. Assuming the code author is part of the team (and if not, why have code reviews at all?), they also have to make sure that others' life is not made worse.
Merging code with performance regression, memory leaks, or crashes will affect everyone on the team, as they could not test their own changes, and/or will have to debug code they didn't write. So reviewers should not approve that.
Merging code which goes against project's conventions - be it bad design, or abstraction violations, or using languages or libraries which should not be used in project - will affect others' ability to fix author's code. Unless people want super-siloe'd system, where every file has a single owner and no one else can make sense of it, the reviews should not accept the code either.
The "Polishing code that’s in a [...] exploratory phase [...] is wasteful" argument only works if there is a clear definition of "exploratory code" - often it's a directory called "sandbox" or "experimental". The review rules should be clear that such directories have little rules, and when code is moved out it needs to be re-reviewed. If there is no such system in place, then all code is "production" because there will _never_ be a time to rewrite and polish it properly. (Especially in "agile" companies which make arguments like that).
Finally, "code author, by virtue of their time and effort, deserves the chance to see their proposals in action" is the most ridiculous argument. Shared code is a shared space, and one should be thinking of others. And I know that I can spend a lot of time and effort making a chair.. but trust me, it will be a horrible chair and you'll never want to sit on it, no matter how much time I wasted on it.
> In fact, I’d argue that the code author, by virtue of their time and effort, deserves the chance to see their proposals in action. They’ve earned the opportunity to prove their detractors wrong.
That sounds so whiny.
Code review doesn't comment on the coder. It comments on the code. Other people who aren't the whiny coder know how well it matches the expectations of surrounding code, how the design will age, what a newcomer to the code next year will need to know to maintain it. They will point out missing test cases. No one is forcing the code author to even write code, that is their choice.
I had a teammate that would take problems away and work on them for a couple of weeks. Come back with a hairball, with obvious design issues like interfaces depending on concrete classes and byzantine relationships. Very tough to usefully feed back on. Go away for longer. After several months of that kind of progress, rather than review more I just rewrote the entire subsystem in 2 weeks. A dev spending time on a system is measuring their time, not their effectiveness.
That sounds simply wrong. Assuming the code author is part of the team (and if not, why have code reviews at all?), they also have to make sure that others' life is not made worse.
Merging code with performance regression, memory leaks, or crashes will affect everyone on the team, as they could not test their own changes, and/or will have to debug code they didn't write. So reviewers should not approve that.
Merging code which goes against project's conventions - be it bad design, or abstraction violations, or using languages or libraries which should not be used in project - will affect others' ability to fix author's code. Unless people want super-siloe'd system, where every file has a single owner and no one else can make sense of it, the reviews should not accept the code either.
The "Polishing code that’s in a [...] exploratory phase [...] is wasteful" argument only works if there is a clear definition of "exploratory code" - often it's a directory called "sandbox" or "experimental". The review rules should be clear that such directories have little rules, and when code is moved out it needs to be re-reviewed. If there is no such system in place, then all code is "production" because there will _never_ be a time to rewrite and polish it properly. (Especially in "agile" companies which make arguments like that).
Finally, "code author, by virtue of their time and effort, deserves the chance to see their proposals in action" is the most ridiculous argument. Shared code is a shared space, and one should be thinking of others. And I know that I can spend a lot of time and effort making a chair.. but trust me, it will be a horrible chair and you'll never want to sit on it, no matter how much time I wasted on it.
[dead]