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 - on strike
  • 1,737
  • 2
  • 19
  • 22
Tisp
  • 817
  • 1
  • 5
  • 7

8 Answers8

81

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,014
  • 2
  • 43
  • 47
  • So you are recommending putting all the work in a single commit ? – Shiplu Mokaddim Aug 06 '20 at 23:56
  • @ShipluMokaddim I'm saying the answer to that should be based on relevance for the commit. There is no useful universally applicable binary answer to this question. – l0b0 Aug 07 '20 at 00:53
  • This is the best answer. It depends on the nature of the project, the release cycles, and the team culture. I've worked on highly risk-averse projects where making PRs very small and relevant is important, and refactoring is considered potentially dangerous and usually requires a ticket and lengthy QA process. But in other projects I've worked on, continual cleanup of legacy code is encouraged and larger PRs are fine. – cbp Aug 26 '20 at 00:29
27

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,050
  • 2
  • 12
  • 14
Hermine
  • 279
  • 2
  • 2
  • One problem with this is that a substantial refactor to some degree masks the direct impact of the actual feature change or bug fix, as people won't be immediately familiar with the "new" version of code that you're then immediately expanding/changing. But I think on balance that's a minor problem that doesn't outweigh the benefits (avoiding doing the same work "twice" in quick succession) – Asteroids With Wings Aug 06 '20 at 12:16
  • 7
    "for each desired change, make the change easy (warning: this may be hard), then make the easy change" https://twitter.com/KentBeck/status/250733358307500032 – Andrea Reina Aug 07 '20 at 05:25
  • Also sometime it is useful to add a third commit if it was hard to get working code so that if it broke during final cleanup/refactoring, it is possible to analyse changes. – Phil1970 Aug 28 '20 at 02:59
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
  • 146,727
  • 38
  • 279
  • 479
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
  • 108,850
  • 29
  • 239
  • 365
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
  • 44,596
  • 8
  • 88
  • 122
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,050
  • 2
  • 12
  • 14
Mark Rogers
  • 297
  • 2
  • 9
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
  • 5
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
  • 42,090
  • 4
  • 59
  • 119
  • 1
    Can you explain why this is "unnecessary work"? Clearly, the reviewer considers the changes to be useful, or they wouldn't be recommending them. So, with that assumption in mind, the question is only between making them part of *this* PR (the one under review) or a *future* PR. Also, I think this answer's suggestion that someone might attempt to spite you in future code reviews for pointing out an avenue of [what is believed to be] significant improvement to be quite distasteful. – Cody Gray - on strike Aug 08 '20 at 09:12
  • Clearly, the reviewer doesn’t have to do it, so they are not objective. I’ve seen reviewers on a power trip, and they need to learn quickly. The final result will be the same, with extra work. No improvement. And yes, if you think you can force people to do unnecessary work, for no useful improvement, be warned that it can sour relationships. – gnasher729 Aug 10 '20 at 14:20
  • Why do you keep making the assumption that this is extra work "for no useful improvement"? Even if the results are the same, style and readability matter; that's one of the whole purposes of a code review. If it were only about functionality, then code reviews would be completely supplanted by automated unit tests. – Cody Gray - on strike Aug 11 '20 at 22:05