Ambiguity

[wstomv]

There is an ambiguity in the first item under Implementation:

• Does this code change do what it is supposed to do?

I started parsing this sentence by interpreting 'change' as a verb (and then the following word 'do' makes no sense, and my first thought was that 'do' should be deleted). While writing this issue, I saw that you can also interpret 'change' as a noun. But this also does not make sense to me, because the change itself is not doing anything, the code as a whole is. I associate 'code doing something' with running the code, i.e., its execution; a code change cannot be executed in itself. But a code change can have a purpose, and that purpose can be accomplished/achieved.

Suggestion: Change the word 'do' into 'accomplish' or 'achieve' (twice).

It would be good for the reader of this list to understand the setting that you have in mind better. I started reading it with the review of a piece of (new) code in mind, rather than reviewing a 'code change' (a delta), where the reviewer is given two pieces of code (old and new/changed), the 'change' being the difference between old and new. Some items are explicitly formulated to apply to the second scenario, whereas others are more open-ended in their formulation.

This raises the question whether reviewing some code (without having some previous version to look at) and reviewing a code change are different things. You may wish to review the checklist with this in the back of your mind.

[mgreiler]

Great input. I fixed the wording in #3.
I have to set some time aside for thinking about how to address the nuances in reviewing complete change versus deltas.
Thank you for the contributions and your thoughts!

[wstomv]

A related concern is 'consistency of wording' in your checklist items. Now, I see the following words being used for very similar things (and to the reader it may not be clear if these are intended as synonyms, or whether you have subtle differences in mind):

• 'the/this code change'
• 'the/this (proposed) solution'
• 'the/this change'
• 'the/this code'
• 'would you have solved the problem'
• 'API', 'UI'
• 'it'
• no specific word for the artifact under review, i.e. it is implicit

So, more generally (and this is possibly an item for a meta-checklist): What is the 'subject under review'? (and how to refer to it in a checklist?)

• I think you agree that it is not the author, but rather the artifact authored. (The 'would you have solved the problem' gets pretty close to putting the author under review. You may wish to reword that one.)
• This artifact could be code (but more generally also data (including configuration data), models, designs, documentation), or possibly a change in such, or parts of such.
• W.r.t. code it may be useful to distinguish
  • documenting parts (like embedded javadoc)
  • declarative non-executable parts (like APIs, header files, interfaces)
  • modules, libraries, frameworks (i.e. incomplete code, that needs something else to execute)
  • code that can be executed in isolation (complete application)

You may wish to streamline the formulation. (But I can live with the idea that you just made available your checklist as a service for others to use 'as is', for inspiration so to say, and leave any streamlining to others.)

Questions for a meta-checklist for (code) reviews:

• Is it clear what exactly is being reviewed? (see above; what is the scope?)
• Does the artifact under review satisfy the entry conditions (is it ready for review)?
  • There could be a separate checklist for that: did you run a spell checker, did you run all test cases, did you run a static analysis tool, etc.
• Is required/useful related information made available to reviewers (reviewing bare code, without have some requirements/design document, may make little sense; or surrounding code)
• Is the purpose of the review clearly formulated? (e.g. a review could just be focused on performance or usability)