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:

void BKE_mesh_orco_verts_transform(Mesh *me, float (*orco)[3], int totvert, int invert)
{
  float loc[3], size[3];
  BKE_mesh_texspace_get(me->texcomesh ? me->texcomesh : me, loc, size);

  if (invert) {
    for (int a = 0; a < totvert; a++) {
      float *co = orco[a];
      madd_v3_v3v3v3(co, loc, co, size);
    }
  }
  else {
    for (int a = 0; a < totvert; a++) {
      float *co = orco[a];
      co[0] = (co[0] - loc[0]) / size[0];
      co[1] = (co[1] - loc[1]) / size[1];
      co[2] = (co[2] - loc[2]) / size[2];
    }
  }
}

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

void BKE_mesh_orco_verts_normalize(Mesh *me, float (*orco)[3], int totvert)
{
  float loc[3], size[3];
  BKE_mesh_texspace_get(me->texcomesh ? me->texcomesh : me, loc, size);
  
  for (int a = 0; a < totvert; a++) {
    float *co = orco[a];
    co[0] = (co[0] - loc[0]) / size[0];
    co[1] = (co[1] - loc[1]) / size[1];
    co[2] = (co[2] - loc[2]) / size[2];
  }
}
void BKE_mesh_orco_verts_denormalize(Mesh *me, float (*orco)[3], int totvert)
{
  float loc[3], size[3];
  BKE_mesh_texspace_get(me->texcomesh ? me->texcomesh : me, loc, size);

  for (int a = 0; a < totvert; a++) {
    float *co = orco[a];
    madd_v3_v3v3v3(co, loc, co, size);
  }
}