Point of code review (Basic)

overview

When I review the code at work, I follow the flow below.

Develop locally by the implementer.
Push the code to GitHub.
Create a Pull Request (PR).
Assign to the reviewer.
Review and add some comments.
The implementer replies with a comment or pushes modified code.
4. Return to
When there are no comments in the review, it is merged and the work is completed.

I wrote a basic "code review point" in this article.
Because I want the implementer to check it myself before requesting a review.
That makes improving the accuracy of the code review.

For more details, please read O'Reilly's "The Art of Readable Code".

Write PR description

Describe the reason for code changes and overview in the "Summary" of the PR.
At a minimum, issues and ticket links that triggered the change are required.

detail

It's a pain to read through the code without knowing any overview of what the changes are.
By reading the "Summary", the reviewer can imagine the code difference and easy to review.

TODO comment

Leave "TODO comments" if you have uncompleted tasks in the PR.

detail

Ideally, one PR should complete one "task".

However, for various reasons during development, there will be situations like "I want you to review it in the middle".
At that time, it is difficult for the reviewer to read while understanding "what is done and what is not done".
TODO comments in the code help it.

In addition, the editor or IDE has a function that lists "TODO comments", which is also effective for organizing remaining issues later.

good

function foo() {
  // TODO: foo API is not implemented. So I return dummy data.
  return {};
}

Delete comment out

Delete physically the code that is temporarily written for debugging or that is to be deleted. (unless there is a special reason.)

detail

We are developing with history management on GitHub.
We can search deletion history by PR search and git blame.
So you should remove any code you no longer need.

If a lot of useless code remains as a comment, it will be noisy when searching for the code, and the code will be difficult to read.

Early return

Make good use of the return statement.

detail

When reading the code, we basically read from top to bottom.
However, if there is an if statement, we may read the contents after knowing "whether else-if continues, and what we are doing after they are finished".
If "actually, the content of the function was only an if statement", it would be a waste of time on the reviewer.

It will be easier to read if the code has a return statement and shows it clearly to exit the function.

bad

function some_long_method() {
  if (foo) {                // 1. find if block. search end of the block.
    // some long process    // 4. read inside the block.
    if (bar) {
      return a;
    } else if (baz) {
      return b;
    }
  }                         // 2. find end block. read next line.
  return null;              // 3. find "return". So I know "if !foo, return null only". So scroll up to read inside the block.
}

good

function some_long_method() {
  if (!foo) {           // 1. find if block. Also find next "return" line.
    return null;
  }
  // some long process  // 2. read the process.
  if (bar) {
    return a;
  }
  if (baz) {
    return b;
  }
  return null;
}

boolean variable naming

Name the boolean variable name as isXXX so that it can be read like an English sentence such as {class name} is XXX.

detail

For example, if the variable name is isUplaod, it is "be verb + verb".
So it cannot be read like an English sentence.
If it is isUplaoding, it can be read as English like "{The class} is uploading".

However, boolean types do not necessarily have to be in the form isXXX.
canUpload and hasChildren are good names because you can understand "when the value is true".

Commonization

Extract methods that are likely to be used on other screens of the system, so that they can be used in common.
Such as tax-excluded / tax-included amount calculation and special date formats.

detail

Some calculations (such as the amount excluding tax) don't change for each process.
And, the same calculation should be performed the same work through the system.
If such calculations are scattered throughout the code, it is difficult to find and correct when the calculation is wrong or when the requirements change.
Therefore, the same process should be cut out into a single function and shared.

However, there are some cautions.
Don't be commonize the processes that are "accidentally" matched.
For example, the calculation of "the game score multiplied by 1.1" and "the calculation of the tax-included price with a tax rate of 10%" should not be commonized even at the same 1.1 times.

Reduce "state"

Reduce "states" as much as possible.
Calculate values that can be calculated from other "states".

detail

You can give a class a "state" just by writing let foo;.
However, if you have it as a "state", you have to think about various things.

When will it be initialized?
When will it be updated?
What is the value when this function is called?
What value can be taken when another "state" is a specific value?

The more "states" you manage, the harder it is to read the code and the more causes to bug.
Calculate the value that can be calculated from other "states" by calculation.

bad

let hasError = false;
let errorMessage = null;

function setError(newMessage) {
  hasError = true;
  errorMessage = newMessage;
}

function printError() {
  if (!hasError) {
    return;
  }
  console.log('message is ' + errorMessage);
}

good

let errorMessage = null;
function hasError() {
  return errorMessage != null;
}

function setError(newMessage) {
  errorMessage = newMessage;
}

function printError() {
  if (!hasError()) {
    return;
  }
  console.log('message is ' + errorMessage);
}

Manage const values (even if the values havn't been determined)

Even if the information is not determined at the time of implementation, make constant according to the meaning.

detail

bad

function method1() {
  location.href = ""; // TODO: Change value if tems URL is fixed.
}
function method2() {
  location.href = ""; // TODO: Change value if tems URL is fixed.
}

good

// TODO: Change value if tems URL is fixed.
const TERMS_URL = "";

function method1() {
  location.href = TERMS_URL;
}
function method2() {
  location.href = TERMS_URL;
}

Make visibility as low as possible

Keep unnecessary things out of sight.

detail

If unnecessary functions/values have "export", it will be difficult to understand "where is it used" and "what is the range of side effects if the contents are changed".
Don't "export" it now even if it "may be used in the future".

Similarly, when declaring variables, try to reduce the number of lines that can be accessible.

Read your changes again

detail

Read the diffs again before requesting a PR review.
If you look at GitHub in your browser, you'll see a tab called "Files changed" that makes it easy to see the differences that are included in the PR.

In particular, check the following points.

Are not there any changes you didn't intend? (e.g. Accidentally touched the keyboard. Changes that are not related to PR.)
Are there all changes you intended?
Does the difference match what you wrote in the PR overview?

Finally

It is tiring to point out small details or point out a simple mistake.
Let's make our code review meaningful by being careful at the time of implementation and before review.