自分は、ゲーム開発においてコードを読みやすくするのはとても重要だと考えています。
- 読みにくいと修正が大変
→ ゲームのロジックは変わりやすいので頻繁に修正する - 大規模開発で他人のコードをいじる機会が多い
→ 他人のコードを理解するのは難しい(癖とか) - 読みにくいとバグが増える
→ バグがバグを呼び、どんどん修正しづらくなっていく
ここでは、自分がこれまでに失敗し改善してきた
を紹介します☆
目次
①短いコードよりも理解しやすいコードにする
技術レベルが上がっていくと、短いコードが書けるようになっていきます。
自分も短いコードが書けるのが嬉しくてどんどん短くしていったのですが、
ある日
自分では綺麗に書いていたつもりが他の人から見たらただの分かりづらいコードだったという。。思えばこれが自分のコードに疑問を持つようになったきっかけかもしれません。
例えば
int index = name.StartsWith(Const.BackButtonName) ? -1 : int.Parse(name.Replace(Const.ButtonPrefix, ""));
上は渡ってきたnameからindexを取得するコードですが、なんかごちゃごちゃして読みにくい(とにかく短くすることしか考えてなかった)
int index = -1;
if(!name.StartsWith(Const.BackButtonName))
{
index = int.Parse(name.Replace(Const.ButtonPrefix, ""));
}
こうする。要はnameがバックボタンの時以外はindexを取得する、というコードだから。
こっちの方がすんなり理解しやすい。
今では「短いか」より「理解しやすいか」でコードを組んでます。
②汎用的な名前を避ける
①を作るために昔のコードを持ってきてまさに思ったんですが
nameとかindexってなんぞや・・・？？public void OnClick(GameObject button)
{
string name = button.name;
int index = -1;
if(!name.StartsWith(Const.BackButtonName))
{
index = int.Parse(name.Replace(Const.ButtonPrefix, ""));
}
// ...この後にもいろいろ続く
}
あ、なるほど、ボタン名から選択されたインデックスを取得してるのね(ふむふむ)
public void OnClick(GameObject button)
{
var buttonName = button.name;
var selectIndex = -1;
if(!buttonName.StartsWith(Const.BackButtonName))
{
selectIndex = int.Parse(buttonName.Replace(Const.ButtonPrefix, ""));
}
// ...この後にもいろいろ続く
}
じゃあこうした方がよりいいかも(ついでにvarで左辺揃えた)。
目的に沿った名前にすることで理解しやすくなります。
ただ「この後にもいろいろ続く」長いコードだったのでリネームしましたが、すぐ終わるような処理だったらそのままでも良かったかもしれません(その方が簡潔なので)。
③コメントのためのコメントをしない
同僚にこんなことを言われたこともありました
当時は褒め言葉と思ってたんですが、時間が経ってふと
/// <summary>
/// フィールドシーン
/// </summary>
public class FieldScene : MonoBehaviour
{
/// <summary>
/// カメラ
/// </summary>
Camera camera;
/// <summary>
/// アンカー
/// </summary>
GameObject anchor;
/// <summary>
/// パネル
/// </summary>
GameObject panel;
/// <summary>
/// インフォ
/// </summary>
GameObject info;
/// <summary>
/// 矢印
/// </summary>
GameObject arrow;
}
これ意味ある・・・？見りゃ分かるでしょ(笑)
/// <summary>
/// フィールドシーン。プレイヤーの移動や敵とのバトルを扱う(チュートリアルやクエストは別シーンなので注意)。
/// </summary>
public class FieldScene : MonoBehaviour
{
Camera camera;
GameObject anchor;
GameObject panel;
// マップのステータス情報(気温、時刻、天候、等)
GameObject info;
// フッターボタンを左右に切り替える矢印
GameObject arrow;
}
こんな感じでコードから分からないことを書くと良さそう。
変数の数だけコメントがあるとそれだけで縦にものすごい長くなるし(読みにくい)。
てかこの場合の問題点って、名前を適当につけてるからってのもあると思うんですよね。
GameObject mapStatusInfo;
GameObject footerSwitchArrow;
こうすればコメントすらいらない。
わかりづらい変数名はコメントで説明するんじゃなくて、そもそもの名前を変えた方がいいと思います(変数名の方がいろんなところで使用されますし)。
④読む人の自然な流れを意識する
Animatorでモーションが終わったら処理を行いたい・・・。
モーションが開始してからモーション終了を検知しないと上手くいかなかった。
void Update()
{
// 終了モーションがある場合は監視し、終わったら初期化する
if(endMotion != "" && animator.GetBool(parameter))
{
var state = animator.GetCurrentAnimatorStateInfo(0);
if(isPlayingMotion)
{
// モーション開始済みで終了モーションに変わったら終わらせる
if(state.IsName(endMotion)) EndMotion();
}
else
{
// モーションが開始した
if(state.IsName(motion)) isPlayingMotion = true;
}
}
}
「開始モーション」↓
「終了モーション」↓
「終了処理(EndMotion)」じゃない？
void Update()
{
// 終了モーションがある場合は監視し、終わったら初期化する
if(endMotion != "" && animator.GetBool(parameter))
{
var state = animator.GetCurrentAnimatorStateInfo(0);
if(!isPlayingMotion)
{
// モーションが開始した
if(state.IsName(motion)) isPlayingMotion = true;
}
else
{
// モーション開始済みで終了モーションに変わったら終わらせる
if(state.IsName(endMotion)) EndMotion();
}
}
}
ということでif文の並び順を変えました。
こうすることで他の人が見た時に自然な流れで読めるかと思います。
特にゲームだと一連の流れで処理する機会が多いかと思うので有効です。
ただ並び順を変えたことで!isPlayingMotionのように否定が増えたことに注意です。
否定文は逆にして考えるので少し頭を使います(肯定の方がすんなり読める)。
今回は流れを分かりやすくしたかったのでこうしましたが、状況によるかと思います。
⑤誤解されない名前を付ける
bool FriendCheck(int userId)
{
foreach(var friendId in friendIds)
{
if(friendId == userId) return true;
}
return false;
}
ということでなんの疑問も持たず作ったわけですが、このFriendCheck、良くないですね〜。
メソッド名からフレンドの「何を」チェックするのか分からないから。
このユーザがフレンドに含まれてるかを判定したいわけだから
bool IsFriend(int userId)
もしくは
bool ContainsFriend(int userId)
の方が直感的かもしれません。
boolの名前は頭にis・has・can・shouldをつけると分かりやすくなりやすいです。
誤解されない名前をつけるには codic がオススメです☆
（名前の最後に「〜か」と入力するだけでboolの名前にしてくれます）
⑥出来たコードを一歩下がって全体を見る
10年前、自分が初めて作ったゲームでの話です(その頃はガラケー)。
ガラケー時代はまだキーボードだったのですが、ユーザ入力中にクリアキーを押すとフリーズする報告がありました。
ということで、// クリアキーで抜けてIMEイベントが飛ばない〜のようにバグ修正をしました。
void UpdateUserInput()
{
if(isUserInput)
{
// ユーザ入力中
if(userInput != UserInput.None)
{
// 入力終了
isUserInput = false;
if(userInput == UserInput.Commit)
{
// テキスト保存
SaveUserInputText(userInputText);
}
}
else
{
// クリアキーで抜けてIMEイベントが飛ばない機種対策として決定キーを押したら終了するようにしとく
if(Keyboard.IsDown(Key.Select))
{
isUserInput = false;
}
}
}
}
見返してて理解しにくいなぁ、、と。今ならこうします。
void UpdateUserInput()
{
if(!isUserInput) return;
if(userInput == UserInput.None)
{
// クリアキーで抜けてIMEイベントが飛ばない機種対策として決定キーを押したら終了するようにしとく
if(Keyboard.IsDown(Key.Select))
{
isUserInput = false;
}
}
else if(userInput == UserInput.Commit)
{
// テキスト保存
isUserInput = false;
SaveUserInputText(userInputText);
}
else if(userInput == UserInput.Cancel)
{
// キャンセル
isUserInput = false;
}
}
まずネストが深いんですよね。なのでif(!isUserInput) return;のように早めに返して1つ浅くしました(このメソッドはユーザ入力中じゃなければ処理する必要がない)。
次に流れが把握しづらいんですよね。
要は「保存」だったらテキスト保存、「キャンセル」だったら何もしない、ただし「クリアキーで抜けちゃった」場合はユーザ入力を強制的に終わらせる、ということなので、処理順を整理しました。
if(userInput == UserInput.Commit)
if(userInput == UserInput.Cancel)
// ↓
if(userInput != UserInput.None)
主にコードを短くするためにこの辺を共通化したことや、そこにバグ修正を加えたことで煩雑になった気がします。
出来たコードは一歩下がって全体を見ると、問題点が見えてきたりします。
⑦困難の分割
int GetRequestedUserBadgeCount()
{
int badgeCount = 0;
for(int i = 0; i < AppData.Instance.User.MenuDTO.FriendDTO.RequestedUsers.Count; i++)
{
AppData.Instance.User.MenuDTO.FriendDTO.RequestedUsers[i].IsNew = false;
for(int j = 0; j < AppData.Instance.User.MenuDTO.FriendDTO.RequestedSaveUserIds.Count; j++)
{
if(AppData.Instance.User.MenuDTO.FriendDTO.RequestedUsers[i].UserId == AppData.Instance.User.MenuDTO.FriendDTO.RequestedSaveUserIds[j])
{
badgeCount++;
AppData.Instance.User.MenuDTO.FriendDTO.RequestedUsers[i].IsNew = true;
break;
}
}
}
return badgeCount;
}
このコードはどうやらソシャゲとかによくあるこういうバッジを取得するコードらしい(このゲームのコードじゃないよ)。承認待ちのユーザの数を取得するメソッドのよう。
しばらく考えてようやく理解できました(難解)。
int GetRequestedUserBadgeCount()
{
var badgeCount = 0;
var requestedUsers = AppData.Instance.User.MenuDTO.FriendDTO.RequestedUsers;
var requestedSaveUserIds = AppData.Instance.User.MenuDTO.FriendDTO.RequestedSaveUserIds;
foreach(var user in requestedUsers)
{
user.IsNew = false;
foreach(var saveUserId in requestedSaveUserIds)
{
if(user.UserId == saveUserId)
{
badgeCount++;
user.IsNew = true;
break;
}
}
}
return badgeCount;
}
こういう場合は requestedUsers requestedSaveUserIds のように小さい名前に分割する。
そうすることで必要な事柄に集中出来ます。
それと
foreach(var requestedUser in requestedUsers)
foreach(var requestedSaveUserId in requestedSaveUserIds)
// ↓
foreach(var user in requestedUsers)
foreach(var saveUserId in requestedSaveUserIds)
細かいですが途中からrequestedの文字も省略した点。
考える際に必要なのは「ユーザ」と「保存ユーザID」のみなので、より読みやすいように省略しました(今回は短いコードだったので省略しましたが、長いコードの場合は正確な名前にしておいた方がいいかもしれません)。
if(user.UserId == saveUserId)
// ↓
var isNewUser = (user.UserId == saveUserId);
if(isNewUser)
のようにロジックを説明する説明変数に入れても分かりやすいと思う(ゲームだとバグ修正で追加追加でif文の判定が長くなっていくからね。。)
⑧目に優しくする
// ボタンを作成
GameObject CreateButton(string fileName, Transform parent, string text, Vector3 pos)
{
GameObject go = Instantiate(Resources.Load<GameObject>($"Prefabs/{fileName}"));
go.transform.parent = parent;
go.name = fileName;
UILabel label = go.transform.Find("Label").GetComponent<UILabel>();
label.text = text;
go.transform.localPosition = pos;
go.transform.localScale = Vector3.one;
return go;
}
// ボタンを作成
GameObject CreateButton(string fileName, Transform parent, Vector3 pos, string text)
{
var go = Instantiate(Resources.Load<GameObject>($"Prefabs/{fileName}"));
go.name = fileName;
var t = go.transform;
t.parent = parent;
t.localPosition = pos;
t.localScale = Vector3.one;
var label = t.Find("Label").GetComponent<UILabel>();
label.text = text;
return go;
}
下の方が流れが把握しやすいかと思います。
- 関連するコードをまとめてブロックにする
varを使って左辺を揃えたり、=を整列する- 処理する順番に並び替える
posを追加したんだと思う(だから順番がおかしくなってる)⑨名前の長さを決める
/// <summary>
/// つながっている「ぷよ」を取得
/// </summary>
/// <param name="id">接続ID</param>
List<Puyo> GetConnectPuyos(int id)
{
// ...重い検索処理が長く書かれている
}
Getってなんか軽いな(今回はただ取得するってより、重い検索処理してるし)。
ConnectPuyosは「つながったぷよ」で、idは「これから探すためのID」だな(そもそもこのメソッドは長いのにidなんて簡素な名前を付けちゃってる)。
/// <summary>
/// つながっている「ぷよ」を探す
/// </summary>
/// <param name="connectionId">接続ID</param>
List<Puyo> FindConnectedPuyos(int connectionId)
{
// ...重い検索処理が長く書かれている
}
こんな感じ。じゃあこれは？
/// <summary>
/// その場所にぷよを置けるか
/// </summary>
/// <param name="fallingPuyoX">落下中のぷよX</param>
/// <param name="fallingPuyoY">落下中のぷよY</param>
bool CanPutPuyo(int fallingPuyoX, int fallingPuyoY)
{
return (puyos[fallingPuyoX, fallingPuyoY] == null);
}
/// <summary>
/// その場所にぷよを置けるか
/// </summary>
/// <param name="x">落下中のぷよX</param>
/// <param name="y">落下中のぷよY</param>
bool CanPutPuyo(int x, int y)
{
return (puyos[x, y] == null);
}
下でいい気がする。
スコープが大きい場合は具体的な名前をつけた方が無難で、スコープが小さい場合は短くしちゃっても大丈夫かもしれません(迷ったら誤解のない名前にしておいた方が安心)。
⑩コードを小さく保つ
Character.cs
abstract class Character<T> : MonoBehaviour where T : CharacterDTO
{
[SerializeField] BattleScene scene;
protected T dto;
public T DTO => dto;
public Character(T dto)
{
this.dto = dto;
}
public void Kill()
{
if(dto is EnemyDTO)
{
(this as Enemy).StopAnimation();
}
gameObject.SetActive(false);
if(dto is PlayerDTO)
{
(dto as Player).UpdateStatus(Result.Lose);
}
else if(dto is EnemyDTO)
{
scene.MoveResult(Result.Win);
}
}
}
class Player : Character<PlayerDTO>
{
public Player(PlayerDTO dto) : base(dto) { }
public void UpdateStatus(Result result) { }
}
class Enemy : Character<EnemyDTO>
{
public Enemy(EnemyDTO dto) : base(dto) { }
public void StopAnimation() { }
}
いや〜ひどいコード。。(多少脚色してますが、ほぼこんな感じのコードでした)
このコードを見やすくしていきます。
Character.cs
abstract class Character<T> : MonoBehaviour where T : CharacterDTO
{
[SerializeField] BattleScene scene;
protected T dto;
public T DTO => dto;
public Character(T dto)
{
this.dto = dto;
}
public void Kill()
{
if(dto is EnemyDTO)
{
(this as Enemy).StopAnimation();
}
gameObject.SetActive(false);
if(dto is PlayerDTO)
{
(dto as Player).UpdateStatus(Result.Lose);
}
else if(dto is EnemyDTO)
{
scene.MoveResult(Result.Win);
}
}
}
Player.cs
class Player : Character<PlayerDTO>
{
public Player(PlayerDTO dto) : base(dto) { }
public void UpdateStatus(Result result) { }
}
Enemy.cs
class Enemy : Character<EnemyDTO>
{
public Enemy(EnemyDTO dto) : base(dto) { }
public void StopAnimation() { }
}
まず、なぜか1ファイルにまとめられていたので「1ファイル1クラス」にします。
1ファイルに複数クラスが入ってると、コード行数が増えてのちのち読みづらくなります。
Character.cs
abstract class Character<T> : MonoBehaviour where T : CharacterDTO
{
[SerializeField] BattleScene scene;
protected T dto;
public T DTO => dto;
public Character(T dto)
{
this.dto = dto;
}
}
Player.cs
class Player : Character<PlayerDTO>
{
public Player(PlayerDTO dto) : base(dto) { }
public void Kill()
{
gameObject.SetActive(false);
UpdateStatus(Result.Lose);
}
void UpdateStatus(Result result) { }
}
Enemy.cs
class Enemy : Character<EnemyDTO>
{
public Enemy(EnemyDTO dto) : base(dto) { }
public void Kill()
{
StopAnimation();
gameObject.SetActive(false);
scene.MoveResult(Result.Win);
}
void StopAnimation() { }
}
次にKillメソッドをそれぞれのクラスに分割します。
元々、共通処理が増えていくだろうと予想して共通化しましたが、結果、1メソッドに複数処理が混在するカオスメソッドに。。
Player.cs
class Player : MonoBehaviour
{
PlayerDTO dto;
public PlayerDTO DTO => dto;
public Player(PlayerDTO dto)
{
this.dto = dto;
}
public void Kill()
{
gameObject.SetActive(false);
UpdateStatus(Result.Lose);
}
void UpdateStatus(Result result) { }
}
Enemy.cs
class Enemy : MonoBehaviour
{
[SerializeField] BattleScene scene;
EnemyDTO dto;
public EnemyDTO DTO => dto;
public Enemy(EnemyDTO dto)
{
this.dto = dto;
}
public void Kill()
{
StopAnimation();
gameObject.SetActive(false);
scene.MoveResult(Result.Win);
}
void StopAnimation() { }
}
最後にCharacterクラスを削除しました。
こうすることで継承がなくなり、1つ1つのクラスが細分化されます(詳しくはコンポーネント指向をご覧ください)。
最後に
自分も初心者の頃はなんとか上手く書こうと必死でした。
その結果が「分かりづらいからなぁ・・・」で、正直凹みました。。
じゃあ何がきっかけで良くなっていったかっていうと、この本に出会えたからなんですよね。
リーダブルコードは「強制はしないけどこういう風にしたらいんじゃない？」みたいに優しく語りかけてきます。指摘も具体的で、ページ数も多くないので読みやすいです。
初心者の方は一度読んでみることをオススメします☆
〜オセロを作りながらゲームのプログラムを学ぼう〜
そんな人のためにこの本を作りました。
オセロを一から作りながら実践形式でプログラムを学べる本です。
すでに完成したプロジェクトを説明するのではなく、実際に作りながら説明していきます。
一緒に手を動かしながら、プログラムを覚えていきましょう🌟
