はじめに

現在、私のチームではレビューガイドラインを明文化して、レビュアーはガイドラインに従ってコードレビューを行なっています。このガイドラインは、昨年末からチームで運用を開始して１年になりますが、チームでも浸透して来ており、メンバーは全員レビュー時に利用しています。

私がチームに join した当時の課題

私は今モバイルの開発チームの所属しておりますが、メンバーは20代のエンジニアが中心です。また、弊社のサービスはSaaSが中心であるため、これまでモバイル開発の経験者が少ない状況でした。そのため、

モバイルの実装スタイルが分からない
結果レビューの観点が分からない

という課題がチームにあり、しっかりとしたレビュー体制が築けていない状況でした。

その結果として、

メンバーの技術力の伸び悩み
リリース後に一定数のバグが発生する

といった状況でした。

どうやって課題を改善するか

そこで、上記の課題改善に取り組むことにしましたが、レビュー指摘を類型化して、メンバーの弱点を定量化できないかと考え、コードレビューに対するガイドラインを作成して、レビューコメントを類型化することにしました。

一定の観点を持ってレビューコメントを類型化することで、KPIとして計測が可能（見える化）になり、弱点分析をすることができるからです。

ガイドラインを作成する上で、Googleが公開するレビューガイドラインを参考にさせて頂き、コードレビューの観点として以下の7つの観点を設けることにしました。

採用したコードレビュー観点

ここからは、実際にレビューガイドラインに記載している内容の一部を紹介します。

No	観点	概要
1	Design	設計が適切か
2	Simplicity	理解容易性
3	Naming	クラス、メソッド、変数名などの命名
4	Style	コードスタイル
5	Functionality	機能（要件）を充足しているか
6	Test	テストの記述、パターンが適切
7	Document	コメント、ドキュメントに関連

特にNo.1 ~ No.4は、オブジェクト指向の観点で非常に重要な観点といえます。しかし、これらの観点の概要だけではレビュアーにより判断が難しくなってしまうため、もう少し具体的に明示している内容を紹介します。

1. Design（設計）

定義

システムにとって適切な責務・振る舞いになっているか。システムとしてアーキテクチャを遵守できているか。また、システム全体として一貫性ある設計になっているか。

具体例

基本的には、以下のようなオブジェクト指向の基本であるSOLID原則に反するような場合、指摘の対象になります。

関心の分離原則違反（≒ 単一責任原則違反）
密結合
低凝集
DRY原則違反 etc.

例えば以下のコードの場合、add() で様々処理を行なっており、責務超過といえるためDesign指摘の対象になります。

class HogeDiscountManager {
    lateinit var manager: DiscountManager
    /**
    * 商品を追加する
    */
    fun add(product: Product): Boolean {
        if (product.id < 0) { // バリデーション 1
            throw IllegalArgumentException()
        }
        if (product.name.isEmpty()) { // バリデーション 2
            throw IllegalArgumentException()
        }
        val temp: Int = if (product.canDiscount) { // 条件分岐 1
            manager.totalPrice + manager.getDiscountPrice(product.price)
        } else {
            manager.totalPrice + product.price
        }
        return if (temp < 3000) { // 条件分岐 2
            manager.totalPrice = temp manager.discountProducts.add(product)
            true
        } else {
            false
        }
    }
}

2. Simplicity（理解容易性）

定義

システムとして可読性あるコーディングになっているか。
処理ができるだけシンプルな振る舞いになっているか。

具体例

以下のように実装が複雑になる場合、指摘の対象になります。

ネストが深いif分
複雑な三項演算子文
冗長なSQL
stream, filter, mapを多用したObject整形文 etc.

このように分岐が多いif分は指摘の対象になります。

Before.kt

val powerRate: float = member.powerRate / menber.maxPowerRate
var currentCondition: Condition = Condition.DEFAULT
if (powerRate == 0) {
    currentCondition = Condition.DEAD
} else if (powerRate < 0.3) {
    currentCondition = Condition.DANGER
} else if (powerRate < 0.5) {
    currentCondition = Condition.WARNING
} else {
    currentCondition = Condition.GOOD
}
return currentCondition

実際のレビューコメントでは、このようにネストを解消するように指摘をする場合などに使用します。

After.kt

val powerRate: float = member.powerRate / menber.maxPowerRate
if (powerRate == 0) {
    return Condition.DEAD
}
if (powerRate < 0.3) {
    return Condition.DANGER
}
if (powerRate < 0.5) {
    return Condition.WARNING
}
return Condition.GOOD

3. Naming（命名）

定義

変数やクラス、メソッドに責務を意図した明確な名前が付けられているか。英語文法に誤りがないか。typoもこれに含まれる。

具体例

このような命名に関する指摘をする場合に使用します。

振る舞いと一致しない変数名、関数名
責務と一致しない関数名
英文法の誤り etc.

他にもiOS開発時では、Swift FoundationやCocoaの命名規則に準拠しない場合も、これに該当します。

4. Style（コードスタイル）

定義

コードスタイル言語仕様に準拠しているか。

具体例

コードスタイルは言語仕様やフレームワークに準拠させることが基本になるため、これに反する場合に使用します。

静的解析違反
不適切なアクセス修飾子
表記違反(スネーク、キャメルなど) etc.

他にもモバイル開発では、公式(Apple、Google等)で公開しているガイドライン違反している場合もこれに含まれます。

5. Functionality（機能要求）

定義

システムとして外部仕様を充足しているか。作者が意図した通りの振る舞いであるか。
また、システムの通信量、パフォーマンスに懸念がないか。

具体例

主な観点としては、外部機能を充足しているかという点が対象になります。

外部仕様の未充足（不具合）
概要設計書のフローチャートと異なるフローになっている
不要データを送信している etc.

6. Test（テスト）

定義

システムとして適切な自動テストを兼ね備えているか。自動テストの内容で品質を担保できているか。
また、システムを担保するパラメータ群を備えているか。

具体例

テストコードが期待になっていない場合や、テストでのパラメータに考慮漏れがある場合、指摘の対象になります。

対象のメソッドがテストされていない
テストパターンが網羅できていない（パタメータテスト、閾値テストの不足）
分岐がパターンが網羅されていない
実装上宣言している静的定数値が直接ハードコードされている
アーキテクトに準じたテストになっていない

7. Document（文章）

定義

ソースコード上に記載されているdoc、コメントが適切な内容であるか。
また、関連するドキュメントは更新されているか。その内容は適切か。

具体例

ソースコードに関連するコメントだけでなく、プロジェクトで管理している関連ドキュメント（README）も対象になります。

関連ドキュメントの更新漏れ（READMEなど）
docやコメントの内容が不適切、内容が不適切

指摘対応の要否

更に、コードレビューの現場では上記の7つの観点に加えて、指摘修正の要否を4つの累計に分けてコメントしています。

観点	概要
MUST	PR、MRをマージするためには必ず修正が必要
SHOULD	修正なしにマージすることはできるが、リリースまでには修正が必要
IMO	レビューアー観点の意見。修正不要
NITS	IMOより細かい意見など。修正不要

このように、コードレビューでマージするために必要な修正はMUST指摘となります。MUSTとSHOULDの使い分けは難しい部分もありますが、これまでのレビューガイドラインの運用では、SQLのパフォーマンスをより良くするための指摘やテストコードの最適化の指摘などで SHOULDは利用されたりしており、その場合は修正タスクを Issue などに積んだ上で（修正スコープの合意）、マージするようにしています。一方、IMOやNITSは修正は不要ですが、修正しない場合はその旨をコメントに返信してもらい、コメントを閉じてからマージする運用をしています。

具体的な利用方法

実際にコードレビューをするとき、上記の7つの観点と修正の温度感をこのように交えたPrefixを付けて、コメントをします。

指摘例

MUST(Design): ドメインロジックがControllerクラスに実装されてます。 domain層の対象packageに新しくクラスを作成して実装を移してください。

このときPrefixの入力を手入力にしてしまうと、入力の手間や入力がミスが生じることもあるので、カスタムscriptで入力をサポートするようにしています。

発展編

更に私のチームでは、このようにPrefix付けたコメントを集計して、KPIとして数値化して弱点分析などを行なっております。具体的には、GitLab APIを利用してコメントを集計しておりますが、その技術的な方法については、こちらをご覧頂ければと思います。

最後に

以上のように、私のチームではコードレビューガイドラインを作成してルールを明文化することで、技術力を見える化させて課題改善を進めています。レビューコメントをこのように分析することで、個人の弱点に合わせたアプローチ方法も見えてきます。このように技術力に対するアプローチとしてPDCAサイクルを回すことで、チームのでの技術育成を進めております。

最後に簡単にまとめると、コードレビューガイドラインを明文化した場合、

コードレビューでオブジェクト指向が学べる
苦手分野が明らかになる
スキルアップのためのアクションプランが検討しやすい

といった恩恵を受けることできるので、ぜひチームに合ったコードレビューガイドラインを作成してみてはいかがでしょうか。