Project Planning – What to Do When Waiting for a Code Review

The idea is actually very nice. Contrary to common workflows, you keep the review directly in code, so technically, you don't need anything but text editor to use this workflow. The support in the IDE is nice too, especially the ability to display the list of reviews in the bottom.

There are still a few drawbacks:

• It works fine for very small teams, but larger teams will require tracking over what were reviewed, when, by whom and with which result. While you actually have this sort of tracking (version control allows to know all that), it's extremely difficult to use and search, and will often require a manual or semi-manual search through the revisions.

• I don't believe that the reviewer has enough feedback from the reviewee to know how the reviewed points were actually applied.

  Imagine the following situation. Alice is reviewing for the first time the code of Eric. She notices that Eric, a young developer, used the syntax which is not the most descriptive in the programming language actually used.

  Alice suggests an alternative syntax, and submits the code back to Eric. He rewrites the code using this alternative syntax that he believes understanding correctly, and removes the corresponding // BLA comment.

  The next week, she receives the code for the second review. Would she be able to actually remember that she made this remark during her first review, in order to see how Eric solved it?

  In a more formal review process, Alice could immediately see that she made a remark, and see the diff of the relevant code, in order to notice that Eric misunderstood the syntax she told him about.

• People are still people. I'm pretty sure that some of those comments will end up in production code, and some will remain as a garbage while being completely outdated.

  Of course, the same problem exists with any other comment; for example there are lots of TODO comments in production (including the most useful one: "TODO: Comment the code below."), and lots of comments which were not updated when the corresponding code was.

  For example, the original author of the code or the reviewer may leave, and the new developer would not understand what the review says, so the comment will remain forever, awaiting that somebody would be too courageous to wipe it out or to actually understand what it says.

• This does not replace a face-to-face review (but the same problem applies as well to any other more formal review which is not done face-to-face).

  Especially, if the original review requires explanation, the reviewer and the reviewee will start a sort of a discussion. Not only you will find yourself with large BLA comments, but those discussions will also pollute the log of the version control.

• You may also encounter minor issues with the syntax (which also exists for TODO comments). For example, what if a long "// BLA" comment spawns on several lines, and somebody decides to write it this way:

  // BLA This is a very long comment which is way beyond 80 characters, so it actually
  // fills more then one line. Since the next lines start with slashes, but not "BLA"
  // keyword, the IDE may not be able to show those lines, and display only the first one.
  

• And finally as a minor and very personal note: don't choose "BLA" as a keyword. It sounds ugly. ;)

I think most developers find themselves in this position at some point, and I hope that every developer who's felt victimized realizes how frustrating it will be when he or she becomes the senior and feels compelled to clean up code written by juniors.

For me, avoiding conflict in this situation comes down to two things:

1. Courtesy. Talking to someone about his/her code allows a dev to know that you're interested and you can discuss it as grown up professionals.

2. Forget about "code ownership" - the team owns the code. Other people wanting to make the changes is a good thing. If a senior dev makes changes that are "unreadable" or worse, then back them out. You don't need to be aggressive, just let an editor know that his/her changes didn't work, and you're more than happy to discuss your reversion.

Remember, team ownership of code is great and it cuts both ways. If you see something that doesn't make sense in someone else's code, then fix it. Being overly possessive and inadequately communicative is a surefire way to a create a poisonous development environment.