Monday, November 6, 2017

Code reviews in a Scrum team

Every team usually consists of people from different background and company cultures and with varying experience when it comes to code reviews. As a team lead I find that it is valuable to re-inforce the team with a set of guidelines to be followed in-order to ensure that everyone understands its value and is on the same page. This is a copy of the code review guidelines that I documented recently for my team.

Purpose of code reviews

  • Code quality - This is ensured by having an expert (inside or outside the team) review the code to help the developer with finding any obvious problems or missing gaps in the code
  • Keeping the developers up-to-date and ensure teamwork - It is good to have more than one developer in the team across any code change, so that as an agile team the skills match and so the team gets better at estimation. This also helps with equipping other team members provide support for the same change in the absence of the author of the code.

Common guidelines to everyone involved

  • Adopt a positive attitude - reviewers should avoid negative criticism and submitters should trust the reviewer and have open and healthy two-way conversations
  • Go by the checklist of standards (Refer to the coding standards being followed by the company - if there is no such documentation, you should work on that first).
  • Do not go overboard with attempting a 100% unit test coverage as we also have other testing mechanisms in-place before the code hits production - although ensure that the common happy-path and few negative scenarios are covered. Also do not request for unit tests for getter-setter routines in isolation as there is very little to no value in it - this should be covered as part of the unit test of the caller routine in most cases.
  • "Best code is no code at all" - this is a famous principle which is self-explanatory. The diff of the new changes should be kept as a minimum by removing unnecessary code changes not relevant to the given requirements.

Guidelines to the Submitter

  • Test your code before assigning for reviews and add unit tests wherever applicable - note that for the big dev tasks you may choose to test most of the scenarios (definitely happy path and most common negative paths as well) but leave the rest of the testing to be done while the code is in review in parallel to have early feedback. Code reviewer's responsibility is not for screening for any defects before QA testing. The code reviewer normally looks for coding standards and the overall architectural approach and so might spot some bugs in the process - but do not rely on the code reviews to have the bugs spotted. (Note that this principle varies from company to company and so you may have to suit your team's needs. In my team, we have unit testing framework, integration testing, then QA testing which covers functional requirements and finally regression and staging testing before something hits production)
  • Provide enough context to the reviewers - give useful comments, well named variables etc. For massive code changes with 300-500 lines of code, ask if the reviewer would like a code walkthrough session to speed up the process (prepare to note the reviewer's spot-comments during the session)
  • Assume best intentions from the reviewer - the code reviewer is expected to take ownership of the code he/she approves and is only trying to help you with the quality of your solution and code. So do not take the criticism on your code personally if alternate approaches are suggested; Also have face to face healthy conversations to close out on any comment that needs additional clarification.
  • Respect the code reviewers' time too - if you are running late with the dev work and provide the code for review just before the deadline, it is not justice to the reviewer expecting them to stop whatever they are doing and then do a quick and less thorough review of your code. If you already know that you are running late due to a certain bug or any specific issue, request for "early" reviews from the reviewers so that they can get comfortable with the code and raise any red flags early (so you also get enough time to work through them) and you may get the bug solved as a bonus too. Alternatively, do pair programming.
  • Review your code quickly before submitting - or at least do 'spot checks' to avoid making silly mistakes while submitting the code like typos in the comments or log lines or badly named variables or code formatting etc.

Guidelines to the code reviewers

  • Provide constructive criticism - avoid phrases like "you" or "your" when referring to the code as it is equivalent of making personal statements. Refer to the code in the comments and not the author.
  • Separate nitpicks and blockers. A way of understanding this distinction is by asking yourself: “Will the current version of this code cause significant negative consequences if merged?”. It the answer is “No”, then the comment is probably a nitpick and not a blocker. If a pull request is reviewed and only nitpicks are found, then the pull request should be approved and left to the developer/submitter to address them optionally.
  • Do not add blocking comments for code refactoring work not authored by the developer/submitter, fixes to old code, or missing implementation outside the scope of the submission. If new bugs or missing features are found, then call out to the developer to add new tasks. Reviewers should only hold submitters accountable for what their pull requests aim to do and implement, and nothing else.
  • If the submitter does not agree on something, have face to face conversations on the subject with the submitter and any other reviewers of the task. If you are the sole reviewer and if you are unable to reach any consensus (which is very rare if you are having a healthy conversation on the subject although possible), respect the opinion of the submitter although point out that you are only helping the submitter and it is his/her responsibility to have a proper solution to the problem.
  • Do not block someone else's review for more than a day - this gives less time for addressing the feedback comments and so causes disruption to the developer
  • Review unit tests as well - optionally start the review with the unit tests (TDD approach) - focus on the inputs and outputs to see if most important scenarios are covered.
  • If a code review is handed over at the last minute and you are unable to attend to it, raise it with the team so that someone else can pick up - if there is no one else to pick it up, raise it with the scrum master so that everyone in the team can be gathered and a decision can be made instead of rushing through the code review process.
  • For huge dev tasks with more than 300-500 lines of code change, request the developer to give a quick code walkthrough to speed up the review process rather than loosing attention due to the overwhelming code quantity. Code reviews should never take more than a couple of hours - the effectiveness of the review subsides with time and so there is no point of having healthy reviews just to approve them anyway without much attention given
If you have any more ideas on this subject, please drop a comment.

References

https://nyu-cds.github.io/effective-code-reviews/02-best-practices/
https://www.ibm.com/developerworks/rational/library/11-proven-practices-for-peer-review/
https://stackoverflow.com/questions/6197370/should-unit-tests-be-written-for-getter-and-setters/39491583#39491583
https://en.wikipedia.org/wiki/Pair_programming

No comments:

Post a Comment