Peeter Joot's (OLD) Blog.

Math, physics, perl, and programming obscurity.

Emails with code review requests.

Posted by peeterjoot on December 13, 2011

We have email distribution lists for various code components in our product and requests for review end up being sent to these lists.

Twice this week I’ve sent emails in response to code reviews along the lines of:

“Please re-send your request with enough information to give the potential reviewer a clue what they should be looking at.”

It does not appear obvious to some people to do this. Our distribution lists is serviced by a number of possible people, with one person selected as the “victim of the week” to own answering the email and/or doing whatever defect work ends up generated by that email if any.

For review requests, it may be that somebody who knows the code better than the person monitoring the list for the week can review, but if you don’t describe the change in more detail then that possible “better reviewer” would never know to look.

Leaving half the work to the reader of the email that can be easily justified in a number of circumstances. However, if your email is distributed, then an attempt to save a bit of time in drafting it, means that all the readers incur the costs. The cost of email servicing can really add up, so make them count.

A review request should include the following

  • Defect # and abstract if appropriate.
  • High level description of what’s being changed, and why.
  • Filenames and function names of what is being changed. If this is a lot then details can be provided outside of the email.
  • Context diffs (for example those generated by (gnu) ‘diff -p -U10’. Many people have preferences on how they compare files, so also provide the new and original versions of the files. Do not attach these to the email, but include a path to where these are available.
  • Instead of providing copies of the files, it may make sense to provide information about the version control sandbox that contains the files. Do this in an effective way ((*) see below).

(*) Perhaps unique to clearcase, there can be bad ways to share information about your changes. It is common for people to share the clearcase branch name that they’ve used. Also provide the viewname (if that view is accessible to your reviewer), and the config spec (assuming your view is not accessible to the reviewer). What your changed files are branched from many not be obvious, and these may not even be visible to the reviewer in their view, if a specific version of the directory element is required for the file to be visible (i.e.: when you have created a new file).


2 Responses to “Emails with code review requests.”

  1. Robin said

    I always and only do reviews via sharing cspec. This method always allows you to see new files if you do things properly (read: create a dynamic view using the cspec or apply the cspec to an existing dynamic view and ctsetview into it) since the cspec will select the new directory version that contains the new file. Unfortunately there are still some people that I send reviews to that ignore my cspec and just try to diff files using the branch name in the cspec, which of course will not work when I have added a new file. There is a cryptic way around that though which sadly is not present in any of our diff scripts that I know of. You can diff newly files using this kind of path assuming the new file lives in the sqp directory: /vbs/engn/sqp@@/my/branch/for/my/change/LATEST/mynewfile.C@@/my/branch/for/my/change/LATEST. This approach will use the new version of the directory that you created in order to add the new file, and this version of the directory will select the new file so that you can view it.

    • peeterjoot said

      I agree that cspecs are the way to go, but a lot of people are resistant to learning, even the relatively simple task of how to create a view using one, and I’ve found it easiest to provide alternate means as well.

Leave a Reply

Fill in your details below or click an icon to log in: Logo

You are commenting using your account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )


Connecting to %s

%d bloggers like this: