Is it okay to refactor during a code review?

Question

We all submit code reviews to our peers for code that we write. Is it justified for our peers to refactor our code while they review it?

Should their response to the code review be a list of revisions to be completed and resubmitted, or a list of things they have changed and why?

EDIT:

I chose the accepted answer because it is most helpful to my everyday scenarios (and seems to be the popular vote as well). Thanks to everyone for the responses. I think it should be noted that the answer is relative to environmental factors and people involved.

EDIT:

Changed the accepted answer because of the level of detail and thoroughness.

Answer 1

It should be a list of suggestions and comments. Otherwise you won't learn from things you've done.

You should make changes in response to suggestions/comments, or comment on why you aren't making changes, and then repeat the code-review cycle until there are no further suggestions/comments to follow up on.

If others change your code during the code review process, it'll be harder for you to learn from the experience.

Answer 2

It depends on the team you're working with, but for me I really like the idea of having the reviewer refactoring code and then going over the changes with the original implementor. You still need to illustrate what was changed, but overall I think that provides more of a team atmosphere IMO.

I would add to that, however, that what I really like to see is the reviewer writing tests (unit/integration/etc) around code. When you get reviewers doing that you'll find that they will have to refactor code in order to write good tests, which really helps improve the quality of the code base.

I'm not saying that the original implementation shouldn't have it's own tests, but I think the review might have valuable contribution in the sense of tests that maybe the original implementation didn't think of.

Answer 3

There is a lot to be said for performing code reviews in a room with no computers and with the code printed out on paper. This removes any temptation to make so-called "improvements" on the fly, and forces someone to take notes on problems etc.

Answer 4

Even better than refactoring when reviewing, would be to refactor and review all the time. By this I mean pair programming. Code review is known to be a good method for finding problems, so in Extreme Programming they took code review into the extreme and it became pair programming.

When two people are writing code together as a pair, they are essentially doing code review as the code is being written. And whenever you notice something that needs improving, it's best to improve it right away; firstly because that's when it's fresh in your mind, and secondly because after doing one refactoring you will often times notice new areas for improvement, which you were not able to notice because of the previous mess. When you refactor the code continuously, it's easier to keep the codebase in top shape, which in turn makes adding new features and making changes faster.

Answer 5

a consequence of refactor-reviews is it forces the team vibe from individual accountability of pieces, to collaborative accountability of the whole. i really like that.

Answer 6

In my experience, code reviews are valuable for various reasons, varied purposes, and there is not one best methodology. Reviewing on paper helps reviewers step away from their workstation and focus on the code. Reviewing at their workstation helps them probe the code, build, test, and hit it with whatever hammers they wish. Reviewing in pairs or small groups at workstation (projector in a workroom, whatever) offers a decent mix. Sometimes reviews are remote. Sometimes there's two weeks to review, other times it's two hours.

To your specific question about refactoring, the answer has to be yes and no. The outcome of a review might be that they propose a refactoring (even going so far as to do it for you), but like all changes, they should have a good reason for this. If you don't like what they propose or it's wrong-headed you've got the original. The code under review might just be stylistically imperfect, but it might be narrowly coded (ignoring impacts on other systems or components) or it might be beautiful and work but set bad precedents.

It's comes with the territory that sometimes a suggested change is important and is going to require scrapping part or all of what you've written. Even more important is where lies the responsibility to insist on a change you don't want to make. The reviewer should insist on the change or escalate it to someone who can. The code remedy might be to clean it up a bit for an impending release, but make the next order of business to fix what you've done. It must become a priority and the longer your broken code exists, the larger the damage can be. (Likewise, if they're wrong, you should make the case against the change.)

The form of the changes can vary, but i often find that a list of their concerns with code or test cases where it's helpful is the most useful format.

• in user.cs, the delete() method needs a userid parameter

  bool delete( string userid ){...}

• in user.cs, the add() method is fine but should be reformatted. See our style guide in the dev wiki.

• your XML elements and attributes are inconsistent. Please fix these and follow our naming conventions.

• log.cs needs to be rewritten and calls to it in all your code will need to change; it ignores failures, doesn't close files, etc. but more importantly, it doesn't use our standard logging components. Look at logging.cs in the blahblah component.

Answer 7

• only if you have unit tests for verification
• and you want to waste time typing when you're supposed to be reviewing
• and you want to do someone else's work for them

seriously, if i was reviewing code that needed to be refactored, i would suggest how it should be refactored so that person that wrote the code could learn by doing it themselves

so i say no, refactoring during a code review is not okay.