I wouldn't call myself a superstar dev, but a relatively experienced one. I try to keep code quality to a high level, and am always looking to make improvements to my coding style, try to make code efficient, readable and consistent as well as encouraging the team to follow a patterns & methodologies to ensure consistency. I also understand the necessity of balance between both quality and speed.
In order to achieve this, I have introduced to my team the concept of peer review. Two thumbs up in github pull-request for a merge. Great - but not in my opinion without hiccoughs.
I often see peer review comments from the same colleagues like -
- Would be good to add a space after
<INSERT SOMETHING HERE>
- Unwanted extra line between methods
- Full stop should be used at the end of comments in docblocks.
Now from my perspective - the reviewer is superficially looking at the code aesthetics - and is not really performing a code review. The cosmetic code review comes across to me as arrogant/elitist mentality. It lacks substance, but you can't really argue too much with it because the reviewer is technically correct. I would much rather see less of the above kinds of reviews, and more reviews as follows:
- You can reduce cyclomatic complexity by...
- Exit early and avoid if/else
- Abstract your DB query to a repository
- This logic doesn't really belong here
- Dont repeat yourself - abstract and reuse
- What would happen if
X
was passed as an argument to methodY
? - Where is the unit test for this?
I find that it is always the same kinds of people who give the cosmetic types of reviews, and the same types of people who in my opinion give the "Quality & Logic based" peer reviews.
What (if any) is the correct approach to peer review. And am I correct in being frustrated with the same people basically skimming over code looking for spelling errors & aesthetic defects rather than actual code defects?
If I am correct - how would I go about encouraging colleagues to actually look for faults in the code in balance with suggesting cosmetic touch-ups?
If I am incorrect - please enlighten me. Are there any rules of thumb for what actually constitutes a good code review? Have I missed the point of what code reviews are?
From my perspective - code review is about shared responsibility for the code. I wouldn't feel comfortable giving the thumbs-up to code without addressing/checking logic, readability and functionality. I also wouldn't bother blocking a merge for a solid piece of code if I noticed somebody had omitted a full stop in a doc-block.
When I review code, I spend maybe between 15-45minutes per 500 Loc. I can't imagine these shallow reviews taking longer than 10 minutes ever if that's the depth of review they are performing. Further, how much value is the thumb-up from the shallow reviewer? Surely this means that all thumbs are not of equal weight and there needs to be maybe a 2-pass review process. One thumb for deep reviews and a 2nd thumb for the "polishing"?