User:Sybren/Code Review Approach

= Sybren's Approach To Code Review =

This is a living document. While doing code reviews, I'm writing down some things that I seem to be doing a lot. It's my hope/intention that this can grow to be a nice "how to do code reviews" document at some point.

Patch Description
Reading the patch description:


 * 1) Is it clear which problem is being solved or feature is being added?
 * 2) Is it clear how this is done, and why this is the best way to solve it?

→ The above should not require actually reading the code. It should be possible to verify the code is doing the right thing by comparing it to the patch description. If this is not possible, the patch description is missing information.

→ See Ingredients of a Patch

Naming

 * Be wary of names that can be verbs or nouns. "Duplicate" is such an example -- does this mean the imperative "Hey, duplicate this thing!", the query "Is this a duplicate?", or the hint "Treat this as a duplicate of something"?

Boolean Parameters
Functions that take a boolean parameter often are hiding the fact that they're actually two functions in one. Older code even hides this further, by not having a `bool` type and using `int` instead. Here is an example:

This should be written as something like this (untested, just for illustration):