Making pull requests easier to review while practicing XP and refactoring

Question

I practice TDD and refactor aggressively. Recently some colleagues have complained that the resulting pull requests are difficult to review, or would require a lot of regression testing.

When I refactor, I'm disciplined about using safe, behavior preserving refactorings, mostly around changing names and extracting methods. When someone says "this will need a lot of testing", I show them how I got there and the reply is "oh, I guess that doesn't need a lot of testing", so my method is sound from a quality perspective.

But, I look at my own pull requests and put myself in a reviewer's shoes and I see where they're coming from. The steps aren't necessarily obvious and the resulting change may be relatively large (say 8 files).

If you practice XP or TDD and refactor aggressively, how do you make your pull requests reviewer friendly?

Answer 1

You wrote

When someone says "this will need a lot of testing", I show them how I got there

So they could not get the information "how you got there" easily from the commit logs, you had to explain it to them? That gives me the impression your commits are too large, or your commit messages should be improved, or both. So I would recommend the following:

• Separate refactorings strictly from changes which affect behaviour, and commit them individually

• Avoid even committing multiple refactorings at once, especially when the affected code areas "overlap".

• Make sure each and every refactoring you did is mentioned in your commit, and focus on writing why you changed something.

For example, when you change the name of a function or of a member variable where you have to touch multiple files, don't intermix this with an "extract function" refactoring. Instead, compile and commit the renaming first, and mention the kind of renaming (and maybe why you did it) in the log. Then make the "extract function" a separate commit, and mention for what reason you extracted the function. If, for example, you also removed some duplicate code by reusing the newly extracted function, that should be mentioned was well. If this remove of duplication will affect many locations in your code, consider to do it in a separate commit.

That way, if your reviewers have problems to understand your latest pull request, they can go through the commit log and follow the individual steps of your changes one-by-one.

Answer 2

I don't know how else to answer this question except by personal experience. I have a similar circumstance, although my colleagues are probably less willing to complain about my pull requests, owing to my position on the team. Still, I try to ensure that the team can understand what I have done and why I have refactored a thing, and have confidence via unit testing or integration testing that the changes I have made don't introduce issues.

There are three things I do, but they all boil down to discipline. There's no substitute for having discipline about your own coding habits.

1. Stick to the assigned issue. Hopefully, you have well-written issue tickets that describe exactly what has to be done. It's often tempting when diving into legacy code to start "fixing" everything that touches your new code. Resist that temptation, and file new issues for the fixes that aren't strictly necessary to resolve for the issue you're working on.
2. Commit meaningfully. I've seen many a developer who works on an issue and then simply does git commit -a when they are done. If many files were touched and the changes took several days, that single commit will include many things and will be difficult to evaluate in a pull request. Less frequently, I've seen developers who commit every single file or little change separately, and a relatively simple pull request will have dozens of commits. Task decomposition is a key skill here. The way I think about how to make proper commits is to describe in words in a bullet list what steps I will take to resolve an issue. That should line up (more or less) to the list of commits I have in my pull request when I am done. Of course, it's always nice to ensure that no single commit breaks the build (compile or test, according to the source code management policy of your project), but sometimes that's not feasible. For myself, I aim to avoid breaking compile on any single commit, but testing may not pass until all the work is done. Of course, if you are following TDD strictly, your tests will by design not pass until your work is done.
3. Increase test coverage as necessary. There's no substitute for test coverage. If you're refactoring legacy code, it is your responsibility to add the appropriate test coverage, even if equivalent tests were not present before. If you know your changes will create a need for regression testing by someone else, that effort needs to be part of the discussion before you begin work on the issue. It's not fair to your coworkers to alter code whose behavior they've become accustomed to (even if it wasn't well-covered), and not also provide tests to cover your changes and account for the additional work of others, and then some. Sometimes I add tests on code I didn't change, just to prove to myself it works and I can depend on it. This often feels like a chore, but will save so much pain and heartache down the road. This doesn't negate the need for additional regression coverage as you described, but will give you confidence down the road.

Now, if you're already a disciplined coder and doing all of these things already, then maybe your colleagues tolerance for risk or change is just lower than yours, or maybe the team does not feel they have the available time to accommodate all the changes. All of these things indicate the need for better team communication, and better shared understanding of impacts before work begins on issues.

Best of luck to you!

Answer 3

One thing that might help would be to list the refactorings you used in the commit message.