Code Review – Should Refactoring Be Requested in a Pull Request?

code-qualitycode-reviewsgitgitflowgithub

I have been studying the best practices for a code review, and I was wondering what to do in the following scenario:

During a code review, I see potential improvements, but decide that they are outside of the scope of the pull request (PR). Should I ask the reviewee to do the refactor in that same PR, or should I defer it to a future PR since it is technically out of scope?

I think that all PRs should strive to improve the overall quality of the code, as this tends to make the whole project better. Is that a wrong thought? Should I be more conscious about narrowing the scope of my code reviews?

Best Answer

There are several relevant trade-offs here:

  1. Review complexity. If a branch has more than one functional change commit or more than one refactoring commit it becomes time-consuming to review the result, since now each commit has to be reviewed separately.
  2. Risk. Any refactoring, no matter how well the code is tested, has some non-zero risk of breaking things. Making a separate branch with a refactoring allows splitting that risk from the more obvious risk of the functional change.
  3. Relevance. Is the suggested refactoring a natural consequence of the functional change? This may be for example breaking up a class hierarchy because the inheritance is no longer natural. If so it might be appropriate to do it in the same commit as the functional change, as per the red-green-refactor TDD cycle.

In general, if the refactoring is really outside the scope of the branch I would recommend making it a separate branch.

Related Topic