こんなコードは駆逐しよう！

こんにちは。決済システムでエンジニアをやっております hoshino33 です。少し前ですが「進撃の巨人」のアニメが完結しましたね！私はまだ見ておりませんが、コードはほぼ毎日見ております。ということで今回はコードを駆逐（リファクタリング）していこうと思います。

はじめに

開発するとほとんどの場合コーディングするよりもコードリーディングする方に多く時間を取られると思います。複雑な仕様、無理な拡張、意図のわからないコード、など過去の負債や様々な要因でコードリーディングが難解な場合はよくあるはずです。ただ、現在自分がコーディングしているものももしかしたら数年後には同じような状態になっている可能性もあるはずです。そこで、今回はそうならないようにわかりやすいコードに焦点を当てた内容となっております。これから記載する内容は私が関わっているプロダクトのコードで気になったところを元に記載しております。

意味のない（わからない）命名

スペルが長いので省略されているというのをよく見かけます。が、残念ながらどんなに考えても答えにたどり着かない、なにを意味しているのかわからない命名に出会ったりもします。スコープが狭ければ問題ないのですが、スコープが広い場合は結構厄介です。

int ac;

Better スコープを意識する。省略せずに多少長くなっても適切な命名にする。スコープが狭ければその限りではない。

int autoCharge;

複雑な条件式

条件式が複雑かつ複数あり、どういう条件か読み取れない。

if (条件A && 条件B && 条件C && 条件D)
{
    // 処理
}

Better 条件をシンプルにするのはもちろん、条件をまとまりのある単位にしたり、条件式をそのまま記載するのではなくメソッドにして名称でわかるようにするのも一つの手です。

if (IsError)
{
    // 処理
}

bool IsError()
{
    return (条件A && 条件B && 条件C && 条件D);
}

値の意味がわからない（マジックナンバー）

急に出てくる1や2など。これがなにを意味しているのか分からない。コードリーディングの妨げになります。

if (payments == 1)

Better 定数などにして名称に意味をもたせ、わかるようにする。

enum Payments
{
    CreditCard = 1,
    Convenience = 2,
}

if (payments == Payments.CreditCard)

処理がとにかく長い

残念ながら数千行に及ぶメソッドがあります。こちらはいろいろな処理をしているため、コードリーディングはもちろん、修正も難しい状態となっております。

void Method()
{
    // 処理A
    // 処理B
}

Better 処理を分割する。大抵の場合1メソッド内でだらだらと複数の処理を行っているため、処理単位を見直して分割する。逆に分割しすぎると見にくくなる場合もあるので注意が必要。

void Method()
{
    MethodA();
    MethodB();
}
void MethodA()
{
    // 処理A
}
void MethodB()
{
    // 処理B
}

ネストが深すぎる

ネストが深いとコードリーディング中に迷子になる。ネスト内に膨大な量の処理があった場合はもうきつい。

void Method()
{
    if (条件A)
    {
        if (条件B)
        {
           if (条件C)
           {
               // 処理
           } 
        }
    }
}

Better ネストが深くならないように早期リターンを駆使する。できない場面では分割するのも一つの手。

void Method()
{
    if (条件A)
    {
        return;
    }
    if (条件B)
    {
        return;
    }
    if (条件C)
    {
        return;
    }
    // 処理
}

無理な共通化

ここだけ修正すればいいので楽と思われがちですが、無理な共通化がされていて、ある場面では使用されていないロジックまで見ないといけない場合がありました。また、肥大化の原因にもなり得ます。場合によっては様々なところで使用され、影響が分からなくなる場面もあります。 ※共通化がだめということではありません。

bool Method(int type)
{
    switch (type)
    {
        case 0:
            // 処理A
        case 1:
            // 処理B
        case 2:
            // 処理C
    }
    // 処理
}

Better なんでも共通化をするのではなく、共通化するべきか意図的に分けるかをしっかり検討する。

bool MethodA()
{
    // 処理A
}
bool MethodB()
{
    // 処理B
}
bool MethodC()
{
    // 処理C
}

変数は使用する直前に宣言する

昔のコードの名残っぽいですが、メソッドの先頭にメソッド内で使用する変数が全て宣言されていることがあります。こちらは人によってはわかりやすいといった考えがあるかもしれませんが、直前に宣言することで、コードリーディングする上ではどこで使われているかがすぐわかる。スコープが狭まる。といったメリットが大きいと思います。

int autoCharge;
// autoChargeを使わない処理
autoCharge = 1;

Better 必要なときに変数を宣言するようにする。

// autoChargeを使わない処理
var autoCharge = 1;

参照しかしない変数は再代入不可にする

値の書き換えがない変数を再代入可能で宣言している。

var tax = 0.1;

Better constなどで再代入不可にしておくと明示的にconstが付与されていた時点でこの変数は変更がないとわかります。

const double tax = 0.1;

その他

他にもありますが、今回はこのくらいにして機会があれば次回に続きを書きたいと思います。

おわりに

いかがでしたか？みなさまのプロダクトのコードに思い当たる部分はないでしょうか？対応しやすいものと対応しにくいものはあると思いますが、すぐに対応できるものがあったらすぐに対応したいところです。

仮に、開発者が5人いたとして、コードリーディングに2時間かかるとします。 5人 × 2時間 = 10時間

そこである開発者がコードリーディング後に1時間でリファクタリングを行い、1時間でコードリーディングできるようになったとします。すると4人の開発者は多くの時間をかけずに把握できるようになると思います。（1人 × 2時間 + 1時間） + （4人 x 1時間） = 7時間（3時間の短縮）

実際、こんなに単純ではないと思いますが、十分な費用対効果は得られると思います。なので、積極的にリファクタリングしていけるようにしていきたいですね。

ちなみに、決済システムではリファクタリングのみを行うといった時間がとれないため、改修が入った際に関連する部分を可能な限りリファクタリングし、コードの品質をあげるような取り組みを行っております。ただ、大きめのリファクタリングが必要な場合は行いにくいため、この部分を今後改善する必要が直近の課題と感じております。

We are hiring！！

ROBOT PAYMENTでは一緒に働く仲間を募集しています!!!

speakerdeck.com
www.robotpayment.co.jp