こんにちは！セーフィー株式会社のサービス開発部 モバイルチームの北本です。 主にSafie ViewerのiOSアプリの開発を担当しています。
今回は、リーダブルコードを読んで、プルリクレビュー改善に活用する話をしたいと思います。
なぜリーダブルコードを読むのか
セーフィーモバイルチームは現在、チームビルディングを積極的に進めています。 ある日のミーティング時に、コードレビューに関して、以下のやりとりがありました。
- シニアエンジニアが若手エンジニアのプルリクエスト（以下PR）にどの範囲まで指摘すればいいかわからない
- 若手エンジニアがシニアエンジニアのPRに何をレビューすればいいかわからない
そこで、チーム開発において良いコードの書き方の基準を定めようということになり、 モバイルチーム全体でコードの理想状態を議論した結果、チームの生産性を高めるコード≒可読性の高いコードとして理想状態を定義し、リーダブルコードをPRのレビュー指針のベースとして取り入れようということを決めました。
今回の記事ではリーダブルコード（第22版）を「チームに取り入れられるレビュー指針はあるか」といった目線で読み返し、議論の叩きとして、レビュー観点をチェック項目形式でまとめた物を共有します！
リーダブルコードとは？
「エンジニアが読むべき本」「エンジニアが影響を受けた本」と検索すると必ずといっていいほどトップに出てくるのがこの本『リーダブルコード ――より良いコードを書くためのシンプルで実践的なテクニック』です。副題の通りより良いコード、読みやすいコードを書くための実践的なテクニックが変数の命名からロジック、テストまで章ごとに体系的に説明されています。皆さんのチームでも、エンジニア同士が可読性の高いコードを説明する上で共通言語として会話に登場することもあるのではないのでしょうか？
この記事では、現場での活用例の共有として、役立てていただければと思います。「なぜこのルールが読みやすいコードに繋がるのだろう？」と気になった方はぜひこの記事をきっかけにリーダブルコードをお読みいただければと思います。
リーダブルコードに書かれていたこと
セーフィーのチーム開発で取り入れることができそうな記述を抜粋し、「どういった観点でレビューすれば良いか」といった視点でまとめます。この記事はあくまで「チームのカルチャーにフィットさせるための議論の叩き」という立場なので現実的なルールの粒度かどうかの観点は考慮していないことにご注意ください。
理解しやすいコードとは？
（1章 理解しやすいコード） リーダブルコードでは以下を著書の目的として定義しています。
（引用 まえがき）
本書の目的は、読みやすいコードを書くことである。その中心となるのは、コードは理解しやすくなければいけないという考えだ。具体的に言えば、誰かが君のコードを読んで理解する時間を最短にするということだ。
また、コーディングには、コードの効率化、設計、テスタブルといった様々な側面がありますが、この章では「コードを読みやすくするという視点はその他の目標と競合しない、高度に最適化されたコードであってももっと理解しやすくできる」といった点も言及されています。iOSの開発では、用いられているSwiftでは高機能なコンパイラによる最適化も施されるため、コードの可読性の追求という点においてトレードオフになる要素はゼロといって良さそうです。
ひとまずセーフィーモバイルチームでも、「レビュワーが理解しやすくするには？」といった視点に立ち、メンバーのコードを理解するまでにかかる時間を最短にすることで、チームの生産性の向上を目指していきたいと思います。
次章以降で具体的なテクニックに関して説明されています。
表面上の改善（第Ⅰ部）
命名規則について
（2章 名前に情報を詰め込む、3章 誤解されない名前）
（引用 p.10）
明確な単語を選ぶ
「名前に情報を詰め込む」には、明確な単語を選ばなければいけない。
getやsizeなどの汎用的な単語を変数名、関数名で避け、明確な単語を選ぶべき理由が説明されています。
- 変数名、関数名に汎用的な単語が使われてないか？明確な単語に置き換えることはできないか？
- 役割が理解できない変数名はなかったか？
（引用 p.19）
名前に情報を追加する
名前は短いコメントのようなものだ。変数名に詰め込める情報はあまり多くない。だけど、名前につけた情報は変数を見るたびに目に入ってくる。
また、時間、サイズといった値の単位や、危険や注意喚起のための情報を変数名に追加するためのテクニックが説明されています。また、「変数のスコープが小さい場合は短い名前でも良いが、エディタには単語補完があるので避ける理由にはならない」といったことにも 触れています。
- 命名に利用されている単語は曖昧すぎないか？情報は十分か？
ここでは「命名のフォーマット規約」についても触れられていますが、iOSの開発に関して言えば、Swiftの公式ドキュメントにあるAPI Design Guidelineや、Apple公式のフレームワークがあるので、こちらに寄せていくことでより明瞭な命名ができそうです。
- プログラミング言語の規約や、公式ライブラリの命名規則に乗っ取った命名ができているか？
コメントについて
（5章 コメントすべきことを知る、6章 コメントは正確で簡潔に）
（引用 p.69〜70）
コメントすべきでは「ない」こと：
・コードからすぐに抽出できること。
・ひどいコード（例えば、ひどい名前の関数）を補う「補助的なコメント」。
コメントを書くのではなくコードを修正する。
記録すべき自分の考え：
・なぜコードが他のやり方ではなくこうなっているのか（「監督コメンタリー」）。
・コードの欠陥をTODO: やXXX: などの記法を使って示す。
・定数の値にまつわる「背景」。
読み手の立場になって考える：
・コードを読んだ人が「えっ？」と思うところを予想してコメントをつける。
・平均的な読み手が驚くような動作は文書化しておく。
・ファイルやクラスには「全体像」のコメントを書く。
・読み手が細部に捕われないように、コードブロックにコメントをつけて概要をまとめる。
この章で「価値のあるコメント」と呼ばれていた、「実装の背景、ToDo」を書く文化は既にあるので守っていきたいです。 ただ、複雑な処理を行う実装でも、レビュアーがコメントを読んでいて実装内容が明確にわかるか、びっくりしたことがないか、を意識する必要がありそうです。 また、読み手の立場になって考えることの重要性が書かれていましたが、コードレビューにおいては、明確に読み手視点でコメントの改善を促すことができます。本人では気付きづらい点をレビュワーが「コメントの内容が理解できたか」の観点で改善を促していくのは効果的だと感じました。
- コードからすぐに抽出できること、命名で補えることをコメントしていないか？
- コメント内容をコードの修正で補えないか？
- なぜ他のやり方ではなくこうなっているのかわからない箇所はあるか？
- TODO: やFIXME: , HACK: などの記法を使って示す箇所はあるか？
- 「背景」がわからない、動かすことのできない定数の値はないか？
- レビュワーが「びっくりしたこと」がなかったか？
- 自明でない挙動、動作について記述したコードはないか？
- ファイルやクラスに「全体像」に関するコメントが書かれているか？
- コードブロックに概要がコメントによって整理されているか？
ループとロジックの単純化（第Ⅱ部）
関数から早く返す、ネストを浅くする
（７章 制御フローを読みやすくする）
（引用 p.97〜98 一部省略）
比較（while (bytes_expected > bytes_received)）を書くときには、変化する値を左に、より安定した値を右に配置する（while (bytes_received < bytes_expected)）。
if/else文のブロックは適切に並べ替える。一般的には、肯定系・単純・目立つものを先に処理する。〔中略〕
ネストしているとコードを追うのに集中力が必要になる。ネストが増えるたびに「スタックにプッシュ」することが増える。深いネストを避けるには「直接的」なコードを選択する。
早めに返してあげると、ネストを削除したりコードをクリーンにしたりできる。特に「ガード節」（関数の上部で単純な条件を先に処理するもの）が便利だ。
私が普段使っているSwiftでは非同期な処理をクロージャを使ってネストして書くことが多いので、なるべくネストを浅くする工夫が必要になってくると感じました。 また、推奨されていたガード節に関して、言語仕様によってはguard文があり、returnを強制することができるので積極的に使っていく方針が取れそうです。
- 比較条件では、変化する値が左に来ているか？
- if/else文のブロックは肯定系・単純・目立つものを先に処理されているか？
- クロージャ、if文等のネストを浅くする手段はないか？
- guard文を利用して改善できる箇所はないか？
変数を用いて分割する
（8章 巨大な式を分割する）
（引用 p.108 一部省略）
最も簡単な方法は「説明変数」を導入することだ。大きな式の値を保持する説明変数には、３つの利点がある。
・巨大な式を分割できる。
・簡潔な名前で式を説明することで、コードを文書化できる。
・コードの主要な「概念」を読み手が認識しやすくなる。
その他には、ド・モルガンの法則を使ってロジックを操作する手法がある。〔中略〕
本章で取り上げた全ての改善コードには、if文の中身が２行以上含まれていない。これは理想的な状況だ。同じことが常にできるとは限らない。そんなときは、問題を「否定」したり、反対のことを考えてみたりすることが必要になる。
レビューにおいては、レビュワーが直感的に複雑な条件や、わからなかった計算があった場合にそれを指摘し、指摘があったレビュイーが「説明変数」、「要約変数」の概念を使って解決するといった方針が取れそうです。 「改善コードのif文の中身が２行以上含まれていない」といった状況を理想的と書かれていましたが、Swiftでは変数のオプショナルラップに利用されており、言語仕様によっては難しい場合もありそうです。
- わからなかった計算はなかったか？
- 複雑な式を説明変数を用いて分割できないか？
- ド・モルガンの法則を利用して多重括弧を削ることができないか？
- 複雑な文の条件を否定、「反対を考える」といった手法で単純化できないか？
変数のスコープと変更
（ 9章 変数と読みやすさ）
（引用 p.126）
変数を減らして、できるだけ「軽量」にすれば、コードは読みやすくなる。具体的には、
・邪魔な変数を削除する。ーー本章では、結果をすぐに使って、「中間状態」の変数を削除する例を示した。
・変数のスコープをできるだけ小さくする。ーー変数を数行のコードからしか見えない位置に移動する。
・一度だけ書き込む変数を使う。ーー変数に一度だけ値を設定すれば（あるいは、constやfinalなどのイミュータブルにする方法を使えば）、コードが理解しやすくなる。
Swiftで利用するXcodeでは、利用されていない変数や、値の再代入のないvarの変数宣言などは警告をしてくれるのでありがたいです。 また、本書の例のようにロジックを整理することで変数そのものを消したり、varの宣言をletに変えられることもありそうです。
さらに「変数のスコープを小さくするために、コード行数をできるだけ減らす」という観点は比較的指摘がしやすく、実際のPRでも使っていきたいです。
- 外部から参照されていない変数、関数はprivateになっているか？
- 継承されていないclassはfinalになっているか？
- 宣言をイミュータブル（let）に変更できる変数はないか？
- 変数は利用する直前で宣言され、利用できる範囲が最小化されているか？
コードの再編成（第Ⅲ部）
無関係な下位問題を抽出する
（10章 無関係の下位問題を抽出する）
（引用 p.130）
本性のアドバイスは、無関係の下位問題を積極的に見つけて抽出することだ。ぼくたちは以下のことを考えている。
1. 関数やコードブロックを見て「このコードの高レベルの目標は何か？」と自問する。
2. コードの各行に対して「高レベルの目標に直接的に効果があるのか？ あるいは、無関係の下位問題を解決しているのか？」と自問する。
3. 無関係の下位問題を解決しているコードが相当量あれば、それらを抽出して別の関数にする。
（引用 p.141）
本章を簡単にまとめると、プロジェクト固有のコードから汎用コードを分離するということだ。ほとんどのコードは汎用化できる。一般的な問題を解決するライブラリやヘルパー関数を作っていけば、プログラムに固有の小さな核だけが残る。
この章では、「プログラムに固有の小さな核」＝その関数、コードブロックの関心事のみを残し、それ以外は別の関数やクラスに切り出して汎用化すべきだということが書かれていました。
- 関数、コードブロックの核は明確化されているか？別の下位問題を扱ってないか？
- 汎用コードとして切り出せるロジックはないか？
また、既存のインターフェース部分の「汎用コード」を自前のextensionを作って簡潔にすることも意識していきたいです。
- ライブラリのインターフェースが複雑な部分から汎用的コードは作れないか？
一度にひとつのことを
（11章 一度にひとつのことを）
（引用 p.155）
読みにくいコードがあれば、そこで行われているタスクを全て列挙する。そこは別の関数（やクラス）に分割できるタスクがあるだろう。それ以外は、関数の論理的な「段落」になる。タスクをどのように分割するかよりも、分割するということが大切なのだ。
この章を読んでPR内の実装が読みにくい場合、一度に複数のことを並行して行っている可能性が高いと感じました。 その場合、読みにくいことをレビュワーが伝え、レビュイーにタスクを全て列挙してもらう、別のクラス・関数に処理への分割を提案するといったアクションが取れそうです。
- 読みづらい関数やロジックがあった場合、別のクラス・関数に、分割できないか？
ロジックを明確に説明できるか？
（12章 コードに思いをこめる）
（引用 p.165 一部省略）
本章では、プログラムのことを簡単な言葉で説明する技法について説明した。説明することでコードがより自然になっていく。この技法は思っているよりも簡単だが非常に強力だ。説明で使っている単語やフレーズをよく見れば、分割する下位問題がどこにあるかがわかる。
〔中略〕
問題や設計をうまく言葉で説明できないのであれば、何かを見落としているか、詳細が明確になってないということだ。
複雑な考えを伝える時に、自分よりも知識が少ない人に簡単な言葉で説明する能力が大事といった内容でした。
PRにおいては、概要や、解決する問題、解決手段をレビュアーに説明する必要があるため、「ロジックを明確に説明できるか？」という観点はそのまま利用できそうだと感じました。PRの内容を概要で説明できること、コミットメッセージが簡潔にまとまっていることへの意識は重要そうです。
- PRの内容が概要に簡潔に説明されていて、レビュアーは意図がわかるか？
また、一度に複数の修正を含んでしまったPRは簡潔な説明にならないはずなので、レビュイーの意識として - PRのコミットを細かい目的ごとに分割する - リファクタリングと不具合修正のPRを分ける ということも必要になってくるはずです。
- コミットの単位は適切か？
- 一つのPRに関して目的が一つに定まっているか？
短いコードを書く
（13章 短いコードを書く）
（引用 p.175）
本章では、できるだけコードを書かないことについて説明した。新しいコードには、テストや文書や保守が必要になる。また、コードが増えると「重く」なるし、開発も難しくなる。
新しいコードを書かないようにするには、
・不必要な機能をプロダクトから削除する。過剰な機能は持たせない。
・最も簡単に問題を解決できるような要求を考える。
・定期的にすべてのAPIを読んで、標準ライブラリに慣れ親しんでおく。
短いコードを書くためのテクニック、意識が紹介されていました。 基本的に同意できる内容だったのでチェック項目だけまとめました。
- 必要のない、利用されていない過剰なロジック、コードがないか？
- 変更・修正の方針が正しいか？
- 問題をもっと簡単に解決できないか？
- 今回使われなくなったコードはちゃんと削除しているか？
- ライブラリ、既にあるロジックを使って解決できないか？（処理が重複してないか？）
さいごに
本書の「読みやすさ」はチームにおいて、コードを分割すべきかどうかの判断基準としては良い共通言語になると思います。私自身も、この記事を書きつつ日々のコードの質や、レビューの観点、コーディングにおける価値観にどんどん影響を受けていくのを感じています。 また、この本の初版は2012年です。10年近く経っているにもかかわらず、現代のエンジニアからも変わらない評価を得続けていることからも「コードは理解しやすくなければならない」という価値観の正しさと汎用性を感じます。私たちセーフィーモバイルチームのように方針や、共通言語として本書を利用するという手法もぜひお勧めです。 一方で、チェック項目全てをPRのレビュー観点として運用することが適切だとは考えていません。記事にする都合上、チームやプロジェクトのカルチャーや温度感、既存のルールに合っているか？という観点を今回は考慮していないからです。既存のプロジェクト内でルールを統一、浸透させるコストや競合する文化などを鑑みて、項目数を増減させたり、表現を修正する必要があるでしょう。セーフィーモバイルチームでも、この運用は議論の叩きの段階なので、今後チームにフィットさせるために議論を重ねていきます。
また、リーダブルコードに書かれていた観点がレビューにおいて見るべき箇所を網羅できてはいません。本書の説明していたロジックの修正の観点に加え、関数の関心がどこに向いているべきなのか、どのクラスで処理すべきなのか、どのパターンを適応すべきなのかといった純粋な技術力に基づく判断は必要でしょう。今後、SOLID原則など基礎となる実装パターンも学んで力を付けたいと思います。
チーム開発において、チームメンバーでレビュー観点に共通認識があり、プロジェクトが読みやすいコードで記述されていることで、レビューの時間が短くなり、コードの理解が早くなり、実装方針に悩む時間が減り、コードメンテナンスがしやすくなります。 皆さんも名著から学んだ知識を現場でどう活用できるか、どんな課題を解決できそうかを考えることで、チームの生産性を大きく上げるきっかけを作れるかもしれません。また新しい発見や学びがあれば、ブログに投稿したいと思います。
セーフィーでは、チームで切磋琢磨し成長していける方を募集しています。ご興味のある方のご連絡をお待ちしています。
