4 ms·
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' lif
by theamk 2y ago
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.