JPragma Blog

Pragmatic Developers, Elegant Software

Toughts on code reviews and dev process

with one comment

Nowadays most software projects set the master branch to be read-only, use pull requests to merge feature branches and require code reviews with multiple approvals. Undoubtedly this a good practice in theory, however, as usual, the devil is in the detail, and quite often typical code reviews are useless and even harmful.

  • Most standard code review tools display all changes in a flat list, so the context of these changes is completely lost. The order, in which files appear has nothing to do with the logical organization of the change (and the codebase in general).
  • It is extremely hard to reason about changes larger than just a few files with more than just 10-20 added/modified lines of code in each – especially when these changes mix design concepts and low-level implementations of particular functions. It requires a very deep context switch and extreme discipline from the reviewer.
  • As a result, many reviews focus only on very simple things. Things that could and should be discovered by code analysis tools, IDE inspections, SonarQube and other automatic quality gates. Reviewer’s time is expensive, code analysis tools are cheap.
  • When PR is submitted for the review, the code is already written. It is often too late to comment on the fundamental problems in the design. Such comments are hard to address and naturally, they are met with lots of resistance and even anger from the code author.
  • Often there is a tight deadline to deliver the feature, so the pressure goes on the reviewer – “It is his fault we didn’t deliver the fix on time, he didn’t timely approve my code change”.
  • Typically author of the PR selects the reviewers. This leads to uneven distribution of the reviews (which consume significant time) or sometimes it promotes selecting reviewers that “go easy on everyone”.

What’s the solution?

I propose using the following process:

  1. Developer starts working on the assigned feature/ticket, e.g. TICKET-123.
  2. Create and check out feature branch – feature/t123/my-feature.
  3. Analyze the feature and if it is complex enough write a design document. Depending on the complexity, this could be just a few sentences in the MD file or full wiki page with diagrams, etc.
  4. Create another branch feature/t123/my-feature-design, commit there your design document (or link to it) and submits the PR with overall feature/t123/my-feature as a target. Some projects might require that only tech leads or architects could review such “design” PRs.
  5. Once “Design PR” is approved, merge it and creates another branch feature/t123/my-feature-blueprint. The purpose of this branch is to create the overall skeleton of the feature, adding lots of placeholders for every implementation detail. The more placeholders you add the better it is. Each such placeholder becomes a subtask of the feature. These placeholders might use some special markers like // PH-1 User input validation, and your IDE can be easily configured to display the consolidated list of such markers. After this is done, submit the PR to merge feature/t123/my-feature-blueprint into feature/t123/my-feature. Here your reviewers will have an opportunity to evaluate the overall feature design at the code level and express their concerns or suggestions.
  6. Next, implement every placeholder defined in the previous step. Each placeholder is implemented in a separate branch that is forked from feature/t123/my-feature and dedicated PR is submitted for each. Note that at this stage “placeholder” branches could be implemented in parallel by several developers. Ideally, reviewers of these placeholder implementation PRs should be assigned randomly, promoting a balanced workload as well as knowledge sharing within the development team.
  7. Once all placeholders are resolved, submit the final PR merging the overall feature/t123/my-feature into master. Since every change in this PR is already reviewed and approved, the goal of this PR is to simply verify the commit history of the feature/t123/my-feature branch and make sure no direct code commits are done.
  8. Finally, merge the feature branch into the master.

This process has many advantages. First of all, reviewers have a chance to comment on feature design before too many resources are spent on implementation. Secondly, instead of one huge PR, we are dealing with several small PRs each one staying at a single level of abstraction and never mixing the overall design with small details of a particular step. Each such PR is easy to understand and provide meaningful comments. It promotes knowledge sharing in the team and minimizes frustration and conflicts when some big PRs are rejected.

As usual comments, opinions, and criticism are more than welcome!

Written by isaaclevin

November 13, 2019 at 1:40 pm

Posted in General, Productivity

One Response

Subscribe to comments with RSS.

  1. Good approach, for more junior team members this can be achieved by team leads doing design and then structuring a bunch of detailed jira stories accorgly. With more senior members, I’ve seen an alternate approach by a team member who simply pushes PRs for larger stories frequently enough before they become too large, that also seems to work well.


    November 14, 2019 at 9:17 pm

Leave a Reply

Fill in your details below or click an icon to log in: Logo

You are commenting using your account. Log Out /  Change )

Google photo

You are commenting using your Google account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s

%d bloggers like this: