69

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?

Cody Gray
  • 1,738
Tisp
  • 817

8 Answers8

83

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.

l0b0
  • 11,547
28

Just nitpicking, but usually I try to do the refactor before the change:

Commit 1: Refactored class hierarchy in preparation for feature XY-123

Commit 2: Implemented feature XY-123

Another one I like is:

Commit 1: Cleanup code base before starting feature XY-123

Commit 2: Implemented feature XY-123

It's not just for other developers, but it also adds more transparency to management or whoever might stumble over it. With some people, it is harder to discuss refactorings after everything works, but nobody would want to start a feature with a wrong class hierarchy and nobody would want to start a new feature in a messy codebase.

So do refactorings first. Or commit them first, pretending you did them first. :-)

Considering the question itself, there are already many good answers. You should really split those two if possible. If not possible, the refactor is just part of the task and can be reviewed together.

Peter Mortensen
  • 1,045
Hermine
  • 289
16

For me, it depends on if the main change is difficult to follow without the refactor. If it is difficult to follow, I'll ask for the refactor to be done as part of the pull request, in the interest of validating the current change. If someone suggests to me such a refactor, I will put the current pull request on hold and go do the refactor in a separate pull request first, then come back to the original one. Some people really push back on suggestions like that, so I pick my battles.

If the main change isn't difficult to follow without the refactor, it doesn't matter when you do it. I usually like to create an issue so it isn't forgotten.

Karl Bielefeldt
  • 148,830
4

If it is tiny, and the sort of change that is very straight-forward so has minimal risk.

Otherwise, that sort of thing is better done as a follow up PR/task (but should still be mentioned during code review).

Telastyn
  • 110,259
3

I think if we can improve our quality of code in all PR, the whole project tends to be better, is that a wrong thought?

It's not wrong, but it can be overapplied.

I generally clean things up as I go. Sometimes, while browsing through code, I might fix a typo or misnomer that I come across. That's really not worth opening a new branch for and going through an entire PR process.

For context, in my company you cannot commit to master and PRs must have an accompanying ticket and go through QA before being merged into master, making it a cumbersome process.
In my own personal projects, I tend not to commit to master but I generally try to keep these small and inconsequential refactorings out of my feature branches, so I will be more inclined to commit these to master directly.

But when the refactoring becomes non-trivial, the added complexity can start to distract from the pull request.

If the refactoring is a consequence of the changes in the pull request, then that needs to be resolved inside of it. But when it's not related to it, it may be better to move this to a new branch/PR of its own.

To summarize, your intentions are not wrong when applied on a small scale but beware of going overboard and putting too much large scale work/change complexity on a single PR.

Flater
  • 58,824
3

It depends on how far out of scope the changes are. Generally, holding a pull request hostage for code improvements outside the scope of a PR should probably be frowned upon.

I've seen a lot of PRs become part of a, sometimes larger, political tug of war where some are forcing their coding preferences on others without a full open discussion.

I think it's better to have a separate task and pull request for that. However, if the new code is bad and everyone agreed to the new style, then it seems reasonable to request changes.

Peter Mortensen
  • 1,045
Mark Rogers
  • 297
1

I don't think it's a good idea generally to combine them. If the refactoring is small or directly related to the task then go for it. If there are dozens of changes with renamed files and formatting fixes, they can make it difficult to pay attention to the actual change being made.

It comes down to the purpose of the code review. For a simple example, say your story is to add refunds to a net revenue calculation. If your change is this:

- net = gross - expenses;
+ net = gross - expenses - refunds;

A reviewer might think about that and realize that the refunds value is negative. The real change should have been:

- net = gross - expenses;
+ net = gross - expenses + refunds; // refunds is a negative value

If you are asking reviewers to look over dozens of changes due to refactoring things that don't have anything to do with the story you're working on, it distracts them, especially if this change is 'hidden' because it's a small piece of code in a much larger block because you refactored the method into another class.

If you are doing something like this, consider leaving your own comments in the review pointing out the specific changes that are for the story you are working on and not simply due to refactoring so reviewers can pay extra attention to those parts of the code.

I like creating separate PRs for refactoring. What is the point of combining different changes into a single PR? It's not too hard to create a second PR for the refactoring. Combining them might be a little easier for the developer, but it makes it harder for the reviewer(s).

Jason Goemaat
  • 175
0

You shouldn’t, unless you feel unable to review the code as it is. You are only creating unnecessary work. And if you feel unable to review the code, you maybe should learn some new tools. You are creating frustration for no good reason, and you should not be surprised if your own code will have problems passing a review, depending on characters involved.

gnasher729
  • 49,096