新卒2年目までに学んだ、コーディングで意識すること

本記事はCraft Egg Advent Calendar 2021の12/3の記事です。
12/2の記事は@ishiguro_takuyaさんの[Unity] UniversalRenderPipelineについて調べた覚え書き
でした。

はじめに

株式会社Craft EggでUnityクライアントエンジニアをしている豊田です。

今回は私が新卒入社してから現在の2年目までに、プルリクエストでチームからもらったコメントや技術書をなどの学びを元に、コーディングで気をつけていることをまとめてみました。

before/afterのコードの例を出すことで、新卒エンジニアが遭遇しやすいコーディングアンチパターンとその改善例の参考になれば幸いです。
コーディング規約やしきたりはチームによって異なりはしますが、できるだけ一般的な内容を取り上げました。

※本記事で取り上げるコードはサンプルです。重要でない部分の命名は「hoge」としたり、簡略化しています。

表面上の改善

リーダブルコードには、第1部に「表面上の改善」が紹介されています。
命名規則やコードの体裁など、第三者（未来の自分自身も）がコードを理解するために最初に意識できる観点だと思います。

適切な変数名にする

• 意味が分かる命名にする
• boolは「is~」「should~」「can~」「exists~」「has~」など
• コールバックは「onCloseDialog」など「on~」
• 略しすぎない。一般的な略称ならOK（× s_pos → ◯ startPosや、startPosition）
• 英文法に誤りが無いか気をつける（× onClosedDialog → ◯ onCloseDialog）

など

適切な関数名にする

• 基本的に動詞始まりにする
• 初期化は「Initialize」「Setup」
• 生成は「Create~」
• 登録は「Register~」
• 変換は「ConvertHogeToFoo」
• 「Add◯◯」なら「Remove◯◯」というように、名前と処理を対にして理解しやすくする
• 「~IfNeed」「Check~」のような命名の関数を避ける（処理を分けられないか検討する）
• 英文法に誤りが無いか気をつける

など

一次変数でも分かりやすい命名にする

一次変数などスコープが小さくてもできるだけ意味の分かる命名にします。

before

UserData u = GetUserData(userId);

after

UserData userData = GetUserData(userId);

※ラムダ式の中でも同様

before

filterUsers = userList
    .Where(x => x.Id % 2 == 0)
    .ToList();

after

filterUsers = userList
    .Where(user => user.Id % 2 == 0)
    .ToList();

ネストを浅くする

例外は先に早期returnすることで思考をクリアにできます。

before

private void Hoge()
{
    if (hoge != null)
    {
        Foo();
        if(foo != null)
        {
            // メインの処理が続く
        }
    }
}

after

private void Hoge()
{
    if (hoge == null)
    {
        return;
    }

    Foo();

    if (foo == null)
    {
        return;
    }

    // メインの処理が続く
}

なお、早期returnは関数の出口が複数でき、利用側の期待する処理が行われず罠にハマることがあるため、何でも早期returnするのではなく、処理を分離できないか検討するなどしてケースバイケースで用いるのが良いと思います。

三項演算子を使う

before

if (IsHoge())
{
    foo = bar;
}
else
{
    foo = null;
}

after

foo = IsHoge()
    ? bar
    : null;

foreachをLINQに置き換える

before

private List<ItemData> CreateItemDataList(List<uint> itemIds)
{
    List<ItemData> itemDataList = new List<ItemData>();
    foreach (uint id in itemIds)
    {
        itemDataList.Add(new ItemData(id));
    }

    return itemDataList;
}

after

private List<ItemData> CreateItemDataList(List<uint> itemIds)
{
    return itemIds
        .Select(id => new ItemData(id))
        .ToList();
}

配列やListに対してforeachで処理する場合の多くはLINQで書けます。

文字列補間を使う

C#の6.0以上で使える文字列補間は、書きやすく見やすいです。
https://docs.microsoft.com/ja-jp/dotnet/csharp/language-reference/tokens/interpolated

before

Debug.LogError("userId : " + userId + ", userName : " + userName + "のユーザでエラー");

after

Debug.LogError($"userId : {userId}, userName : {userName}のユーザでエラー");

コメントアウト

自明なコメントは書かない

Initialize(); // 初期化
と書かれていてもコメントの有無で情報量が変わりません（むしろノイズになりうる）。

できるだけコードで説明することを心がけます。

処理の内容ではなく理由を書く

特殊な実装をしていたら、なぜそうしているかのコメントがあると、読んだ人をびっくりさせません。

なお、低レベルのコードを要約したり、処理のまとまりを説明する場合など、ケースによって処理の内容は書くことはあると思います。

問題のあるコードにコメントを書く

こちらも読んだ人をびっくりさせることを防ぎます。
やむを得ず綺麗じゃない実装をしているのなら、しれっとコードに紛れ込ませるのではなくTODOやHACKなどを添えて問題点を説明します。

安全性、Nullチェック

Get関数は失敗を考慮する

before

UserData userData = GetUserData(userId);
userData.HogeMethod();

この例だと、userDataの取得に失敗した時にNullエラーになりかねないので、Nullチェックを挟みます。

after

UserData userData = GetUserData(userId);

if (userData == null)
{
    Debug.LogError( "UserData is null. userId : " + userId );
    // 場合に合わせて例外処理を行う
    return;
}

userData.HogeMethod();

Null演算子を使って簡潔にする

before

if (userData != null && userData.Card != null)
{
    // userData.Cardにアクセスする処理
}

after

if (userData?.Card != null)
{
    // userData.Cardにアクセスする処理
}

※ リストのNull、空チェックも同様に簡潔にできます

before

if (userList != null && userList.Count != 0)
{
    // userListにアクセスする処理
}

after

if ((userList?.Count ?? 0 ) != 0)
{
    // userListにアクセスする処理
}

なお、MonoBehaviourなどUnityEngine.Objectを継承したものに対してのnullチェックは以下のような問題もあるので、Null演算子は使わずに== nullを使います。

エラー通知は必要な時にはっきり行う

ログは、エラーが起きた時にエラー出力をさせるなど、必要な時に行います。
正常挙動である時はログは出力させません。

なお、追いにくい低レイヤの処理には正常ログを入れる場合もあるかと思います（問題が起きた時に追いやすくする）。

制御の改善

処理の対象を絞ってLoop処理を回す

before

foreach (var hoge in hogeList)
{
    if(!hoge.IsFoo)
    {
        continue;
    }

    hoge.HogeMethod();
}

先にIsFooで絞ってからforを回したほうが、どういうデータを処理したいループなのかが分かりやすいです。

after

foreach (var hoge in hogeList.Where(hoge => hoge.IsFoo))
{
    hoge.HogeMethod();
}

状態を保持する変数はenumで定義する

フラグを乱立すると状態が分かり辛く、ソースを読み解くのが大変になります。

before

private bool isProcessing;
private bool isInterrupted;
private bool isInitialized;

after

private enum StateType
{
    None,
    Processing,
    Interrupted,
    Initialized
}

private StateType currentState;

設計の改善

モジュールを「純粋」にして、モジュール間を「疎遠」にすることを意識します。
凝集度と結合度の話ですが、以下の要素は基本的に凝集度を高く結合度を低くするための実践例だと思います。

長すぎるメソッド、クラスを書かない

一つの関数が長くなっているのであれば、まず分離できないかを検討します。

before

public void Setup()
{
    // UI、コンポーネントの初期化が数行に渡って書かれる
    ...
    // ロードの準備が数行に渡って書かれる
    ...
    // ロード処理が数行に渡って書かれる
    ...
}

after

public void Setup()
{
    SetupComponents();
    PrepareLoad();
    Load();
}

必要ないものはpublicにしない

クラスが、利用側の知る必要のない内部の詳細部分を隠蔽すれば、やりとりがシンプルになりコード全体の複雑性を下げることができます。利用側からみても、使い方がシンプルになり使い勝手が良くなります。

まずprivateで書いてみて、公開する必要があればpublicにする、がいいかもしれません。

ロジックとデータは近くに置く

before(データ)

public class PurchaseData
{
    // 単価
    public uint Price { get; }
    // 個数
    public uint Count { get; }

    // コンストラクタは省略
}

before(利用側)

// 単価100円、5つのデータとする
PurchaseData purchaseData = new PurchaseData(100, 5);
Debug.Log($"合計金額は{purchaseData.Price * purchaseData.Count}円です")

このbeforeの例では、データとなるPurchaseDataクラスと、利用側の2つのコードがあります。
データのクラスは、DB由来のモデルクラスと捉えてもいいです。
改善したいのは、Debug.Log内の金額の計算を利用側で行っている点で、ロジックがデータの外側に実装されてしまっています。

after(データ)

public class PurchaseData
{
    // 単価
    public uint Price { get; }
    // 個数
    public uint Count { get; }

    public uint GetAmount()
    {
        return Price * Count;
    }

    // コンストラクタは省略
}

after(利用側)

// 単価100円、5つのデータとする
PurchaseData purchaseData = new PurchaseData(100, 5);
Debug.Log($"合計金額は{purchaseData.GetAmount()}円です")

afterでは、合計金額を計算するロジックをデータのクラス側に移動しました。

例えばこれに、「軽減税率を適用するか」という「データ」を増やし、消費税計算の「ロジック」の実装が必要になった時に、改修の対象はPurchaseData内のみですので、利用側はロジックの変更を知らなくて済みます。

引数で処理の内容が変わる関数を避ける

以下のように引数を元に処理が変わる関数だと、利用側が関数のロジックを把握する必要があり、ブラックボックス化できません（制御結合になっている）。

制御結合になっている例

public void hoge(bool flag)
{
    if(flag)
    {
        // 処理Aが続く
    }
    else
    {
        // 処理Bが続く
    }
}

ただし、システムによっては、制御結合を避けられない場合もあるので、集まっている機能を精査しながらより結合度を下げられないか検討します。

無駄な引数を送らない

before例ではスタンプ結合になっています。関数にはできるだけ使うものだけ送るようにします。

before

public void SetUserNameLabel(UserData userData)
{
   userNameLabel.text = userData.Name;
}

after

public void SetUserNameLabel(string userName)
{
   userNameLabel.text = userName;
}

Unity関連

GetComponent、Find系の関数を使わない

パフォーマンスに悪いので、事前にSerializeFieldで参照を持たせます。

Update()は使わない

コールバックやコルーチンで実現できないか検討をします

Animatiorの引数はhash値を使う

Tips的な項目ですが、animatorの引数はhash値を使った方がパフォーマンス的に良いです。
Animator.StringToHash で取得した値を保持しておくのがミソ（先輩の言葉を引用）

before

private void PlayRunAnimation()
{
    animator.Play("run");
}

after

private readonly int AnimationHashRun = Animator.StringToHash("run");

private void PlayRunAnimation()
{
    animator.Play(AnimationHashRun);
}

さいごに

コードの可読性、保守性、安全性や柔軟性を上げるための手法、設計の原則は今回記事に取り上げたことの他にも様々あると思います。
業務でのプルリクエストでのコメントで得た学びの他、「リーダブルコード」と「プリンシプルオブプログラミング」からも執筆にあたって参考にしました。これらの書籍は1年目で読んだのですが、とても勉強になりました。

今回は局所的なコードの改善事例を取り上げましたが、今後インタフェースを使った実装の分離や、デザインパターンを用いた実践なども体系的にまとめてアウトプットしてみたいです。

アドベントカレンダーの明日の記事は @kai_yamamoto です。お楽しみに！

参考文献