Thursday, March 31, 2016

Coaching and the PR Code Review

There is rarely a time when all the members of a team have a deep understanding of all the technologies being used on a project. On top of that the technologies that they're using are evolving and all team members need to keep up with the changes. Some members will learn new techniques and features earlier than the others.

An effective way of cross training team members is through the Pull Request (PR) Code Review.

Comments on a typical PR will be:
  • This looks like a mistake because...
  • This doesn't conform to our standard because... (This should rarely happen because a lint step should have caught this earlier. If it is happening then the lint rules should be reviewed.)
  • This works. I would do it this other way because...
  • Please add/modify a test for this.
In addition to that, as a reviewer, you should be asking:
  • I'm new to this technology, what does that do?
  • Please can you add a comment above this line explaining what this does?
  • I haven't seen this syntax before, is it the same as?
  • This is fantastic! I never knew you could... (Call it out when someone does something you haven't seen before or does something really well and you learned from it. Everyone loves positive feedback.)
If you submit a PR that has new features or techniques then consider immediately adding comments (in the PR) about them to help the reviewers if know that they will not understand what something does.

The question then arises, should I add a comment in the code or a comment in the PR? My take on this is that comments about how a language, framework or technology works, in isolation, should go in the PR comments. You don't want to clutter the code with information that can be found through a search on the web. The comments that go in the code are how the code works.

These are the advantages of learning through code reviews:
  1. The reviewer is learning in the context of the project domain. You don't get more real world than that. Not only will the reviewer be learning new syntax/constructs but they will also be understanding the business domain and gain knowledge in part of the code base that they may need to maintain in the future. Compare this to learning contrived examples in a class.
  2. The reviewer is learning a subset of the technology as it pertains to this domain. We would all love to learn all aspects of each technology we touch. Time constraints do not make this possible. Learning like this in situ provides the most time efficient way of learning the essentials. Again compare this to class learning where you may gain some super interesting knowledge and then not apply it.
  3. The coach, the person who submitted the PR, is forced to look at and explain their code. When you ask your seemingly naive question you may see a comment like "This construct in this language does... Now that I look at it again I see a problem/way-that-I-could-improve-it."
Using this technique you get a better code review on the PR and a great contextual training session at the same time.