Is consequent and efficient refactoring enemy of code reviews in a legacy code context?

Question

There are some Tech Leads that perform code reviews for each Git pull request.

Apparently, a very good practice to be aware of the evolution of code.
Basically, they expect to inspect a few lines of code differing from previous code in order to ensure "quality" easily.

As a lot of good developers know, another very good practice is to clean code during refactoring steps especially whenever newly applied on poorly designed legacy code.

You may guess the point... Refactoring is likely to make code reviews really harder to grasp, since it's not about few lines of code changing, but extracted classes, well-defined private methods, dropping of useless pieces of code etc.

Commits would then contain changes about features + changes regarding refactoring.

How to find a good compromise between Refactoring and Code Reviews facility?

Answer 1

For pull request you need already provide possible clean code. Where during review other pair of eyes can possible discover some "small" improvements:

• method/classes names
• extracting methods
• etc.

If every pull request requires "big" amount of refactoring then you doing something wrong.

In TDD the task will be done only when you go through all three phases

• write failing test (red)
• write production code for passing the test (green)
• refactor - this must be done before you can commit changes

Answer 2

If you have to refactor constantly, it only means the code is in a state that needs refactoring, and nothing more. It can happen using any type of development. Generally speaking, it is a good thing if code needs less refactoring over time: it means the code is working as needed, and is well designed. This should be one goal, which is in line with the minimal changes needed for the code reviews.

I suggest to avoid the coding reviews until the legacy code is cleaned up. Another option is to slow down the refactoring, holding off until a design is finalized, then do a bit of clean up, commit, and repeat. Try to refactor each thing once (i.e. with careful thought), and commit after each refactoring.

Answer 3

TDD makes reviews easier because test cases make the workings of the code changes more obvious. A well designed test suite would also help give you confidence that the change set isn't introducing a regression into the rest of the codebase. This allows the reviewer to focus on whether the code is missing obvious edge cases or doesn't follow good practices & standards.

You seem to be under the impression that TDD leads to constant refactoring. This is not the case. While TDD can make it easier to refactor your codebase and do so without introducing new bugs, refactoring is not a requirement.

While it's normal to iteratively write tests & write code, tweaking the code to match the requirements for the new tests which can lead to large changes in the early stages of development, this does not mean that developers should be committing code after every test is complete. Commits should still maintain the granularity of a completed feature or bug fix.

Answer 4

As @Sean mentions, refactoring should be separated from feature additions to keep things really clean.

I think it is as useful to review refactoring as it is to review feature additions.

Any given refactoring needs to be explainable, if it is sufficiently simple, then in terms of the small refactoring steps, but more importantly whether small or large, in terms of the better domain-applicable abstraction that is being provided.

Good abstraction operates in terms of consumer-appropriate concepts and behaviors that work together to support the consumer's usage scenarios.

In an abstraction that is no longer providing for newer usage scenarios, sometimes the incompleteness is made up by adding code to the consumer often that knows too much about the implementation details of the abstraction and needs to reach around to a lower level to make up the incompleteness. This creates technical debt by spreading responsibilities between a provider and its consumer.

This situation indicates refactoring to create improved abstraction. Such a refactoring may augment an existing set of concepts and behaviors, or may replace existing abstractions with a new set of higher-level more domain-appropriate concepts and behaviors.

When we do refactoring, it should be explainable: the new abstraction incorporates another capability or another concept with capabilities, or perhaps it is a whole new set of simpler or more useful concepts & behaviors.

In either case, it should make things easier for the consumer, so the next level up can "stand on the shoulders" of the refactored abstraction instead of working around it.

In some cases, however, the implementation burden for a given abstraction is too high (it cannot be provide it without serious compromises) and in those cases the abstraction should provide something lower level, simplifying the implementation and being presented to the consumer in a different way.

Refactoring is about negotiating the boundaries of our abstractions, and, these adjustments in boundaries should be explainable to those reviewing code.

Answer 5

I find that keeping refactoring in separate commits from other changes can make this easier. I also like to do one refactoring in one commit, and say what the refactoring is in the commit message, e.g. "Extract Method getAThing() from foo.php"

Using the names of the refactorings from Fowler's catalogue of refactorings also helps.