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 method Y?
  • 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"?

  • 2
    Have you tried mentioning this to that person? – Bryan Oakley Jun 15 '16 at 20:26
  • 9
    I had a superior that required full stops in all comments, as well as proper capitalization and punctuation and grammar. He was also very anal about whitespace. It drove me crazy, but it also led to very readable, consistent code. – Bryan Oakley Jun 15 '16 at 20:49
  • 4
    The first three bullets in your post are bike-shedding, unless they are part of a coding style shop standard. If you're writing documentation, I would expect perfect spelling and grammar. That's not hard to achieve nowadays, given the abundance of spell checkers and grammar checkers in word processors. Similarly, coding-style issues can often be automated in suitably-intelligent code editors. I wouldn't expect to see these sorts of things in a code review; everyone's time is too valuable. – Robert Harvey Jun 15 '16 at 22:30
  • 2
    the arrogant/elitist review is easy and require almost no effort. To make a technical review you have to read and understand the code and then think of better solution ... it means that you have to work. I'm not surprised by the outcome of your proposal. You should organize the review process with measurable and well defined tasks to achieve something. – JoulinRouge Jun 16 '16 at 7:54
  • 2
    If you're using Agile this is something you could bring up at retros without pointing out a single target. Mention that the main function of code reviews is the correctness of the code, and not the aesthetics of the code. In that case it becomes a 'need to change', and something you can continually bring up until it actually does change. – Canadian Coder Jun 16 '16 at 19:15
up vote 21 down vote accepted

Types of reviews

There is no one true way to do peer reviews. There are many ways in which to judge whether code is of a sufficiently high quality. Clearly there is the question of whether it's buggy, or whether it has solutions that don't scale or which are brittle. Issues of conformance to local standards and guidelines, while perhaps not as critical as some of the others, is also part of what contributes to high quality code.

Types of reviewers

Just as we have different criteria for judging software, the people doing the judging are also different. We all have our own skills and predilections. Some may think that adhering to local standards is highly important, just as others might be more concerned with memory usage, or code coverage of your tests, and so on. You want all of these types of reviews, because as a whole they will help you write better code.

A peer review is collaboration, not a game of tag

I'm not sure you have the right to tell them how to do their job. Unless you know otherwise with certainty, assume that this person is trying to contribute the way he or she sees fit. However, if you see room for improvement, or suspect maybe they don't understand what is expected in a peer review, talk to them.

The point of a peer review is to involve your peers. Involvement isn't throwing code over a wall and waiting for a response to be thrown back. Involvement is working together to make better code. Engage in a conversation with them.

Advice

Towards the end of your question you wrote:

how would I go about encouraging colleagues to actually look for faults in the code in balance with glaring aesthetic errors?

Again, the answer is communication. Perhaps you can ask them "hey, I appreciate you catching these mistakes. It would help me tremendously if you could also focus on some deeper issues such as whether I'm structuring my code properly. I know it takes time, but it would really help."

On a more pragmatic note, I personally divide code review comments into two camps and phrase them appropriately: things that must be fixed, and things that are more cosmetic. I would never prevent solid, working code from being checked in if there were too many blank lines at the end of a file. I will point it out, however, but I'll do so with something like "our guidelines say to have a single blank line at the end, and you have 20. It's not a show-stopper, but if you get a chance you might want to fix it".

Here's something else to consider: it may be a pet peeve of yours that they do such a shallow review of your code. It may very well be that a pet peeve of theirs is that you (or some other teammate who gets a similar review) are sloppy with respect to your own organization's coding standards, and this is how they have chosen to communicate that with you.

What to do after the review

And lastly, a bit of advice after the review: When committing code after a review, you might want to consider taking care of all the cosmetic things in one commit, and the functional changes in another. Mixing the two can make it hard to differentiate significant changes from insignificant ones. Make all of the cosmetic changes and then commit with a message like "cosmetic; no functional changes".

  • Thank you for an excellent answer. I guess my frustrations lie where bigger issues are not being addressed. Such as missing indexes on a DB table. Or attempting to use a methodology or algorithm without understanding the reasoning and as such doing so incorrectly. For me - these are more important and should be addressed and resolved primarily - with the aesthetics being a secondary concern. – Gravy Jun 15 '16 at 20:55
  • 1
    do you know bigger issues are being missed, or are you just afraid they will? When a big issue is missed, in a retrospective or team meeting you can bring up that these are the sorts of things that should be caught in code review. You might even consider asking who on the team focuses on database changes, and if no one raises their hand, maybe appoint some to try to focus only on db changes for the next review. – Bryan Oakley Jun 15 '16 at 21:00
  • To be honest - the majority of cosmetic comments are not generally targeted at my code. When I review other's code, I see comments against PRs relating to cosmetics, right next to code which I would consider a big issue. Further, a lot of the team's first language is not english. So from my perspective - the odd spelling / grammar error is forgivable. I am not here to review their use of english in a doc-block comment, but their code. – Gravy Jun 15 '16 at 21:04
  • 2
    Well, their comments are part of the source-code, and if they are unneccessarily hard to parse, misleading or even wrong, having them at all can be a disservice to anyone who later has occassion to look at that code for any reason. They need not be formal or works of art to achieve their goal, but broken english, irrespective of the writers proficiency in the language, impedes the goal. Better to have no comments than bad ones. – Deduplicator Jun 15 '16 at 21:44
  • 1
    Most people appreciate when errors in the language are pointed out to them so they can improve. – gnasher729 Jun 16 '16 at 6:44

People comment on code formatting and typos because they are easy to spot and don't require much effort from them.

This part is easy to fix - almost any language have a linter or style checker tool. Plugin it in you build process, so that it would fail the build if there is a redundant space. As a result there will be no style issues to comment on.

Getting them to find real problems might be quite a challenge. Not only this requires a special inquisitive and detail-oriented mindset, but also a significant motivation.

And this motivation comes from experience. Experience of trying to work with crappy code written by previous developers. Senior engineers have plenty of that. Swimming in the ocean of shit gives you quite a desire not to get there again.

If I need to pick one main thing to check during code review, it would be code maintainability. At all times, developer reviewing this code should understand it, and be ready to enhance and modify it. If he does not have a clue of what's going on here, he need to tell it right away, and the code need to be rewritten.

  • I agree with you on your comments. And if you see that ocean of shit written by me - then I would encourage you to suggest I rewrite. But if you ignore the shit but asked me to fix the cosmetic stuff - thats what gets me frustrated. – Gravy Jun 15 '16 at 22:13

Here comes the practical answer:

Scenario 1: You are the senior member and someone who can decide the practice

Call a meeting and lay out the Code Review rules and guidelines. Trust me, clear guidelines work better than any 'honour' system or training. Make it clear that code formatting issues must not be raised at all. Some members will object. Listen to them and then ask them to follow the guidelines for first few months as an experiment. Show willingness to change if the current guidelines do not work.

Scenario 2: You are not someone who decides the practice or you are a relatively junior member on the team

Your best bet is to just resolve the code review issues until you manage to reach scenario 1.

The simple answer to prevent "trivial" code review comments are to insist (for the sake of efficiency) that the reviewer should be the one to fix them. So if the reviewer finds that you've (horror!) missed a fullstop or spelt a comment wrongly, they should just fix it and move on.

In my experience this means that:

  • your reviewers stop making these relatively pointless review comments and passing them back to be fixed.
  • only the most OCD reviewers will fix them, meaning most of your reviews will pass. this has the knock-on effect of requiring devs to perform more substantive reviews, those that report trivia because they are not actually reviewing code will end up having to justify why all their reviews pass without comment.

Either way, this is a benefit. The cost of a failed review is high in terms of making a developer stop what he's working on and revisit his code, and the subsequent follow-up review. If a review finds real code quality or architectural issues then this cost is worth every penny, but paying this cost for trivia is not.

Review the Review Process

In addition to code reviews I would suggest to also review the Review Process on a regular basis. There is certainly a lot that people can learn here as well, e.g. how to give proper code reviews.

Usually some of the bike-shedders are only inexperienced and just don't know what to look for. They need a bit of guidance not only w/regard to their code, but also vis-á-vis doing helpful code reviews. Providing feedback about the reviews will lead to a learning process that (hopefull) results in better reviews, and better code.

Next, it might be a good idea to (loosely) formalize a set of values and criterions, what the organisation or team perceives as Good Code™, and what are anti-patterns that should be avoided. It's not about setting sth. in concrete, but about getting a common consensus about code quality from the beginning.

As someone who's been on both sides of this (reviewing code written by others, as well as having code I've written reviewed), I'd say that I have three categories any feedback falls into. Well, four, there's also the delightful case of "all is well".

"Would be nice, but it won't block you" (if all the feedback fall in this category, I may send off the merge request with a "will merge at XX:XX, unless you tell me you want to fix them", or "sure, go ahead to check in, whatever block the system would throw has been disabled"):

  • Forgetting full stops at end of sentences (in doc strings, or comments). Clumsy grammar in anything that isn't emitted to users in any way shape or form (again, doc strings and comments)
  • Code that has a more elegant way, but what's there is understandable and idiomatic (I'll probably even put my suggestion in, so it's easy to either leave or fix)

"Things that needs to be fixed, but I'll trust you to do that" (if all the things fall in this or the previous category, I'll respond with "fix these, I'll merge when you tell me you're done" (and at that point I'll probably quickly scan to see that nothing else popped up)):

  • Minor quibbles ("you're comparing a boolean to true, that's a bit paranoid...", ...)
  • Minor style violations, ("the style guide says X, what you've done is outside that; I would do Y or Z, depending on what way you want to go")
  • A few missing test cases, that should not be hard to write

And finally, "things that are problematic, I will need to review your code again once you've fixed these issues" (if there's any in this category, there will need to be another round of review, so put in a comment saying "there's also a couple of minor things, would be nice to see some of those fixed while you're at it" if there's anything from the first two categories present):

  • This would be things like "no, there really is a MUCH better way of writing that", "you are not computing what you expect, because your unit test misses these edge cases" and all other severe issues.

It seems some people have forgotten the most important question: What is it that you want to actually achieve with code reviews?

The purpose of code reviews is to get bug-free and maintainable code quicker. Code reviews achieve this in several ways: Developers write better code in the first place because they know it will be reviewed, knowledge is shared as part of the review process, bugs will be found because the reviewer is not blind to their own mistakes as developers can be.

If you see the review process as a chance to put down your colleagues, or to create work for them, then you are doing it wrong.

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.