こんにちは、タビアン株式会社でエンジニアをやっている伊藤です。
2021年からタビアンに大学生エンジニアとして入社し、2年間のインターン期間を経て、2023年4月より新卒正社員として業務しています。現在はNuxt3と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から作らない
チーム内で似たような実装がされていないか、過去に類似チケットがないか事前に調べ、コードを書く前に準備しておきましょう。これもよくやってしまいがちですが、時間のロスになってしまいます。時間をかけて調べて実装した処理がすでに他の人によって実装されていたことを知ったときは落ち込みました…。
3. 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}`) // ←不要なら消しても良い
4. 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になる
セルフレビューの回数を増やし、実装のクオリティを担保しよう
今回は私が実際に指摘を受けた内容をまとめてみました。コードレビューは品質を高めるために非常に大切なものです。
しかし、人間である以上、問題のあるコードを見落としてしまうこともあるでしょう。それを防ぐためにもセルフレビューの回数を増やすことを推奨します。
以下のタイミングで自分が書いたコードを見直してみてください。
見落としていた問題点に気づく確率が上がると思います。
- コードを書き終わった後
- 変更点を確認するとき
- ステージングに追加するとき
- ステージした後
- コミットする前
- プッシュする前
- Pull Requestを作る前
- Pull Requestを作った後
ジュニアエンジニアは上司や先輩からコードレビューを受ける前に上記の内容をぜひ確認してみてください。
最後に、一緒に働いてみたいというエンジニアメンバーも絶賛募集中です！
同世代の若手エンジニアの方や、大学生でエンジニアインターンをやってみたいという方がいたら、是非一度お話しましょう！