Can notes/to-dos in code comments sent to code-reviews result in an effective refactoring process?

Question

I want to start/improve a culture of collective code ownership at my company but at a geographically distributed level... I'd say there is some current collective code-ownership mentality, but only at single geographical sites.

This is a follow-up to this question: What is the politically correct way of refactoring other's code?

I'm just wondering if submitting *just code comments* for code reviews (we have ReviewBoard, possibly upgrading to Crucible) could actually be an effective mechanism to get the conversation started on improving code, without having others feel territorial about their code.

For example, if I add:

//ToDo: Refactor this code and that code because of reasons X and Y

Then, submit it for code review, and it gets accepted... it could be considered as an agreement (which I think is sometimes harder to get with new code up front). At the same time, the author (and others) might have an easier time digesting and accepting the proposal; rejecting a proposal because it might break things will not longer be a valid reason and therefore the fear of making a change is lost... and at the same time, do not invest 10 hours optimizing something that no one thinks it is worth it and opposes to it just out of fear.

This is all conjecture, but I'm feeling something like this (submitting refactoring notes in code comments at the code-review process) would work. Has anyone done something like this in practice?, if so, what have been the results?

Answer 1

Use a Formal Review Process

It works, if this is indeed part of a process that all members need to follow.

Meaning that when a code review is initiated, it's expected that there will be comments, and that these comments MUST be acted upon, either by discussing and discarding them or by implementing a change.

Link to your Issue Tracker for Follow-up Actions

If you want to be extra formal (which might be required depending on your environment and audit procedure), you could even open new tickets in your tracker for each required action, which can then be closed either as fixed or rejected with the necessary comments for clarification. If further reviews are needed, then open a review request (assuming you have a tool to do so). Code changes need to be verified, preferably by the original reviewer.

Beware of Crippling your Codebase

If that's not part of your process, then such comments are likely to become akin to dead-code or code-rot, and possibly leave more interrogations than answers behind. So if you don't have a formal review process, you might want to avoid those, except if it's for personal tracking and you keep a tight tab on them. These should preferably have a short lifespan, and be acted on quickly. Or they shouldn't be TODOs and be actual code comments about what's wrong and needs to be changed if it's a major change, to serve as a warning to other developers.

I give additional reasons against (in the general case) inline action tags and useless inline comments in this answer about the use of non-documenting inline comments.

In fact, if you can and have the right tool for it, I'd suggest you avoid these for code reviews and use a code review and code annotation tool that allows to separate this from the code.

Rely on Code Review and Code Annotation Tools

It's a tricky decision to make, as sometimes you'd prefer to have everything with the code in your SCM (in fact there's even a code review tool for Eclipse that stores review comments in text files along with the source files, for instance). But for larger teams and projects it works quite well to have dedicated tools to annotate code and have extended discussions recorded somewhere, within the context of your code. You can also link them to current code-reviews, to tasks, or directly make comments on a file or a changetset.

^{In the screenshot above, for instance, you can see a threaded discussion directly inlined with the code to highlight a concern, and this is being done as part of a formal core review item (with the ID CR-ANERDS-8) relating to an open ticket (with the ID ANERDS-7).}

^{Here is an example of a use case where you create a new ticket to be able to track future follow-up actions on the review. The reviewer can easily create tasks as he sifts through your code and notices shady spots.}

There are numerous tools for this, but personally I tend to find FishEye and Crucible above par (and obviously working great hand in hand with JIRA, or even Confluence to link to even more detailed docs if needed).

Answer 2

In my experience this doesn't work because nobody takes these comments seriously. Nothing is funnier than seeing "TODO: get rid of this asap" and then doing svn blame and noticing that the comment has been there for almost a decade.

What you really need to do is make sure actual developer time is dedicated to refactoring, and the refactoring tasks are well defined (not open-ended). You need to present this to the manager and outline the problems as well as the business-benefit of doing it.

For example: "Implementing features X, and Y will become way easier, and will take much less time if we first refactor Z. Let's make some time to refactor Z!"

Answer 3

This seems like a cultural issue to me, so I would suggest working on the culture. As an indirect answer to your question, I've had success introducing this topic of collective code ownership and the refactoring process by starting a technical book club.

Clean Code by Bob Martin and Working Effectively with Legacy Code by Michael Feathers do a good job of exploring these topics.

Answer 4

I have experienced, that comments alone will change nothing. If the programmers do not talk at all - except through code-commentary - something is wrong. Just adding a //Todo now and then will not solve you problem, it might even make them worse. For example if someone feels offended.

If you find something that is worth refactoring (and you're sure you totally understand the piece of code AND the impact on the existing program, then just do it. If you do not think you have an absolute understanding of the code (this might be the reason you want to refactor in the first place), try to make contact with the initial developer. Involve him in the process, discuss what you can do, and i'm sure he will be a valuable partner through the whole process of refactoring.