The problem I have with most similar articles/comments is that the only two options for Code Review now seem to be:
- point out strict violations of the team's enshrined code style guidelines
- accept any and all code as-is, so long as there are no "bugs" or performance issues
Which, at that point, why are we even doing PRs? Unit test and lint...
When I receive a Code Review I want to know about the function that reduces my 10 lines to 1. I want someone to notice that my git mistake introduced a bunch of unneeded diff noise. I want to change variable names that are confusing to the first reviewer, because that means they'll be confusing to me when I read the code again next month.
Give me the development "human factors" in the code review. Some of that will be subjective and that's ok.
I know the pain - but its no use reducing it to a dichotomy (All or Nothing).
The strong reactions are due to pedants who block the source base doorway and pick at everything going by. It can sink a project. So folks push back, say things like "Only comment if its a real bug!" and so on.
There's lots of middle ground in a company source process. For instance, point out real bugs if present. But for anything else, comment and approve, respecting the contributing Engineer to do what they can in the time available.
Code review at least ensures that more than one person has seen and read the code. Hopefully they've at least kind of understood it, too. That's valuable in itself against the bus factor or "WTF is this?" surprises.
I think you might have to ask for that sort of feedback since people differ in how much criticism they can take. (Sometimes based on how busy they are.)
But the tips in this article work for subjective comments too. Rephrasing comments as questions is nearly always applicable.
- point out strict violations of the team's enshrined code style guidelines
- accept any and all code as-is, so long as there are no "bugs" or performance issues
Which, at that point, why are we even doing PRs? Unit test and lint...
When I receive a Code Review I want to know about the function that reduces my 10 lines to 1. I want someone to notice that my git mistake introduced a bunch of unneeded diff noise. I want to change variable names that are confusing to the first reviewer, because that means they'll be confusing to me when I read the code again next month.
Give me the development "human factors" in the code review. Some of that will be subjective and that's ok.