Reviews are an essential step of the development process. They are also the best way to improve a team's skills and quality of the code. Every review and comment should be treated as a growth opportunity.
Approval & Approvers
- Merge requests need to have two format approvals from any of the people with the right to do so in the project.
- Approval right is earned through active reviewing on own team & other teams MR's
- Approval right can be lost
- Feature -> Master MR's that already had it's content review through task -> feature MR's don't need actual review/approval but that's not something we can configure in gitlab so approvals should be given as soon as such a MR appears.
Code Review Best Practices Summarise below are some of the best practices agreed upon by many in the software development community that should be respected.
Finding defects in code reviews is a good thing The point of software code review is to eliminate as many defects as possible, regardless of who "caused" the error. Reviewers should be constructive and provide examples when necessary, while authors should take every comment as an opportunity for professional growth and better overall product quality. There are obviously many ways to solve each problems, the goal isn't for the reviewer to impose his style but rather for the code to be of the best possible quality and for best practices to be shared.
Review fewer than 200–400 LOC (lines of code) at a time Anything higher than that will result in lower defect detection rate. This means more regular merge request / code reviews since adequately sized reviews are usually done rather fast. This also means quicker feedback loops within the development teams, usually leading to less code rewrite.
Aim for an inspection rate of fewer than 300–500 LOC per hour Going faster means finding less defect. Going really fast through reviews basically means the reviewer isn't really looking at the code.
Do not review for more than 60-90 minutes at a time When people engage in any activity requiring concentrated effort over a period of time, performance starts dropping off after about 60 minutes.
Authors should annotate the code before requesting the review Annotating is a process where an author takes some time to put something in the code review description, and adds some comments for to guide his reviewers through his code. This has the added benefit of forcing an author to review his own work and pre-emptively address defects.
Fast reviews While a reviewer should take its time to review, he should start promptly since the author is waiting on his feedback to continue his work. Code under review is always closer to delivering value than any other.
References
- https://www.ibm.com/developerworks/rational/library/11-proven-practices-for-peer-review/
- https://smartbear.com/learn/code-review/best-practices-for-peer-code-review/
- https://smartbear.com/learn/code-review/what-is-code-review/
- https://gist.github.com/olhza/88cd30bd75bf6ac3f64a
- http://goodmath.scientopia.org/2011/07/06/things-everyone-should-do-code-review/
- http://support.smartbear.com/support/media/resources/cc/book/social-aspects-of-review.pdf
Project-specific process When applying the best practices and adding some common sense, only internal process details are left to specify.
Collaboration gitlab ain't no forum.
When a comment starts to become a thread or requires a lenghty or complicated answer, a meeting should be had involving all concerned parties. Zoom is the recommended solution for inter-office discussions.
Doing it any other way will most of the time lead to nothing good.
About WIP merge requests
- WIP merge requests are meant to get early feedback on unfinished code.
- Authors should point out to reviewers what kind of feedback they mean to get.
- The WIP merge request should be closed when feedback has been gathered. Authors shouldn't leave WIP merge requests open for a long time as it creates unnecessary in the merge request page.
Author: Asking for a code review Warning: MR's that break too many of these recommendations will get closed and it will be upon you to fix what made it be closed.
- Keep your reviews under 500 LOC's unless some wide-scale focused (one thing) refactor is involved.
- Review your own code before asking for your merge request to be code reviewed.
- When creating the merge request, a message should be sent to the proper slack channel to request for reviewers.
- You should actively seek for reviewers since code in merge request is closer to deliverying value than any other
- JIRA or confluence link should be provided in the description whenever possible to help reviewers
- All tests should pass unless exceptions and team agrees
- TDD: Tests have to accompany the code at all times since it's hard to tell if tests are adequate when they are not committed at the same time, and most of the time code will change as tests are done.
- At least two thumbs up required before merging. Author could ask for more or specific reviewers.
- Every discussion needs to have an accompanying comment on what the resolution was.
- When a thumbsup is given, any pending comment have to be resolved (gitlab functionality) before merging.
- When a thumbsdown is given, code shouldn't be merged even if two other reviewers gives thumbs up.
- No backdoor commits: use another branch for any code added on top of an open merge request, even for code review changes, unless minor.
- No fire and forget: when an author finally merges codes, babysitting should occur to see that everything is still good in target branch.
- The branch should be in sync with it's target beforehand.
- When asking for senior resources outside o team for a review, the review process should have been done by a close colleague beforehand to ensure as many issues were already found and solved as possible.
As a reviewer
- No skimping: thumbs up without any or with very few comments are not helping anyone.
- When doing a review one should stick to it and not get sidetracked. The author is waiting for precious feedback and thumbups to go on with his work.
- Checking out the reviewed code branch makes it much easier to get context on the changes.
- Give one, get one: it's ok to have an emergency or being on the critical path once in a while, but reviews have to be done, it's part of the job.
- One should quickly let other team members know when he cannot review.
- A thumbsdown should be given when a showstopper is sighted.
- You shouldn't do more than 1 hour of review per day for both reducing code review fatigue and have wider base of people doing code reviews
- Even if you aren't familiar or don't feel qualified to review you should still do it - asking questions on things you don't understand is a legitimate part of the review process
- If you are within the team of the person who submitted the code review you are a good candidate for reviewing said code.
- If a particular code issue is often found and fixed, the good practice should be documented and linked to in the future.
Tips and Tricks Merge requests / Code reviews size management These strategies can help breaking down reviews size
Story branches Create a story branch to which many smaller pieces of codes will be reviewed then merged. Each of these will be reviewed independently, when the work is completely the story branch can be then merged without any further review as all code in it has already been reviewed.
Only one concern per code review It is much easier for a reviewer if the author presents the code one step at a time. If some repacking or renaming has to be done, it should go in a separate code review to reduce noise from other reviews. It is very easy to perform a review where only a few package have changes names then others with single business logic changes in them then packing everything at once. A common problem amongst the business logic changes can also be caught earlier making for less code review repairs.
New mechanics or concepts in the codebase A quick meeting should be had with team right after exploratory phase before reaching end of code so that everybody agrees on approach and there is no surprise in the code reviews.
Introducing new dependencies Dependencies should be introduced with care and each such change should be team approved. POMs are first-class residents.
Author cheatsheet for faster code reviews
- Merge request should comply with everything in this page
- Breaking rules is ok as long as reviewers agreed beforehand
- Review your own merge request before asking for it to be reviewed
- Nothing was left to be found that might be a problem but could go under the radar
- Smaller chunks are reviewed faster. Half to a few days worth of work is usually enough
- Project conventions and coding style is respected
- Code have been synced with the target branch and the build + tests pass 100%
Delete your branch upon merge
- Housekeeping really, but it can get quite messy so author should delete branches after accepting merge requests.
I learnt a lot