どうもSuです。
皆さんは、コードレビューはしますか？
コードレビューは、チームがより高品質なコードを生み出す手助けをするために重要なプロセスです。今日はコードレビューで大切な観点を紹介します。効果的なコードレビューを実施して、より良いコードを作っていきましょう！
ちなみに、
コードレビューで品質は担保できません！
いくらコードを見て「このコードは大丈夫！」となったとしても、それでテストが不要になる、品質が担保できるわけではありません。
もちろん、コードで分かるような簡単なバグは見つけることはできます。ただし、最終的な品質担保はテストで行わないといけません。その点は頭に入れておきましょう！
コードレビューで大事な事
- 可読性
- 適切なコメント
- コードの一貫性
- DRY（Don’t Repeat Yourself）
- モジュール性
- エラーハンドリング
- テスト
- セキュリティ
それでは1つ1つ見ていきましょう！
可読性
可読性とは、コードは他の開発者が理解しやすいようにすることです。他の開発者には「未来の自分」が含まれます。簡潔で明確な命名を行い、整理された構造にすることです。
例えば、変数名が分かりづらい場合、コードを読んで理解するのに時間がかかります。例えばこんなコードです。
function calc(a, b) {
return a + ( a * b / 100 );
}
console.log(calc());意味が分かりましたか？
これは、値段と消費税率を渡して、税込金額を返す関数です。もちろん書いた人は理解できているでしょう。しかし、他の人や1年後に自分が見たら意味が分からないかもしれません。
コメントを書くことで説明することはできますが、まずは「コードだけで理解しやすくできないか？」を考えましょう。変数名、関数名で意味がわかるようにすることで、文章を読むように理解することができます。
単に関数の説明をするだけのコメントは、保守性が悪いです。関数を修正した時にコメントも一緒に直す手間があるからです。関数を修正した時、コメントを修正し忘れていると最悪の状況になりますね。
function calcPriceIncludingTax(price, taxRate) {
return price + ( price * taxRate / 100 );
}
console.log(calcPriceIncludingTax());様々な書籍や記事で言われるように「コードは書く時間よりも見る時間の方が多い」です。読む労力を減らすためにも分かりやすいコードになるように努めましょう。
分かりやすいコードはレビューする人の負担も減らしますからね。
適切なコメント
コード内には適切なコメントが必要です。特に、複雑なロジックや処理には説明が必要です。
先ほど言った通り単に説明だけするコメントは避けましょう。コメントの目的は、「コードを読む時の理解を助けるため」です。
複雑なロジックでは、コメントがあると読むのが楽になるでしょう。他のコードと違って特殊な書き方としている場合は、その理由を書いていると理解しやすくなると思います。
また、複雑なロジックに対するコメントを書いている場合は、「この複雑なロジックでないとダメなのか？」、「複雑なロジックを簡単にできないか？」、「ロジックを分割して、シンプルにできないか？」などを検討する良いタイミングになります。複雑なロジックしている場合、実はあまり考えずに実装してしまっていて、複雑になっているケースもあります。コードを書いた人に理由を聞いてみて、改善できないか検討すると良いです。
他にも、TODOコメントはよく使われます。
現状だと動くコードではあるけど、リファクタリングした方が可読性や保守性が上がることが分かっているのであれば、TODOコメントを残しておいて、次の修正の時にリファクタリングがスムーズにできるようにしておくのは良いアイディアです。
コードの一貫性
プロジェクト全体で一貫したコーディングスタイルや規約を適用してください。これには、インデントやスペース、命名規則などが含まれます。
一昔前は、コーディングスタイル規約書といった文書があり、それを開発者が参照しながら書いていたなんてこともあります。もちろん、コードレビューでチェックされる箇所ではありますが、近年は、コーディングスタイルのチェック自動化によって、その作業はなくなっていると言っていいでしょう。もし、チェック自動化ができていないのであれば、Linterによるコーディングスタイルの自動チェック＋自動整形を導入するのがおすすめです。
コーディングスタイルの自動チェックができるようになれば、開発者はすぐにコーディング規約に違反していることに気づけるので、レビュー時にコーディング規約違反になっていることは少ないです。また、レビュアーはコーディング規約の観点でコードを見る必要がなくなります。その結果コードレビューの効率化につながります。
DRY（Don’t Repeat Yourself）
重複したコードや機能はなるべく避け、共通の関数やクラスを利用して、コードの再利用性を高めましょう。これは有名な原則ですね。
「リーダブルコード」という書籍にもDRY原則について書かれています。
コードを書く人にとって、必須の書籍だと思いますので、読まれていない方は、ぜひ読んでみてください。
ちなみに、テストコードを書く時は、DRYにしない方が良いことが多いです。今回は、詳しくは書きませんが、頭に入れておくといいでしょう。
モジュール性
コードは複数の小さなモジュールに分割されており、各モジュールは一つの機能や責任を持つことが望ましいです。
「関数名が長い」、「関数の行数が長い」場合、1つの関数の責務が大きい可能性があります。1つの関数にやることを詰めすぎると、複雑化しやすいです。
例えば、以下の関数は責務が多すぎて複雑化していますね。こんな時は、「ユーザデータだけを取る関数」、「ユーザデータをもとに更新する関数」、「性・名を半角スペースで結合して返す関数」のように分けてみましょう。関数がすっきりして見通しが良くなりますし、関数単位でのテストも非常にやりやすくなります。
function getUserAndUpdateUser(firstName: string, lastName: string) {
const user = getUserData();
user.firstName = firstName;
user.lastName = lastName;
updateUser(user);
return `${firstName} ${lastName}`;
}エラーハンドリング
エラーハンドリングは、アプリケーションを正常に動作させるために非常に重要なポイントです。
try-catch文は頻出する構文ですが、catchした時の処理は適切でしょうか？意外とcatch文の中はコードレビュー時にテストされていないことが多い印象です。
実際にテストをやってみたら、「catch文の中でエラーが発生していて、うまく処理できていなかった」ということも良くあります。
「エラーハンドリングの目的は？」、「そもそもこのエラーハンドリングいるの？」、「エラーが発生した場合に期待する動作になっている？」、「エラーハンドリングした時は適切なログが出ている？」と言った観点でエラーハンドリング部分を見ると、より今後の開発や運用に役に立つ観点を得られると思います。
テスト
テストの視点はとても重要です。ポイントとしては、「この関数のテストコード書くとしたら、簡単に書けるかな？」です。
簡単に書けないということであれば、「処理が複雑で、処理を分割した方が良い」、「内部生成している情報を外部から渡せるようにした方が良い」といった別な改善点が出てくるでしょう。
セキュリティ
セキュリティの観点は様々ありますが、まずは「不要な情報を返していないか？」ということが1つです。例えばサーバサイドで扱っている情報の中には、「クライアントに返してはいけない機密情報」や「クライアントに返す場合はハッシュ化しないといけない」と言った内容です。
機密情報はアプリケーションからの漏洩は決して行わないように注意しましょう。もちろん、APIなど設計時に考慮できているのが良いのは言うまでもありません。
最後に
コードレビューは奥が深いです。まだまだ語れないこともたくさんありますので、また今度、続きを書きたいなと思います。
コードレビューは、コミュニケーションの1つです。大事な事は、「お互いに尊敬をもってコードレビューのやり取りをしよう！」ということです。コードを書いているのは人間で、意識せずとも「書いたコードは自分であり、指摘されると自分が傷つけられたような気がする」は心理としては間違ってはないと思います。
ヘタすれば言葉の暴力になりかねないコードレビューですが、うまく活用して、知識の共有や円滑なチームコミュニケーションができると良いですね！