ジュニアエンジニア向け　セルフコードレビューの注意点

こんにちは、タビアンのエンジニア梁瀨（やなせ）です。前職は営業で、2022年5月にタビアンにエンジニアとして入社しました。現在はNuxt 3とAWS Amplifyでアプリ開発をしています。

長い時間をかけてコードを書いたにもかかわらず、たくさんの指摘事項をもらうとレビュアーの時間を割いていることもあり、プロジェクトの進行を遅れさせる原因にもなり得ます。今回はジュニアエンジニアがコードレビューの際に陥りがちなミスや指摘されがちな部分について、私自身の反省点や経験を踏まえ、まとめてみました。

指摘事項を減らし、スムーズなプロジェクト進行を目指すためにも、是非参考にしてみてください。

なお、この記事ではコード例をJavaScript/TypeScriptとVue.js（v3）で記述しています。

セルフレビューの注意点まとめ

1.コードの可読性を良くする

インデントが適切か

セルフレビューでインデントに注意する理由は、読みやすく保守性の高いコードを書くためです。レビュアーにとって、読みにくいコードをレビューするのは大きな負担となります。なのでレビュアーに自分が書いたコードを読んでもらう場合は、読み手に負担をかけないコードにする必要があります。

以下の悪い例をご覧ください。インデントが正しくないと、ブロック構造がわかりにくくなり、コードの読みやすさや保守性が低下します。

<悪い例>

<script setup>
const message = 'Hello, world!'
   </script>

<template>
   <div class="p-4">
       <h1 class="text-5xl">About page</h1>
    <p>
      {{ message }}
       </p>
        </div>
    </template>

<良い例>

<script setup>
const message = 'Hello, world!'
</script>

<template>
  <div class="p-4">
    <h1 class="text-5xl">About page</h1>
    <p>
      {{ message }}
    </p>
  </div>
</template>

適切に改行がされているか

こちらについても、レビュアーにコードを読んでもらうときに気を付けるポイントとなります。適切に改行をすることで、diffで差分が見やすい書き方をすることが重要です。

以下の悪い例を見てください。schoolYearだけを追加したにも関わらず行全体が変更扱いになり、どこが変わったのか分かりづらくなっています。

良い例のように適切に改行を行うと変更があった行のみが変更に上がってくるので変更内容が分かりやすくなります。

<悪い例>

<良い例>

コメントを入れることで、他の人が見てもコードの意味が分かるようになっているか

長い関数の場合、ぱっと見で処理内容を理解しづらいときがあります。

処理内容や使い方、何を引数に取るのかなどをコメントしておくことで、後でコードを見たときにコードを理解する速度が上がるので効率的です。

適切にコメントを残すことにより、レビュアーが処理内容を理解した上でコードを読むことができるので負担を軽減できます。

また、コードを別の場所からコピペした場合、後になって見返したときに何のコードだったかわからなくなってしまうことがあります。

少しでも「わかりづらいな」と思ったら自分自身が忘れたときのためにコメントを残す習慣をつけましょう。

<分かりにくい例>

※コメントはレビュアーが気になるポイントとして記載しています

// レビュアー目線：titleしか引数でもらわないのは正しいのかな？分からない
async function addTodo (title) {
  // レビュアー目線：これ何する関数だったっけ・・・await必要か？
  const item = await buildItem(title)

  const currentList = getTodoList()

  // レビュアー目線：あれ、なんで返り値もらってきてるんだ？？？
  const {
    list,
    index
  } = addItemToList(currentList, item)

  // レビュアー目線：あれ、前の関数呼び出しでリストに追加したんじゃないの？！
  list.splice(index, 0, item)

  await saveItem(item)

  // レビュアー目線：あれ、前の関数呼び出しと何が違うんだ？！
  await saveToDoList(list)

  // レビュアー目線：この1って何だよ！！！（怒
  return 1
}

<良い例>

/**
 * リストにアイテムを新規追加する
 *
 * タイトルだけ引数で渡して、説明文や期限などはダイアログで入力する。
 * データベースへ保存する。
 */
async function addTodo (title) {
  // ダイアログを表示して説明文や期限を入力する
  const item = await buildItem(title)

  // 現在表示されているリストを取得する
  const currentList = getTodoList()

  // 新規のアイテムを挿入する位置を決める
  const {
    list,
    index
  } = addItemToList(currentList, item)

  // 自分で配列に挿入する必要がある
  list.splice(index, 0, item)

  // DBに保存
  await saveItem(item)

  // 表示用に新しいリストを指定して再描画する
  await saveToDoList(list)

  // 増えたアイテム数を返す
  return 1
}

変数名や関数名がパッと見で意味が分かるものになっているか

これも意外と大事ですね。コードが長くなってくると読みづらくなってくるので、変数名や関数名は中身を見なくてもどういうものか想像できる名前にしておくと良いです。

以下の例ではある人物の情報をオブジェクト形式で定義しています。
悪い例の方でも特に問題はないのですが、良い例のように明確な名前をつけてあげることによって「あ、学生の情報か」とより具体的なイメージをしやすくなります。

<悪い例>

const people = {
  name: 'yanase kenta',
  age: 22,
  schoolYear: 4
}

<良い例>

const student = {
  name: 'yanase kenta',
  age: 22,
  schoolYear: 4
}

長いコードを適切に分割できているか

長いコードを適切に分割することによって、コードの可読性と保守性を向上させることができます。

悪い例のように1つの関数内でずらずらと処理を書いていると、処理内容を追いづらくなります。また、データをフィルタリングするコードを他の関数でも使用したい場合、悪い例のようにしてしまうと再利用できなくなってしまいます。

良い例のように処理を複数の関数へ適切に分割することにより、それぞれの関数を独立させてテストすることもできるため、可読性も保守性も向上します。

<悪い例>

function processData(data) {
  const filtered = data.filter((item) => item.isActive)
  const sorted = filtered.sort((a, b) => a.date - b.date)
  const formatted = sorted.map((item) => {
    return {
      id: item.id,
      date: new Date(item.date).toLocaleDateString(),
      name: item.name.toUpperCase(),
      amount: item.amount.toFixed(2),
    }
  })
  const totalAmount = formatted.reduce((acc, item) =>
    acc + parseFloat(item.amount)
    , 0)
  const result = {
    data: formatted,
    totalAmount: totalAmount.toFixed(2),
  }
  return result
}

<良い例>

function processData(data) {
  const filteredData = filterData(data)
  const sortedData = sortData(filteredData)
  const formattedData = formatData(sortedData)
  const result = calculateResult(formattedData)
  return result
}

function filterData(data) {
  const filtered = data.filter((item) => item.isActive)
  return filtered
}

function sortData(data) {
  const sorted = data.sort((a, b) => a.date - b.date)
  return sorted
}

function formatData(data) {
  const formatted = data.map((item) => {
    return {
      id: item.id,
      date: new Date(item.date).toLocaleDateString(),
      name: item.name.toUpperCase(),
      amount: item.amount.toFixed(2),
    }
  })
  return formatted
}

function calculateResult(data) {
  const totalAmount = data.reduce((acc, item) =>
    acc + parseFloat(item.amount), 0
  )
  const result = {
    data,
    totalAmount: totalAmount.toFixed(2),
  }
  return result
}

既存コードの書き方に倣い、プロジェクト内でのコードの書き方を統一することが可読性を高めるためにも大事なことです。私はよく我流のコードを書いてしまい、指摘されることがあるのでみなさんも注意しましょう。

まずは基本を押さえ、簡単なところから指摘事項を減らしていきましょう。
特に可読性に関する部分は基本的なところなので、ポイントを押さえてケアレスミスを防ぐようにしていきましょう。

2.その他

すでに他の人が作成した関数やコンポーネントが利用できる場合はそれを使い、似たコードを1から作らない

チーム内で似たような実装がされていないか、過去に類似チケットがないか事前に調べ、コードを書く前に準備しておきましょう。これもよくやってしまいがちですが、時間のロスになってしまいます。時間をかけて調べて実装した処理がすでに他の人によって実装されていたことを知ったときは落ち込みました…。

console.log / debuggerなどデバッグ用のコードは消す

デバッグ用のコードは、デバッグのために一時的に使われるもので、本番環境で実行する必要のない余分なコードです。本番環境でこれらのコードが残っていると、パフォーマンスが低下し、セキュリティ上の問題が発生する可能性があります。

また、コードを開発者以外の人が見た場合に、デバッグ用のコードがあると混乱する可能性があります。そのため、デバッグ用のコードは不要になったら削除することが望ましいです。

悪い例をご覧ください。console.logはデバッグの際には使っても良いのですが本番環境では不要となるため消しましょう。

また、以下の例では関数内と関数外のどちらも同じ出力結果となるので、残す場合でも片方で良いです。

<悪い例>

function calculatePrice(quantity: number, price: number) {
  console.log('calculatePrice', quantity, price)
  const totalPrice = quantity * price
  console.log('totalPrice', totalPrice)
  return totalPrice
}

const quantity = 10
const price = 5
const totalPrice = calculatePrice(quantity, price)
console.log(`Total price: ${totalPrice}`)

<良い例>

function calculatePrice(quantity: number, price: number) {
  const totalPrice = quantity * price
  return totalPrice
}
const quantity = 10
const price = 5
const totalPrice = calculatePrice(quantity, price)
console.log(`Total price: ${totalPrice}`)　// ←不要なら消しても良い

TypeScriptを使っているケースではanyやasを極力使わないように注意する

自分もTypeScriptを始めたばかりのときは全てanyで書きたいと思うことがしばしばありました。
逃げたくなる気持ちはわかりますが、これは不具合の原因になるのでやめましょう。悪い例をご覧ください。

multiplyはnumber型のnum1とnum2を引数にとり、合計値を返す関数になっています。

通常であればnumber型以外の引数は受け付けませんが、any型が使用されているためstring型の'2'や'3'が引数として渡されてしまいます。その結果、resultにはNaN（Not a Number）が設定され、関数が正しく機能しなくなります。

<悪い例>

function multiply(num1: any, num2: any) {
  return num1 * num2
}

let num1: any = '2'
let num2: any = '3'

let result = multiply(num1, num2) // エラーは発生しない

console.log(result) // 結果はNaNになる

<良い例>

function multiply(num1: number, num2: number) {
  return num1 * num2
}

let num1: number = 2
let num2: number = 3

let result = multiply(num1, num2)

console.log(result) // 結果は6になる