技術力を見える化する！オブジェクト指向コードレビューの実践

はじめに

こんにちは akihiyo76 です。現在、私のチームではレビューガイドラインを明文化して、レビュアーはガイドラインに従ってコードレビューを行なっています。このガイドラインは、チームで運用を開始して２年になりますが、チームでも浸透しレビュー時に必ず利用するようになりました。

はじめに
コードレビューの課題感
課題改善に向けて
- 採用したコードレビュー観点
指摘対応の要否
具体的な利用方法
- 指摘例
最後に

コードレビューの課題感

私は現在モバイル開発チームに所属しておりますが、メンバーは若手エンジニアが中心です。一方、弊社のサービスは SaaS が中心であるため、これまでモバイル開発の経験者が少ない状況でした。そのため「モバイル技術のセオリーが分からない」という課題がチームにあり、コードレビューに苦労する状況でした。

その結果として、

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

といった状況でした。

課題改善に向けて

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

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

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

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

では、実際にレビューガイドラインで採用した観点を紹介します。

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 分は、Simplicity の指摘の対象になります。

// 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 の命名規則に準拠しない場合、Naming の指摘対象になります。基本的な命名規則は、利用しているフレームワークや言語の特性によるものが判断基準になります。

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

定義

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

具体例

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

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

他にもモバイル開発では、公式(Apple、Google 等)で公開しているガイドライン違反している場合もこれに含まれます。コードスタイルの判断はその人の経歴などの主観的な部分も影響するので、コードフォーマッターを導入し、機械的な判断基準を設けることもこの指摘点を減らす有効な手段です。

5. Functionality（機能要求）

定義

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

具体例

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

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

6. Test（テスト）

定義

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

具体例

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

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

7. Document（文章）

定義

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

具体例

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

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

指摘対応の要否

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

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

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

具体的な利用方法

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

指摘例

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

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

最後に

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

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

指摘数に応じて技術力を見える化できる
コードレビューでオブジェクト指向が学べる
スキルアップのためのアクションプランが検討しやすい

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

RAKUS Developers Blog | ラクスエンジニアブログ

株式会社ラクスのITエンジニアによる技術ブログです。