Tuesday, November 12, 2013

The Curse of the Toxic Peer Review

A peer review system is meant to do two things:
  1. disseminate knowledge and understanding of how a new module works
  2. help train the developer group as a whole on what the best practices are for the development team considering that best practices are expected to evolve over time.
It IS NOT meant to be a tool used by the authority-needy to impose their judgements for the day's set of finished code.  If one single person on a team of 3 or more does all of the pr's, then you are doing it wrong.  If the rest of the team feels that they need to run pr past a single person, you're doing it wrong.  For the code being pr'ed, if individual teammates are not able to independently come up with the same set of criticisms, you are doing it wrong.  If there are no clearly stated requirements and standards of good code, you are doing it wrong.  If your junior members do not seem to be understanding what is required of them in order to get their pr merged into the development branch without completely rewriting the code, you're doing it wrong.

There are also requirements for who can do pr's.  It is expected that those doing pr's know the code base they are working in and know what is expected from them when writing code in the particular language.  A developer who is uninformed and unable to demonstrate understanding as to what the current developer group standards are should not be doing pr's.
  • Why are setting up a peer review system?
    • Do you know what you're looking for when code is submitted for review?
      • it's been linted
      • it has tests
      • you can run the tests and see that they pass
      • the documentation is sufficient to understand how to use the code without asking the author for more details/information
      • you understand how the code works and can demonstrate it
If you find yourself in the situation where you are the main person all PR's go to or if you feel that your teammates are not doing what you expect from them when they do pr's, it is YOUR job to discuss the situation with them and get on the same page across the team.

Considering that newer developers may not feel they carry enough weight to address a toxic peer review situation where a senior developer is in charge of all PR's, it is the manager's job to ensure that proper peer review policy is carried out.  If not handled properly, a peer review situation is not just useless, it's actually harmful in that it glorifies and deifies one or two developers to the point where a company feels it is completely dependent upon those individuals and behaves as if they are the saviors for the company.  This is not a good situation for any company to be in considering how fickle many developers are and the likelihood of any good developer sticking around for more than a year after an ipo is rather low.

You are trying to create a good product, you are trying to create a successful company. You owe it to your teammates and the rest of company to do more than simply wish that you could clone a particular developer.  You owe it to them to set up a proper peer review system such that information and understanding is spread across all developers in a team as easily as possible.

No comments:

Post a Comment