Zenn
Open13

コードレビュー観点まとめ

JavaScript
コードレビュー
𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖

強すぎたり、否定的だったりする言葉で指摘しない

〜はおかしい
→ 〜は違和感がある
→ この部分で〜という挙動になりそうに見える

〜のほうが良い
→ 〜のほうが良いと思う
→ 〜のほうが読みやすいと思う
→ 〜のほうが見通しが良いと思う
（指示ではなく、レビュアーの意見として受け入れてもらえるようにする）

𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖

コードの書き方の指摘はどう改善するかを併記する

〜のように書くほうが良いと思います
→ 〜のように書くとコードの見通しが良くなると思います
→ 〜のように書くと意図が伝わりやすくなると思います
→ 〜のように書くほうが一般的かなと思います

𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖

過剰な型指定

初期値から値が変動しない場合に、初期値と同じ型指定がされていないか。
const foo: number = 1 のような記述は @typescript-eslint/no-inferrable-types で警告を出せる。

let count: number = 0;
// ↓を勧める
let count = 0;

const fn = (age: number = 20) => age > 20;
// ↓を勧める
const fn = (age = 20) => age > 20;

const [state, setState] = useState<string>('');
// ↓を勧める
const [state, setState] = useState('');
𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖

Promise.all の誤用

同期実行されてしまったり、実行されなかったりする Promise.all がないか。
誤用しがちな Promise.all を引用して説明する。

await Promise.all([
  await asyncFn(),
  async () => {
    const { data } = await fetchData();
    return data;
  },
]);
// ↓を勧める
await Promise.all([
  asyncFn(),
  (async () => {
    const { data } = await fetchData();
    return data;
  })(),
]);
𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖

適切な配列操作

値を返すメソッドは返り値を使う場合のみ使う

値を返さなくて良い繰り返し処理には forEach を使う。

keys.map((key) => {
  delete obj[key];
});
// ↓を勧める
keys.forEach((key) => {
  delete obj[key];
});

存在するかを判定する場合は some / every を使う

返り値を値として使うかどうか、を明示するために存在するかだけが必要な場合は真偽値を返すメソッドを使う。

const item = arr.find((item) => item.exists);

if (item) { ... }
// ↓を勧める
const existsItem = arr.some((item) => item.exists);

if (existsItem) { ... }
𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖

ブロック外の配列に対しての破壊的な配列操作を避ける

ライブラリや前後のコードによって、破壊的な配列操作をすると挙動が変わることがあるので、ブロック内で定義された配列以外では快適な操作をしないようにする。

const [array, setArray] = useState([]);

const onSubmit = () => {
  array.push(0);
  array.splice(0, 1);
  setArr(array); 
};
// ↓を勧める
const onSubmit = () => {
  const newArray = array.concat(0).slice(0, 1);
  setAttr(newArra);
};

// ↓はブロック内で配列を定義しているので問題ない
const onSubmit = () => {
  const newArray = [];
  newArray.push(0);
  setArray(newArray);
};
𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖

（前提）コードの見た目は指摘しなくて良いように設定しておく

コードの見た目については、レビュー時にほとんど指摘が必要ない程度に Linter / Formatter で設定しておく。

自動修正されるルールについては積極的に設定して、自動修正でコードの見た目がだいたい揃うようにしておくとコードレビューの負担が相当下がる。
見た目についての指摘が多い場合は、その指摘をカバーしてくれるルールがないか調べる。

よく設定するルール

𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖

更新されたままの状態

メソッドが実行されたタイミングでローディング状態を更新する場合、エラー時や異常系処理時に状態を戻し忘れている箇所がないか確認する。
少なくとも、エラーの場合は状態を戻す必要があるケースが多いので注視する。

また、react-use/useAsyncFn で置き換えられないか検討する。

const onSubmit = async (): Promise<void> => {
  setIsLoading(true);
  await axios.post(...);
}
// ↓を勧める
const onSubmit = async (): Promise<void> => {
  try {
    setIsLoading(true);
    await axios.post(...);
  } catch {err) {
    setIsLoading(false);
    throw err;
  }
}
𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖

プロパティを変数に入れるときは分割代入を使う

プロパティを変数に入れる際に、プロパティ名とローカル変数の命名がずれることを防いだり気づきやすくするために分割代入になっていない箇所は修正する。
prefer-destructuring で警告を出せる。

const foo = params.foo;
const userId = user.id;
// ↓を勧める
const { foo } = params;
const { id: userId } = user.id;
𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖

プロパティ名を分割代入するときに意味が変わらないようにする

プロパティを分割代入する際に名前を変更する場合はプロパティ名をより具体的にする以外の命名がされないようにする。

たとえば、id を shopId にすることは問題ないが、 orderId を receiptNumber にはしないようにする。
実装側で命名がブレると修正時に混乱を招くため、後者の命名変更が必要な場合はなるべくデータソース側でプロパティ名を変えられるようにする。

特に気をつけている命名

  • id と number と code
  • time と date
  • shop と store
𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖

実装の意図が汲み取れないコードは意図を確認してから指摘する

コードから実装者の意図がわからない場合は、こちらの解釈や改善案を提示する前に実装者へ意図を確認してからにする。
実装者を試すためではなく、実装者の意図によって改善の促し方が変わるため確認している。

例えば、実装者の勘違いであれば修正を促すし、複雑に考えて難しい実装になってし待った場合は要件の解釈の仕方などを指摘するようにしている。

𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖𝕤𝕚𝕞𝕠𝕔𝕙𝕖𝕖

サーバーサイドでクライアント限定スクリプトが実行されていないか

ユニバーサルフレームワークでは、ランタイムがサーバーサイド・クライアントサイドのコードが混在するため、いずれかでしか使わないスクリプトが呼び出されていないことを確認する。
特に、型チェックなどでは検知できない部分なのでコードレビューで見つける必要がある。

例えば、 getServerSideProps() や useAsyncData() で dataLayer.push することはできない。