TOPICS

TECH BLOG

【TECH BLOG #8】コードレビューの心得

#コードレビュー

ファンプレックス株式会社 エンジニア部 アンドゥです。

弊社はゲーム運営のプロフェッショナル集団として、いくつものタイトルを運営しています。
しかし、それらのタイトルは作られた環境や考え方、使っているシステムなど一つひとつ異なります。ソースコードもその一つです。

弊社ではリリースされる全てのソースコードに対しプロジェクト毎にコードレビューを行っています。
コードレビューに関して、使われている言語やゲームエンジンによって細かなルールは変わってきますが、共通して意識して欲しい内容や気を付けるポイントなどは存在します。
弊社ではそれらの内容を洗い出し、メンバーへ共有しています。

普段からコードレビューを実施する人にとっては”当たり前”と思う内容かとは思いますが、
組織の中で1つの共通指標を提示することは全体のベースを整える為に重要な事だと考えています。

今回はその共通指標の中でクライアントエンジニア向けのコードレビューに関する内容を紹介致します。

はじめに

コードレビューに対して、若い人やあまり実施したことがない人にとっては
「なんだか難しい」「指摘される(する)のが怖い」と言ったイメージをお持ちの方が多いかと思います。
そのイメージを払拭する為にも、まずはコードレビューを実施する上での心得を2つ紹介します。

1.コードレビューは断罪するものに非ず
コードレビューはコミュニケーションと多くの視点によって、よりよいプログラムを作っていくものです。この原点に帰ると、実装者がどう設計すべきか、レビュアーがどのように評価すべきかが自ずと見えてくると思います。

2.コードレビューの心得
実装者は萎縮せずバンバンレビューしてもらって、自身のコーディング能力を上げていきましょう。レビュアーは実装者のコードが洗練されてきたら褒めてあげましょう。「あやつは儂(わし)が育てた」と胸を張って明言してもよいかと思います。

如何でしょうか?
この内容を心に留めて、以降は具体的な内容となります。

コードレビューの際のチェック項目

コードレビューの際は下記のチェックを行います。

1. ロジックの正しさ
2. コード全体のフォーマットがチーム基準に即しているか
3. 余計なimportやincludeを含んでいないか(C#だとusingディレクティブ)
4. コードの可読性、視認性
5. クラスやメソッドの役割を逸脱していないか
6. デバッグしやすさ
7. コメントの内容が適切かどうか
8. パフォーマンス(実行速度)を考慮した実装になっているか

1. ロジックの正しさ

コードレビューで一番大切なことですが、一番確認が難しいところでもあります。
なるべくコメントを多く残したり、シンプルな構造にしたりする工夫をしてください。
また、一気に巨大なコミットをレビューするというのはレビュアーにとって大きな負担になります。
できるだけ、小さい単位でレビューを受けるようにしてください。
その他、下記の点もレビューの観点になります。

1. 同一のロジックが2箇所以上で書かれていないか
2. コピペでコードを持ってきた場合にコピペ元の不要なコードが入っていないか
3. メソッド名から想定しづらいような副作用が存在しないか、もしくはドキュメント化されているか

2.コード全体のフォーマットがチーム基準に即しているか

チーム全体でコードのフォーマッターを統一することが望ましいです。
ただし、それ以外にもgitでのレビューが行いやすいように、タブやインデントをソフトタブにするのは必須です(インデントやタブの深さはチームで決めてよし)。
また、フォーマッター以外にもチーム内でのコード規約は基本的に遵守しましょう(googleやMSのコード規約に則るのが無難かと思われます)。

◆C# のコーディング規則 (C# プログラミング ガイド)
https://docs.microsoft.com/ja-jp/dotnet/csharp/programming-guide/inside-a-program/coding-conventions

◆google/styleguide
https://github.com/google/styleguide/

3.余計なimportやincludeを含んでいないか(C#だとusingディレクティブ)

Objective-CやC/C++では、不要なコンパイルが走ることがあり開発効率に大きく影響します。
C#ではそのような問題は起きないのですが、IL2CPPなどでC++コードに置き換えるフローの中で、余計なコード解析が走ることになるので、結局のところ開発効率に関わります。
可能な限り、綺麗な状態を保つようにしましょう。

4.コードの可読性、視認性

いくつか例をあげます。

1. コードの間隔を適度に空ける。ぎゅっと詰まっているコードは見にくい
(しかし、レビュー段階で指摘しにくいので、できるだけホスピタリティを意識する)

2. 適切なネスト深さを心がける
4段、5段になる場合、ネストの中身を別のメソッドに置き換えたほうが見やすい。
またラムダ式を入れ子にして深いネストになるようなことを可能な限り避ける。

3. 一行に長過ぎる処理を記述しない
次のデバッグしやすさにも起因するのですが、長い数式や複数の処理を一行にまとめてしまうと、レビュアーが見づらいだけではなくデバッガでステップ実行の際に変数の変更を追うのが難しくなります。基本的には下記のように心がけてください。
   a. 1行には1命令
   b. テキストエディタを横スクロールしないと見れないような長い式は分割する
   c. if文などで、括弧の中にメソッドを記述しない
   (変数等に戻り値を保持して、それをconditionとする)
   d. &&演算子、||演算子を使いすぎない
   e. 1行のif文を使わない。必ず括弧付きにするのが望ましい。
    if (hoge == null) return 0;ではなく、if (hoge == null) {return 0;}とする

4. ローカル変数をメンバ変数を区別できるような記述を心がける(プレフィックス、サフィックス、this記述など)

5. クラス、メソッド、変数の名前をあまり省略しない(かといって長過ぎるのは問題を孕んでいる可能性があるので、設計を見直す)

6. 同一のロジックが2箇所以上で書かれていないか
   a.ただし、ラムダ式などでごく短いセンテンスが重複する程度(5行程度)は、許容する
   (インライン展開での実行速度を考慮して)
   b.Don’t repeat yourself(DRY)原則を遵守する

7. コピペでコードを持ってきた場合にコピペ元の不要なコードが入っていないか

8. メソッド名から想定しづらいような副作用が存在しないか、もしくはドキュメント化されているか

9. 複雑な処理はコメント記述して、説明を残すようにする(アルゴリズム名などを書くのもよい)

5.クラスやメソッドの役割を逸脱していないか

プロジェクトごとによりますが、下記の観点で評価を行います。

1. その開発環境において、標準的なコーディングが行われているか
(UnityならUnityらしさ、cocos2d-xならcocos2d-xといったゲームエンジンやプラットフォームに即した標準的な構造や設計というものを準拠すべき)

2. そのクラスにふさわしくない処理を記述していないか
(ModelクラスにControlクラスが行うべき処理を実装するなど)

3. ひとつのメソッドに極端に処理を記述しすぎていないか
(処理を適切に分割して構造を明確にしておく)

6.デバッグしやすさ

コードの可読性の項にも記述しましたが、一行に複数の処理を記述するとステップ実行の際に処理を追いにくくなり、バグを生む可能性が高まります。
ローカル変数などを利用し、一行のセンテンスが短いものがよいコードだと思います。
ラムダ式やコルーチンでは、ブレイクポイントのあとのコールスタックを追えなかったり、ブレイクポイントでストップできない場合などもあり、これらの仕組みは大変便利ですが、大きな処理は直接記述せずに、別メソッド内で行うのが望ましいです。

またラムダ式やコルーチンがネストするようなコーディングは大きな複雑性を孕むので、このような構造になる場合、本当に必要な構造なのか設計を見直すキッカケになります。
テンプレートクラスやジェネリッククラスはうまく使えば大変便利なものですが、乱用によりプロジェクトを複雑にする一面もあります。
できるだけ組込みのものを使うようにして、自分でどうしても定義が必要な場合は、他のレビュアーに説明ができるようにしてください(詳細なコメントを残すのも有効です)。

7.コメントが適切かどうか

コードは他人に見られるものだという前提を忘れないようにしてください。
リファクタリングや処理の変更によって、昔書いたコメントが逆にミスリードを生む場合もあります。
コメント自体が修正されるのがベストですが、誤ったコメントを残すぐらいなら、ざっくり削除したほうが良いケースも多いので、心がけてください。

8.パフォーマンス(実行速度)を考慮した実装になっているか

パフォーマンスも重要です。
詳細な対応内容に関しては、使われている言語やゲームエンジンによって異なる為、プロジェクト毎のルールとなります。
以下に例をあげますので、当てはまるものを参考にしてください。

1. 無駄なループ処理が無いか
2. ToArrayやToListを使用するなどを多用して型変換が頻発していないか
3. 不必要なクラスインスタンスやコンテナのコピーが発生していないか(特にC++)
4. 探索アルゴリズムを考慮した適切なコンテナを使っているか
5. Boxingを意識しているか (C#)
6. ソートは適切か(アルゴリズム、コールバックメソッド、ソート範囲など)
7. 禁止行為はないか(例えば、無許可でのスレッド生成など)
8. インスタンシングが重い箇所でオブジェクトプールなどの工夫の余地はないか
9. コンテナを全探索するような箇所で処理の枝刈りは行われているか
10. 実行速度を追求した結果、メンテナンス性を損なっていないか

多くの場合、パフォーマンスの改善は小さな改善の積み重ねとなります。
この積み重ねの内容は職人芸と言っても過言ではありません。コードレビューでしっかりと指摘し、メンバーへ伝承しましょう。

最後に

何か参考になる内容はありましたでしょうか?

弊社では運営タイトルが多く、メンバーのアサイン先変更も少なからず行われます。
各プロジェクトでは、今回ご紹介した内容を基に詳細なルールを決めていますので
別のプロジェクトにアサインされた後も安心して、素早く自身のパフォーマンスを発揮できるような仕組み作りを行っています。

またの機会に他の共通指標も掲載したいと思います。

ファンプレックス株式会社

グリー株式会社の100%子会社で、ゲーム運営事業を行う。
公式サイト: https://funplex.co.jp/
公式Facebook:https://www.facebook.com/funplexInc

[本件に関するお問い合わせ先]
ファンプレックス株式会社 広報担当
東京都港区三田一丁目4番1号 住友不動産 麻布十番ビル 5階
E-mail: info.funplex@funplex.co.jp