Code Reviews – Reconciling the Boy Scout Rule and Opportunistic Refactoring

code-reviewsrefactoring

I am a great believer in the Boy Scout Rule:

Always check a module in cleaner than when you checked it out." No
matter who the original author was, what if we always made some
effort, no matter how small, to improve the module. What would be the
result? I think if we all followed that simple rule, we'd see the end
of the relentless deterioration of our software systems. Instead, our
systems would gradually get better and better as they evolved. We'd
also see teams caring for the system as a whole, rather than just
individuals caring for their own small little part.

I am also a great believer in the related idea of Opportunistic Refactoring:

Although there are places for some scheduled refactoring efforts, I
prefer to encourage refactoring as an opportunistic activity, done
whenever and wherever code needs to cleaned up – by whoever.
What this means is that at any time someone sees some code that isn't
as clear as it should be, they should take the opportunity to fix it
right there and then – or at least within a few minutes

Particularly note the following excerpt from the refactoring article:

I'm wary of any development practices that cause friction for
opportunistic refactoring … My sense is that most teams don't do
enough refactoring, so it's important to pay attention to anything
that is discouraging people from doing it. To help flush this out be
aware of any time you feel discouraged from doing a small refactoring,
one that you're sure will only take a minute or two. Any such barrier
is a smell that should prompt a conversation. So make a note of the
discouragement and bring it up with the team. At the very least it
should be discussed during your next retrospective.

Where I work, there is one development practice that causes heavy friction – Code Review (CR). Whenever I change anything that's not in the scope of my "assignment" I'm being rebuked by my reviewers that I'm making the change harder to review. This is especially true when refactoring is involved, since it makes "line by line" diff comparison difficult. This approach is the standard here, which means opportunistic refactoring is seldom done, and only "planned" refactoring (which is usually too little, too late) takes place, if at all.

I claim that the benefits are worth it, and that 3 reviewers will work a little harder (to actually understand the code before and after, rather than look at the narrow scope of which lines changed – the review itself would be better due to that alone) so that the next 100 developers reading and maintaining the code will benefit. When I present this argument my reviewers, they say they have no problem with my refactoring, as long as it's not in the same CR. However I claim this is a myth:

(1) Most of the times you only realize what and how you want to refactor when you're in the midst of your assignment. As Martin Fowler puts it:

As you add the functionality, you realize that some code you're adding
contains some duplication with some existing code, so you need to
refactor the existing code to clean things up… You may get something
working, but realize that it would be better if the interaction with
existing classes was changed. Take that opportunity to do that before
you consider yourself done.

(2) Nobody is going to look favorably at you releasing "refactoring" CRs you were not supposed to do. A CR has a certain overhead and your manager doesn't want you to "waste your time" on refactoring. When it's bundled with the change you're supposed to do, this issue is minimized.

The issue is exacerbated by Resharper, as each new file I add to the change (and I can't know in advance exactly which files would end up changed) is usually littered with errors and suggestions – most of which are spot on and totally deserve fixing.

The end result is that I see horrible code, and I just leave it there. Ironically, I feel that fixing such code not only will not improve my standings, but actually lower them and paint me as the "unfocused" guy who wastes time fixing things nobody cares about instead of doing his job. I feel bad about it because I truly despise bad code and can't stand watching it, let alone call it from my methods!

Any thoughts on how I can remedy this situation ?

Best Answer

OK, so there are now more things here than are suitable for a comment.

tl;dr

Your intuition about what you ought to do (refactoring as you go) is correct.

Your difficulty implementing this - given that you have to work around a poor code review system - comes down to your difficulty manipulating your source code and VCS. Multiple people have said that you can and should split your changes (yes, even within files) into multiple commits, but you seem to have difficulty believing this.

You really can do this. That really is what we're suggesting. You really should learn to get the most out of your editing, source manipulation and version control tools. If you invest the time in learning how to use them well, it makes life much easier.

Workflow/office politics issues

I'd make essentially the same recommendation as GlenH7 that you create two commits - one with only refactorings and (demonstrably or obviously) no functional changes, and one with the functional changes in question.

It may be useful though, if you're finding lots of errors, to pick a single category of error to fix within a single CR. Then you have one commit with a comment like "dedupe code", "fix type-X errors", or whatever. Because this makes a single type of change, presumably in multiple places, it should be trivial to review. It does mean you can't fix every error you find, but may make it less painful to smuggle through.

VCS issues

Splitting the changes made to your working source into multiple commits shouldn't be a challenge. You haven't said what you're using, but possible workflows are:

  1. if you're using git, you have excellent tools for this

    • you can use git add -i for interactive staging from the command line
    • you can use git gui and select individual hunks and lines to stage (this is one of the few VCS related GUI tools I actually prefer to the command line, the other being a good 3-way merge editor)
    • you can make lots of tiny commits (individual changes, or fixing the same class of bug in multiple places) and then reorder them, merge them or split them with rebase -i

  2. if you're not using git, your VCS may still have tooling for this sort of thing, but I can't advise without knowing what system you use

    You mentioned you're using TFS - which I believe is compatible with git since TFS2013. It may be worth experimenting with using a local git clone of the repo to work in. If this is disabled or doesn't work for you, you can still import the source into a local git repo, work in there, and use it to export your final commits.

  3. you can do it manually in any VCS if you have access to basic tools like diff and patch. It's more painful, but certainly possible. The basic workflow would be:

    1. make all your changes, test, etc.
    2. use diff to make a (context or unified) patch file with all changes since the last commit
    3. partition that into two files: you'll end up with one patch file containing refactorings, and one with functional changes
      • this isn't entirely trivial - tools such as emacs diff mode may help
    4. back everything up
    5. revert to the last commit, use patch to replay one of the patch files, commit those changes
      • NB. if the patch didn't apply cleanly, you may need to fix up the failed hunks manually
    6. repeat 5 for the second patch file

Now you have two commits, with your changes partitioned appropriately.

Note there are probably tools to make this patch management easier - I just don't use them since git does it for me.

Related Topic