今回の内容について
本記事はUnityAdventCalendar3の記事です。
Unityでチーム開発を行う上で、プルリクではこういう点を自分は見るよというのをまとめたものです。
あくまで執筆者はこういう点をレビューしている or レビューを他人にお願いするときにこのあたりをチェックすること期待しているよ、という内容です。人によって主義主張は違うかと思うので、強烈なマサカリはご遠慮ください。
また、使用言語や人数、プロジェクト規模によって適切なレビュー体制というものは異なります。
この記事は「Unity C#開発で、プログラマ以外も含め多くの人間が開発に参加しており、中長期的（数年に渡り）保守運用する必要があるプロジェクト」をターゲットにして書いています。
全体を通して
要件を満たしているか
要件とは、いま開発しようとしているものが満たすべき条件のこと。
要件を正しく認識し、作られた機能に齟齬がないかどうかを確認します。
たとえキレイな設計で、美しいコードを書いていたとしても、要件を満たせていない場合はApproveできません。
負債になりそうな作りになっていないか
負債とは「後になって問題になりそうな実装」のことを指します。
「文字列でswitchして処理を分岐する」とか「１個のクラスの中に処理が詰め込まれすぎている」とか、こういうのは負債になりやすいです。
こういう負債になりそうなコードは早めに潰しておくべきなので、プルリク段階でその空気を感じ取った場合は理由を書いてRejectします。
（「文字列で分岐はタイプセーフではないので、可能であればenumなどを定義して使ってほしいです。さらにいえばswitch文をそもそも使わない実装にしたいです」みたいな）
あとで確実にリファクタリングすることがわかりきっている、かつ、早急にマージしないといけない、みたいな限定的な状況ではApproveすることもあります。
（バグが出たので急ぎでHotfixを当てなくてはいけないときなど）
プルリクの粒度が妥当であるか
File Changedが300を超えているみたいな、巨大なプルリクはレビューする気が失せます。
そして確実にレビュー漏れが起きます。
外部ライブラリを追加したりした場合は、それ単体でまずプルリクを出し、マージされてから繋ぎこみのプルリクを出すなど小分けにしましょう。
また新規機能を追加する場合や大きくリファクタリングを行う場合は、まず設計段階でレビューを依頼するのもありです。
その方が実装に入ってからの手戻りが少なくて済みます。
余計な変更が混ざっていないか
ある機能を実装したついでにまったく別の部分のリファクタリングも一緒にやりました、みたいなパターン。
リファクタリング自体はありがたいですが、これをマージしてしまうと、あとで変更をrevertしたいとなったときに関係ない部分も巻き戻ることになってしまいます。
あまりにも関係ないコミットが混ざっているようなら、それだけ分離して単体のプルリクとして出してください。
デグレしてないか
デグレとは「変更を加えた結果、逆に壊してしまった」という状態のこと。リグレッションと呼んだりもします。
（レビュー段階で気づければそれはデグレでは無い気もしますけど…）。
リファクタリングをしたときに他の機能を壊してしまった、みたいな状態です。
自分が詳しくない部分の機能変更や修正を行った場合、その機能に詳しい人をレビュアに追加してください。
スクリプト全体
typoがないか
typoとは、タイプミスのこと。
しょうもないtypoは結構恥ずかしいです。
名前付けが適切であるか
プログラムにおいて、「名は体を表す」という言葉ほど大事なものはありません。
プログラムは「クラス名」「変数名」「メソッド名」を見て何ができるかパッと連想できる状態が好ましいです。
名前付けにおいてチェックする項目を次に挙げます。
クラス名やメソッド名にあやふやな名前をつけない
あやふやな名前をつけてしまうと、やってる処理の内容も雑多になってしまい、可読性が著しく落ちてしまいます。
「処理の内容と名前が一致している」という状態を保ってください。
もし処理の内容について一意な名前をつけることが困難な場合、その処理は複数の処理の連続になっている場合があります。
その場合は処理を分割してみることを考慮するとよいでしょう。
- 何をするのか名前からまったく連想できない
PlayerControllerクラス -> プレイヤの初期化処理のみを担当するPlayerInitializerにリネーム - 何を意味するのかわからない
bool flag;というフィールド変数 -> 動作中を表現するbool isRunning;にリネーム - 何をチェックするのかわからない
Check()メソッド -> 何をするのか明確な名前にリネームCanPlayerJump()
動作を指す場合は対になる単語を使う
add/remove、start/stopなど、ある動作表す英単語には、それと対になる単語が存在します。
片方の英単語を使った場合、対になる英単語をあわせて使うようにすることで、使用時に混乱を減らすことができます。
だめな例： 処理を開始するときStart() → 処理をやめるときKill()
よい例： 処理を開始するときStart() → 処理をやめるときStop()
特別な意味を持つ単語は気をつける
たとえば、GetStatus()という名前のメソッドがあったときに、一般的にこのメソッドには副作用が無いことを期待します。
つまり、GetStatus()は何度実行しても状態を上書きするようなことがあってはいけません。
（一般的にGetから始まるメソッドに副作用をもたせてはいけないという暗黙的なルールがある）
たとえば「アイテムを拾う（拾ったアイテムは数が減る）」といったメソッドを用意したい場合を考えます。
この場合はGetItem()よりも、副作用があることを連想しやすいTakeItem()やPickUpItem()といった名前をつけた方がよいでしょう。
名前に一貫性を持たせる
処理内容が非同期的に実行される場合はメソッド名に「-Async」というsuffixをつける場合が多いです。
返り値がvoidであったときなど、型情報からメソッドの動作が読めない場合はメソッド名で非同期処理かどうかを分かるようにしておくとよいです。
また、IObservable<T>を使っているとき限定の話ですが、IObservable<T>は長さが不定という特徴があります。
そのため、返り値がIO<T>のメソッドやプロパティをSubscribeしたときに、値がいくつ発行されるのかわからない問題が発生します。
この場合は「発行されるメッセージ数が1個ならAsync」「複数個ならAsObservable」をsuffixとしてつけることをオススメします。
IObservable<GameObject> OnSpawnPlayerAsync() // プレイヤが1回だけ非同期的に生成されるIObservable<GameObject> OnSpawnPlayerAsObservable() // プレイヤが複数回非同期的に生成される
テストメソッド名はわかりやすくする
日本人同士でのみ開発するなら、テストメソッドの名前は日本語を使ってもよいです。
無理に英訳して視認性が落ちたり、伝えたいニュアンスが漏れるなら、いっそのこと日本語で細かくメソッド名を書いたほうがよいです。
ただし、これはあくまでテストメソッド上でのみ。実コードで日本語メソッドや変数はやめてほしい。
//これより
[Test]
void Retry3TimesWhenFailedToDownload()
{
}
//こっちの方が【日本人にとっては】わかりやすい
[Test]
void ダウンロードに失敗したときは3回までリトライする()
{
}
コーディング規約を満たしているか
C#には公式のコーディング規約が存在しています。
このコーディング規約に準拠できているかどうかをチェックします。
ハードタブが紛れ込んでいたり、ハンガリアン記法になっていたりした場合は指摘します。
また、C#バージョンによっては省略記法を使える場合があります（自動実装プロパティや文字列補間など）。
省略記法が使える場合はそちらを使うように指摘します。
なお、コーディング規約に定義されていない部分については、あらかじめチーム内で合意をとって統一しておくべきです。
privateフィールド変数の頭に_をつけるかどうかなど。
コードフォーマットがされているか
インデントやif文での{}の扱い等は統一したいです。
IDEを使っている場合はコードフォーマットの自動適用が可能なため、極力IDEでのコーディングを勧めます。
ディレクトリ構造や名前空間が妥当であるか
C#はJavaと違い、ディレクトリ構造と名前空間を一致させる必要がありません。
そのため名前空間は自由に定義することができます。なので名前空間の粒度で紛糾することがあります。
これについてはチーム内で議論して名前空間の粒度をあらかじめ決定しておくとよいでしょう。
たとえば、 Assets/HogeProject/Scripts/FugaSystem/Impls/Piyo.cs みたいなディレクトリ構造の場合は、名前空間は「HogeProject.FugaSystem」にするみたいな。
（AssetsやScripts、Implsといったほとんど意味を持たないディレクトリ名は名前空間から省く）
コメントが正しく書かれているか
「なぜこの実装にしたのか」という理由が書いてあるとよいです。トートロジーなコメントはいらないです。
private Enemy[] _enemies;
// (省略)
private void Update()
{
if (IsEliminatedAll())
{
// (敵が全滅していたときの処理)
}
}
private bool IsEliminatedAll()
{
// すべてのEnemyのIsDeadがtrueであるか確認する
//（みたら分かることをそのまま書いてあるのでコメントとして価値が薄い）
for (var i = 0; i < _enemies.Length; i++)
{
if (!_enemies[i].IsDead) return false;
}
return true;
}
private Enemy[] _enemies;
// (省略)
private void Update()
{
if (IsEliminatedAll())
{
// (敵が全滅していたときの処理)
}
}
private bool IsEliminatedAll()
{
// Update()で実行するメソッドのためLINQを使うとコストが大きい
// そのためより低コストで実行できるfor文にした
// （なぜLINQを使わなかったのか、という理由が書いてある）
for (var i = 0; i < _enemies.Length; i++)
{
if (!_enemies[i].IsDead) return false;
}
return true;
}
リソースを確実に解放しているか
IDisposableなオブジェクトを使っている場合、それを適切なタイミングでDispose()しているかどうかがを見ます。
usingステートメントが利用できる場合はそちらを使うように促します。
また、自前で何かクラスを定義した場合はIDisposableを実装しているかどうかもチェックします。
特にマルチスレッド処理を実行している場合は、アプリケーション停止時に確実にスレッドを停止させているかも確認します。
スレッドの止め忘れは気づきづらく、原因の特定が困難なバグを生むことがあるため本当に注意深くレビューします。
（スレッドを止め忘れていると、Editor起動直後は正常に動くが、何回か動かすと裏でスレッドが溜まってリソースを消費し正常に動かなくなる、みたいな現象が起きてしまう）
またasync/awaitを使っている場合はちゃんとCancellationTokenを指定しているかを確認します。
同様に、UniRxを使っている場合はちゃんとOnCompletedメッセージの発行、ないしDispose()をしているかを確認します。
パフォーマンスに問題がない作りになっているか
パフォーマンスは主に「計算量」「メモリ」「I/O」をチェックします。
計算量
データ量が多いコレクションを操作する場合や、何かのデータのパースを行うときなどは注意してレビューします。
純粋に計算量が多く、メインスレッドで処理するには負荷がかかる場合は別スレッドに処理を逃がすように指摘します。
（UnityAPIに依存した処理の場合はどうしようもないけど）
また、ロジックを修正することで計算量が大幅に削れる場合はそれも指摘します。
メモリ
主にGC Allocateが発生するかどうかを中心にチェックします。
また、サイズの大きいstructのコピーも割とバカにならないコストがかかるので、そこもチェックします。
-
structとclassはメモリ周りでの挙動が大きく異なるため、そこを意識して使い分けができているか。 -
LINQのToArray()やToList()はそれなりにコストが大きいと認識できているか。 -
LINQの使い所が妥当であるか（Update()で連打したりしてないか）。 - 参照渡し(
ref)を正しく用いているか（C# 7ならinとoutを活用しているか）
I/O
I/Oとはアプリケーションと外部との通信部分のことです。
「ファイルの読み書き」や「ネットワーク通信」に該当する部分です。
こういったI/OはCPUの処理速度と比較して桁外れに遅いので、非同期化が必須となります。
同期処理で書いてあったりした場合は非同期化するように指摘します。
また、処理に時間がかかるため、処理の途中でシーン遷移が発生するなど、キャンセルが発生する可能が高い箇所でもあります。
そのため処理が正しく完了するまで待機しているか、キャンセルする場合はCancellationTokenを使った中断処理が正しく行われているかもチェックします。
MonoBehaviourを正しく扱えているか
MonoBehaviourを継承するとUnityのライフサイクルへ乗ることになります。
SerializeField、GetComponent、Update()などが利用できるメリットがある反面、寿命管理を自由にできなくなるデメリットがあります。
またGameObjectに紐付けないと扱えないインスタンスになってしまうため、取り回しも非常に悪くなってしまいます。
クラスの責務を確認し、MonoBehaviourを使う必要性がないのであればPure Classとして定義するように促します。
とくにドメイン層のオブジェクトはMonoBehaviourを継承するべきではありません。
ZenjectのInstallerが正しく定義できているか
Zenjectを使っている場合、Installerの定義を正しく行えているかを確認します。
Zenjectを使うとインスタンスの寿命管理も任せることができるようになります。
IDisposableを実装したクラスを設定するときは、BindInterfacesAndSelfToを使っているかどうかをチェックします。
（BindInterfacesAndSelfToでバインドすると、IDisposableインタフェースを実装していた場合はコンテキスト破棄時にDispose()を実行してくれる）
また、Bindの書き方によってはAsCached()などが正しく機能しない場合もあるので注意します。
// こうかくと、Piyoは2個生成されてしまう
Container.Bind<IHoge>().To<Piyo>().AsCached();
Container.Bind<IFuga>().To<Piyo>().AsCached();
// これだとIHogeとIFugaで共通したPiyoが生成される
Container.Bind(typeof(IHoge), typeof(IFuga)).To<Piyo>().AsCached();
エラーハンドリングを正しく書けているか
正常系だけでなく、ちゃんと異常系のことを考慮したコードになっているかを確認します。
- 失敗時に必要な情報をログ出力できているか
- 失敗時にリトライするのか、中断するのかが明確になっているか
- 失敗時にそのまま無視して処理が突き進まないか（正しく失敗を検知できるか）
- 失敗時にアプリケーション全体まで巻き込んで停止してしまわないか
- 正しく例外を投げているか
- 上流で
try-catchを正しく設置できているか - 失敗時に
finally等で正しくリソースを解放できているか - キャッシュ機構がある場合は、失敗状態までキャッシュしてしまわないか
処理の失敗時に例外を投げるべきなのか、nullやdefault(T)を返すべきなのかは文脈で変わるため一概にどっちにしろと言い切ることはできません。
重要なことは「予期せぬ失敗が起きたときにそれを正しく検知できる実装になっているか」です。
設計
「ドメイン」の概念がコード上に存在するか
「ドメイン」とは、「いま作ろうとしているものごとにおいて本質的に扱いたい対象領域」を指します。
正直、すごく意味を伝えづらい単語ではあります。
ものを作るとき、その対象を「一番大事にしたい部分」と「本質的にはどうでもいい部分（期待したパフォーマンスで期待した挙動をするならどんな実装でも構わない部分）」の2つに分けることができます。
この「一番大事にしたい部分」がドメインです。
たとえば、「プレイヤが敵に触れると死ぬ」という処理があったとします。
これをドメインとそうでない部分でわけてみます。
ドメイン
- プレイヤという存在
- 敵という存在
- ダメージという概念
- ダメージをプレイヤに与えることができるということ
- プレイヤはダメージを受けると死ぬということ
- 敵はプレイヤにダメージを与えることができるということ
- プレイヤは移動できるということ
- 敵は移動できるということ
ドメインではない部分
- 敵がプレイヤに触れたことの検知方法（
OnTriggerEnterを使うのか、Update()で距離を毎フレーム計測するのか） - 敵の移動方法の実装（
Transformを毎フレーム更新するのか、Rigidbodyの物理演算で動くのか） - プレイヤの移動方法の実装（
Transformを毎フレーム更新するのか、Rigidbodyの物理演算で動くのか） - ダメージを与えたときのエフェクトの表現方法（画面にどのようなエフェクトを出すのか）
このように、**本質的に「何が存在するのか」「やりたいことは何なのか」**を抽出して整理したものがドメインです。
ドメインを明確にしておくと、実装するときに混乱せず、やろうとしていることが明確になります。
たとえば、移動処理にRigidbodyの物理演算を使っているコードがあったとします。
ドメインの概念がないと、「そもそもなぜ物理演算を使っているのか」の議論をすることができず、その実装が本当に適切であるかの判断ができません。
ドメインの概念を持っていれば、「ドメイン的には移動方法にこだわりはないはずので、Transformを毎フレーム更新するだけでこと足りるのでは」といった議論ができるようになります。
レビューする場合もドメインをまず把握することで、全体の見通しを簡単に立てることができるようになります。
ドメインのモデリングが妥当であるか
ドメインのモデリングとは、すなわちこれから作ろうとしているもののドメインを明確にする作業を指します。
具体的にいえば、「クラス設計やインタフェース設計を行い、これから何を作るべきかを洗い出す作業」です。
プルリクをレビューするときは、まずはこのドメインモデリングが妥当であるかを見ます。
ここが歪になっていたり、無理がある作りになっていると、今後どうがんばってもプログラムがキレイにならないからです。
いわばこれから作るプログラムの根幹を定義することになるため、ここはレビュー時にかなりしっかりチェックします。
なお、ドメインのモデリングは非常に難しいです。
難しい理由は2つあり、「仕様についての深い理解が必要であるから」「設計についての十分な知識が必要であるから」です。
仕様について理解が浅い、つまり我々がいま何を作ろうとしているのかをそもそも理解していない人が正しくドメインを設計できるわけがありません。そのためドメインモデリングを行うのであれば、これから何を作ろうとしているのか完全に理解してから行う必要があります。
（何を目的にしたアプリケーションで、今後どういう方針で機能を追加していくのか、既存機能はどういう歴史的経緯で追加されてきたのか、を完全に把握している必要があります。）
「設計についての十分な理解があるか」については、長くなるので次の項目で説明します。
設計に無理がないか
ソフトウェアを作る上で、いきあたりばったりでコーディングを始めるとぐっちゃぐちゃのコードになります。
そのためドメインを明確にし、何を作るのかをあらかじめ洗い出しておく必要があります。
この洗い出し作業が「設計」にあたるのですが、これはかなりの経験と知識が必要です。
そのため文章化するのがとても難しいのですが、次の項目を満たしていればとりあえずはOKです。
- SOLID原則を満たしているか
- インタフェース定義が過不足ないか
- 依存性注入を正しく活用できているか
- クラス間の相互参照や循環参照がないか
- 依存関係が妥当であるか
- デザインパターンを用いる場合はそれが正しく運用されているか
- View要素がある場合は、View-Modelの関係に一貫性があるか（何かのデザインパターンを用いているか）
- 名前付けが妥当であるか
- テストが書きやすい作りになっているか
このあたりについては過去にスライドを作ったので、よければみてほしいです。
可能であるならばドメインのみを定義し、具体的な実装は何も書いてない状態で一度プルリクを出すことをオススメします。
ドメインモデルの設計に問題が見つかったときに、すでに実装に入ってしまっていると手戻りが起きてしまいます。
腐敗防止層を正しく配置しているか
やばそうな空気がするSDKなどを直接ドメインで触っている場合は指摘します。
そのとき「腐敗防止層を設置してください」と指示することがあります。
腐敗防止層とは、外部のシステム（何かのSDKなど）と連携する場合に必要となる場合があるレイヤです（必ず必要なわけではない）。
- レガシーすぎて現代の思想と噛み合わないSDK
- 逆に変更頻度が激しすぎてよく破壊的変更がよく行われるSDK
- 癖が強すぎて扱いずらいSDK
こういったSDKをそのまま組み込んでしまうと、SDKの思想によって我々のドメインモデルまで汚染されてしまう可能性があります。
（たとえば、SDKが提供する謎の神クラスインスタンスをドメイン層で引きずり回さないといけない、など）
そこで一度、外部システムとドメインモデルとの間でマッピングを行うレイヤを設けてしまいます。
それを担うのが「腐敗防止層」です。腐敗防止層を設置すると、SDKの思想に汚されることなくドメインを純粋な形に保ったまま開発を続けることができるようになります。
**ただし、**往々にして腐敗防止層の構築にはそれなりのコストが発生します。
「じっくり時間をかけてキレイなものを作る」「汚いけど手間は少なくさっさと実装が終わらせる」
どちらを選択するべきかはよく考える必要があります。
もし、あとあとそのSDKを捨てたり、別のSDKに差し替える可能性があるのであれば腐敗防止層を設置しておくと後で楽ができます。
DTOを用いているか
外部システムと連携する必要があるが、腐敗防止層ほどのものが必要でない場合はDTOを定義するように指摘します。
DTOとはData Transfer Objectの略であり、外部システムとの連携時に用意するデータ構造です。
たとえば、「jsonからデータを読み込んでドメインで使う」というコードを書く場合、DTOを用いないと次のような問題が発生します。
DTOを使わない実装
{
"WalkSpeed": 10,
"MaxHp": 100
}
using System;
using UnityEngine;
namespace Domain
{
/// <summary>
/// 敵のパラメータ
/// </summary>
[Serializable]
public struct EnemyParameter
{
public float WalkSpeed;
public int MaxHp;
}
/// <summary>
/// 敵クラス
/// </summary>
public class Enemy : MonoBehaviour
{
private EnemyParameter _enemyParameter;
private int _currentHp;
// 外から与えられたEnemyParameterを参照して動く
public void Initialize(EnemyParameter enemyParameter)
{
_enemyParameter = enemyParameter;
_currentHp = enemyParameter.MaxHp;
}
public void Update()
{
// 正面に向かって移動する
transform.position += Vector3.forward * _enemyParameter.WalkSpeed * Time.deltaTime;
}
}
}
public class EnemeyGenerator : MonoBehaviour
{
[SerializeField] public GameObject _enemyPrefab;
private async void Start()
{
// Jsonをファイルから読み込み
var json = await Task.Run(() => File.ReadAllText("enemy.json"));
// jsonから直接ドメインオブジェクトに変換
var enemyParameter = JsonUtility.FromJson<EnemyParameter>(json);
// 敵を生成する
StartCoroutine(GenerateCoroutine(enemyParameter));
}
// 与えられたパラメータで敵を生成する
private IEnumerator GenerateCoroutine(EnemyParameter enemyParameter)
{
while (true)
{
var enemyObject = Instantiate(_enemyPrefab);
var enemy = enemyObject.GetComponent<Enemy>();
enemy.Initialize(enemyParameter);
yield return new WaitForSeconds(3);
}
}
}
上記の実装では、ドメイン層にオブジェクトであるEnemyParameterを直接Jsonの定義に用いています。
Jsonの定義がこのまま変更されることなく、かつ、ドメインの要素も変更されないのであればこれでも問題はありません。
ではここで、「Jsonの定義が使いにくいので構造を変えてください！」と利用者から依頼が来てしまったときに、この実装では問題が起きます。
というのも、この実装ではjsonの定義を変更しようとするとドメインオブジェクトであるEnemyParameterの定義まで変更する必要がでてきてしまいます。
つまり、「本質的にやりたい重要なデータ」を定義してあるドメイン層が、「jsonが書きにくいから」という至極どうでもいい理由によって歪められてしまうことになってしまいます。
{
"Move": {
"WalkSpeed": 10
},
"Health": {
"MaxHp": 100
}
}
/// <summary>
/// 敵のパラメータ
/// </summary>
[Serializable]
public struct EnemyParameter
{
public Move Move;
public Health Health;
}
/// <summary>
/// 移動についての定義
/// </summary>
[Serializable]
public struct Move
{
/// <summary>
/// 歩く速度
/// </summary>
public float WalkSpeed;
}
/// <summary>
/// 体力についての定義
/// </summary>
[Serializable]
public struct Health
{
public int MaxHp;
}
DTOを使う実装
こういった、「外部要因によるドメイン層の歪み」を防止するために用いるのがDTOです。
具体的には、Jsonの形式に則ったDTOオブジェクトを定義し、「Json→ドメインオブジェクト」と変換していた部分を「Json→DTO→ドメインオブジェクト」とDTOを仲介する形に変更します。
namespace Domain
{
/// <summary>
/// 敵のパラメータ
/// ドメインとしてはこのデータ構造で十分
/// </summary>
public struct EnemyParameter
{
public float WalkSpeed { get; }
public int MaxHp { get; }
public EnemyParameter(float walkSpeed, int maxHp)
{
WalkSpeed = walkSpeed;
MaxHp = maxHp;
}
}
}
public class EnemeyGenerator : MonoBehaviour
{
[SerializeField] public GameObject _enemyPrefab;
/// <summary>
/// 敵のパラメータのDTO
/// </summary>
[Serializable]
private struct EnemyParameterDto
{
public MoveDto Move;
public HealthDto Health;
}
[Serializable]
private struct MoveDto
{
/// <summary>
/// 歩く速度
/// </summary>
public float WalkSpeed;
}
[Serializable]
private struct HealthDto
{
public int MaxHp;
}
private async void Start()
{
// Jsonをファイルから読み込み
var json = await Task.Run(() => File.ReadAllText("enemy.json"));
// jsonからDTOにマッピング
var dto = JsonUtility.FromJson<EnemyParameterDto>(json);
// DTOからドメインオブジェクトにマッピング
var enemyParameter = new EnemyParameter(dto.Move.WalkSpeed, dto.Health.MaxHp);
// 敵を生成する
StartCoroutine(GenerateCoroutine(enemyParameter));
}
// 与えられたパラメータで敵を生成する
private IEnumerator GenerateCoroutine(EnemyParameter enemyParameter)
{
while (true)
{
var enemyObject = Instantiate(_enemyPrefab);
var enemy = enemyObject.GetComponent<Enemy>();
enemy.Initialize(enemyParameter);
yield return new WaitForSeconds(3);
}
}
}
もっとも注目してほしい点はここです。
// jsonからDTOにマッピング
var dto = JsonUtility.FromJson<EnemyParameterDto>(json);
// DTOからドメインオブジェクトにマッピング
var enemyParameter = new EnemyParameter(dto.Move.WalkSpeed, dto.Health.MaxHp);
一度jsonからDTOにマッピングしたのち、ドメインオブジェクトに変換しています。
一見手間に見えますが、この工程を踏むことによりドメインオブジェクトを保護することができるようになります。
このように、データをドメイン層の外に持っていく場合、または外からドメイン層にデータを持ってくる場合は、DTOを仲介させてデータの変換をあえて挟むことをおすすめします。
テストが書かれているか
ドメインを正しく定義できている場合、その部分は分解してテストを書きやすい場合が多いです。
（MonoBehaviourを使わないロジックを持ったクラスなど）
可能な限りテストを整備してください。
Unityプロジェクト全体
AssetBundleで用いるファイルを変更していないか
AssetBundleを用いる場合、共有するファイルやスクリプトを変に上書きしたり、metaファイルをいじってしまうとAssetBundle読み込み時に不具合が出てしまいます。
AssetBundleを利用している場合は、共通スクリプトなどに変更を入れていないかを確認します。
IL2CPPビルドでスクリプトがstripされないか
IL2CPPビルドを実行した場合、静的に呼び出されていないメソッドやクラスはコンパイルから除外されてしまいます。
とくにリフレクションでメソッドコールしていたりすると、実行時エラーになってしまう可能性があります。
自分でリフレクションを使ってなくても、依存しているライブラリが使っていることがあるので注意する必要があります。
回避策として、PreserveAttributeというものがあるので、該当しそうな箇所にはこれを設定しておきましょう。
(MONOビルドの場合はlink.xmlを指定するしかないです)
ビルドして動作確認しているか
UnityEditorでは動くが実機ビルドすると動かないということはよくあります。
プルリクを出す前に実機確認をしてください。
非プログラマのことを考えた作りになっているか
UnityEditorをプログラマではない人も操作する場合があります。
そのため、訳のわからないパラメータがInspectorViewに露出していたり、設定しなくていはいけないパラメータが複雑だったりすると使う人が混乱してしまいます。
「運用でカバー」も負債の一種なので、できるだけなくしたいです。
- Editor拡張でパラメータの設定が簡単になっている、設定を失敗していると警告がでる、など
- フールプルーフ的な設計になっている（enumで規定値を選ぶだけで済むようになっている）
- 各種パラメータの設定方法や、Editor上での操作方法についてドキュメントが整備してある
このような気配りがあると良いでしょう。
metaファイルを上書きしていないか
Unityは同一オブジェクトであるかの判定にmetaファイルを使用しています。
metaファイルを変に上書きすると謎のファイルが登場して参照がおかしくなったり、Inspector上で参照が外れたりしてしまいます。
データの移動を行う場合は必ずUnityEditorのProjectView上で実行するようにしてください。
また何かのリソースをProjectに追加する場合は必ずmetaファイルも含めてコミットするようにしてください。
Prefabやシーン構成を変更したときはGitHubに画像を貼ってくれるとうれしい
Prefabやシーン構成を変更しても、GitHub上ではすべてテキストデータの差分として表示されます。
そのため何がどう変更されたのか非常にわかりにくく、チェック漏れする可能性があります。
（手元にcheckoutして内容を確認するのも結構な負担になる）
可能であればPrefabをどのような設定に変更したのかといった画像ファイルを添付してくれると助かります。
まとめ
他人のコードを読むときに、自分がどのような点を気にしているかを言語化してみました。
チーム開発する方の手助けになればよいかなと思います。