Code review process

gistfile1.txt

My process for (many) code reviews:

1) Make sure there are tests or test changes as may be necessary.
2) Check out the code.
3) Run all the tests and look for failures.
4) Run the code on a panel and check that the feature works (or bug is gone).
5) Revert the non-test code and re-run tests, looking for a failure.
6) Read the actual code and see if it makes sense.
7) Look for obvious bugs in the code.
8) Look for inefficiencies in the code.
9) Look for pitfalls in the code.
10) Look for other refactoring that should be done.

By "inefficiencies" I do not mean poor run-time performance. There are
very few cases where better asymptotic running time will make any
noticeable difference to the user. Instead, I'm looking for things
that will increase the cognitive load of future developers such as:

- New variables,
- New classes,
- New conditional trees or switches,
- New levels of indentation,
- Code moved to another unit or file,
- Use of design patterns,
- Introduction of a new tool, data format, or file type,
- New names.

In each case I think about whether this is truly necessary and whether
they can be lessened or removed entirely. Standard design patterns in
particular are almost always unnecessary so I usually think hard about
those.

By "pitfalls" I mean things that could get the code into trouble:
uncaught exceptions, unhandled error cases, missing conditional
clauses, unsynchronized data access, uninitialized variables,
unchecked bounds,

A very incomplete list of things in particular that I will look for:

* Heavy operations that can be done more succinctly (and usually better)
  by an existing standard or Maven-accessible library. Parsing,
  complex string handling, array processing,

* Using an numeric for loop (e.g. `for (int i = 0; i < data.length; i++) {
  ... data[i] ... }`) when a for-each would be clearer (and safer)
  (e.g. `for (String s : data) { ... s ... }`).

* Unused methods, statements, variables, classes, files, etc.

* Separation of tightly coupled code into multiple methods or classes.

* Combination of loosely coupled or unrelated code within the same
  method or class.

* Duplicate code.

And some questions I ask myself while looking at the code:

  - Can the new code be rewritten in terms of existing code?
  - Can any existing code be rewritten in terms of the new code?
  - Can the new code be done more simply?
  - Are there obvious bugs, inefficiencies or other pitfalls in the new
    code?

And one very important final check is to see if code coverage has
decreased.

-----