汚いコードと対処法 - 君はコードなんか汚いと思いながら

あらすじ

徹夜明けの深夜テンションで書いた怪文書が思いの外多くの人の目に止まったようなので、実際にどういうコードが汚くて、どう改善できるのか、みたいな事を簡単にまとめてみる。

モジュール・クラス・変数の名前がおかしい

名前から全く想定できない作用がある、名前が嘘

例えば、validateForm()という名前のメソッドを実行すると、決済処理が完了してレシートが印字されるとケース。おまえはvalidationではない。でもvalidationなので、DBには保存しない。
（何を言っているんだ？？？ちなみに、外部APIやデバイスのコールはこのメソッドの中でできてしまうが、フレームワーク制約でDB更新はここではできない、みたいな状況でそういう事が起こる）

const blue = "#ff0000"、おまえは青色ではない。真っ赤なウソだ。

これは、しばしば致命的なバグにつながる。既存のblueを信じてコードを書くと大変な事になる。

対処法：名前は想定できる作用をもとにして定義し、嘘をつかないようにする。

亜種にコメントが嘘などもある。嘘をつかないようにする。コメントや名前は間違っていてもエラーが出ないので気をつける。

否定形、意味を一意に取れないboolean

isNotSucceededとかerrorという名前の変数がbooleanの場合、trueとfalseの意味がわからなくて混乱する場合がある。
これも、しばしば致命的なバグにつながる。機能の実装者によってerrorがtrueでエラーがあったり、errorがfalseでエラーがあったりすると、かなりつらい。

対処法：取り違えのない肯定形の名前にする。succeededとかhasErrorとか。

極端に長い

validateAndLockDatabaseAndUpdateRecordsIgnoreError()みたいな名前の関数は見たくない。認知負荷が高くなり、内容を正確に理解しにくく、改修時のエンバグやメンテナンスコストの増加につながる。

対処法：できる限り4単語以内で検討する。どうしても短くならない場合は、そもそも処理や役割が過剰に複雑ではないか・分割できないかという事を検討する。それでもどうしようもない事もあるが、なるべく分かりやすくする。

ただの記号すぎる

グローバル変数$aとか。これも改修時のエンバグやメンテナンスコストの増加につながる。ただ、これは結構難しいところもあり、システム全体で$aとはこれのことだ、みたいな明確な合意が形成できるならば、短いほうが認知負荷が減る場合もあるので、これまでに書いたものよりはマシな場合も多い。少なくとも嘘だったり理解不能だったりはしないので。（じゃあminifyされたJSを読めるかと言われると、しんどいが...）

対処法：原則として、変数名の長さは寿命（スコープの広さ）に比例させるイメージを持っておく。ループ変数などで明らかな場合も、一単語ぐらいにしておいた方が長い目で見るとわかりやすい場合が多いが、インデックス等で一文字の方が読みやすい場合もある。だいたいループ変数以外は一文字ではない方がよいかな。また、単語にするときは、まったく無意味な単語にはしない。

これは、単語の選び方みたいなものを意識する練習をすれば、例えば最初は変数名を考えるために少し止まってしまうかもしれないが、やがて無意識的に変数名をつけられるようになる。
また、実装中常に最初から適切な名前でなくても、後で全体を俯瞰することで適切な名前がわかるといったケースもあるので、初手としての名前を仮にして進めることは否定しない。どちらかというと、書いた後に適切な変数名に直したり、概念を整理して処理を最適化すればよい。少なくとも現在は、多くのエディタにリファクタリング機能が備わっているし、LLMにコード断片を食わせて適切な命名についての助言を求めることはできる。

とりあえず動いた状態で終わっている、あるいは実装者が挙動を理解していない

コード全体で流儀が不統一

これは様々な流儀について言えるが、例えば、業務上想定されるエラーのハンドリングをするときに、例外を送出するのか、戻り値でエラーを渡すのか。戻り値でエラーを渡す場合も、例えば0が正常だったり0が異常だったり。ほぼ同じ機能Aと機能Bで、ハンドルの仕方が異なっていると、それを呼ぶ実装者の勘違いによってバグが生じることがある。

対処法：できる限り、コード全体で均一にする。ただし、徹底ができない部分もあるので、モジュールやクラスやメソッド・関数など、単位が小さくなればなるほど確実に徹底する。

マジックナンバーがいたるところにある

意味ありげな数値がコード上に出てくるが、その意図がよくわからない場合などがある。後述の安易なdelayなどと組み合わさると、なぜ普段は3秒待つのにここだけ5秒待つのか？といった疑問が無限に発生するコードが生まれる。結果として、改修時のエンバグやメンテナンスコストの増加につながる。

対処法：なにかの数値や文字列を使用するときは、意味のある名前で定数として定義したり、コメントで意図を補うなどする。

インターネットやAIから拾ってくるタイプのコピペコード

インターネットやAIで得られたコード断片をコピペして、とりあえず動いたのでヨシ！という種類のコピペコード。
これは、コピペという行為が悪いというよりは、最終的に実装者が内容を理解しない状態で成果物になってしまうのが悪いということ。特にコードを単純にコピペしている場合は、変数名などの流儀が他と違ったり、純粋に不要な処理が混在したり、バグっていたりする。

（もちろん、参照元によっては著作権的にコピペ自体が悪いケースもあり得る。ただし、一般論として、日本国内では著作物として認められるケースは創作性がある場合に限られ、誰が書いても概ね本質的に同じような実装になるであろうコード断片については、ほぼ創作性は認めらないので、ソフトウェアの主要部分を剽窃するみたいな事でなければコピペ自体が悪という事ではない。逆に言えば、ある機能を実装するためにOSSの主要部分をまるまるコピペした、みたいな場合は当然著作権的に問題があるのだが、ここで想定しているのは公式やその他サンプルとして明示されたコード断片をコピペするような場合のイメージである。）

例えば、従来は注文受付を行っていたWebアプリで決済を行いたいとなり、注文を確定する処理で決済もすることになったとしよう。このとき、外部APIで決済を確定させる必要があるのだが、その外部APIをコールする実装をインターネットの海から探して、既存実装のvalidateForm()の中にコピペすると、記事冒頭に示した「名前から全く想定できない作用を持つメソッド」が爆誕する。
これは、本質的には実装をコピペする行為ではなくて、そもそもvalidateForm()で外部APIを呼べばヨシ！という考え方に問題があるのだが、よく併発する。

対処法：機能を実現できた後に、最低限セルフレビューする。

• 新しい実装方式を導入したのであれば使い方を公式で調べる
• 他の機能と書き方が違っていないか確認する
• メソッド名や全体の構造から考えて修正内容が適切であるかを確認する
  • もし今回の機能を全くの新規で実装した場合に、今のコードを書くか？不自然な書き方をしていないか？
• その他、これまでに示した観点（および、この後に示す観点）でおかしな箇所がないか確認する

スパゲティ非同期処理と安易なdelay

モバイルアプリ等では、UIスレッドとその他のスレッドを明示的に分けて考える必要があり、それによって非同期処理の実装が必要になる。この非同期/並列処理の実装が雑で、ある処理の後にどの処理が呼ばれるのかわからなくなる事がある。
この「わからなくなる」が、可読性の意味での「わからなくなる」であればそれはマシなケースで、本質的に「わからない」ケースがある。典型的なものとして、安易なdelayを入れるというケースがある。
例えば、モバイルアプリで外部APIとの通信を行う場合、処理Aを行って、外部APIと通信して結果を待ってから処理Bを行う必要があるが、この結果を待つ処理について、「とりあえず3秒待てば動くからヨシ！」という感覚で、固定で3秒待つ処理を実装されたりする。この待ち時間が仕様として保証されていない場合、処理A→結果通知→処理Bなのか処理A→処理B→結果通知なのかという処理順番が本質的に「わからない」事になる。汚いというか普通にバグっている
（ふわっと外部APIと書くと、通信遅延等も考えれば3秒固定なんてありえない、と思うのだが、例えば物理的に接続しているデバイスとの通信になると、とりあえず3秒待つみたいな実装を行う人はいくらでもいる）

また、待ち受け処理の間にキャンセル処理を入れようとすると、これも本質的に難しいので、てきとうな実装をしていると一層ひどいコードが出来上がってしまう。

対処法：非同期処理はレースコンディションなど本質的に難しいという事を理解したうえで、スパゲティにならないように注意してコードを書く。特に、絶対に保証しないといけない処理順序などを意識して、それをなるべく分かりやすく表現する。

分割が適切ではない

ここからは、設計的な要素、構造的な要素が増えてくる。（ただし、適切な名前がつけられていないケースも、設計的な問題でそもそも適切な名前をつけられないからそうなったみたいな場合があるので、名前の問題だからといって設計の問題ではないとは言えない）

単純に一つの処理が長い

たとえば、一つの関数の実装が5000行を超えるケースなど。認知負荷が大きくなり、改修時のエンバグやメンテナンスコストの増加につながる。
この5000という数値には諸説あり、一般的には20〜30ぐらいを閾値とする事が多い[要出典]が、個人的には100行ぐらいまではやむを得ないケースも多いように思う。パフォーマンス等でどうしても長くなるケースもあるが、長い処理はなるべく概念的に整理して簡単な処理の組み合わせになるように構造を調整する。

ネストが深い

function something() {
    if (condition1) {
        process1();
        if (condition2) {
            process2();
            if (condition3) {
                process3();
                if (condition4) {
                    // 略
                }    
            }
        }
    }
}

早期リターンや、処理の分割などでネストを浅くする。

function something() {
    if (!condition1) return;
    process1();
    if (!condition2) return;
    process2();
    if (!condition3) return;
    process3();
    if (!condition4) return;
    // 略
}

全く別の目的の処理が、分岐で無理矢理一つの処理にまとめられている

function doEverything(state) {
    if (state === 'create') {
        // とても長い処理
    } else if (state === 'update') {
        // とても長い処理
    } else if (state === 'print') {
        // とても長い処理、ていうかprintってなに？
    } else if (state === 'error') {
        // とても長い処理
    }
}

これは、フレームワークなど事情があって一つの関数/エンドポイントの中で分岐する必要があるケースもあるが、特にそういった事情があるわけでもないのに、なぜかこのような実装をして、呼び出し元では以下のようにして呼ぶケースがある。

function someFunction1() {
    // 略
    doEverything('create')
}
function someFunction2() {
    // 略
    doEverything('error')
}
// 略

これは、特に制約がなければ、以下のような構造の方がいい（関数名は対比のためてきとうにしているが、もっと適切な名称があればそれをつけるべき）

// 各処理の定義
function execCreate() {
    // とても長い処理
}
function execUpdate() {
    // とても長い処理
}
function execPrint() {
    // とても長い処理
}
function handleError() {
    // とても長い処理
}

// 呼び出し
function someFunction1() {
    // 略
    execCreate()
}
function someFunction2() {
    // 略
    handleError()
}
// 略

ある種のemitする処理や、メタ的にstateをハンドルする処理で元のような実装をするメリット・必然性はあるが（メインループの処理とか、reduxなどのreducerとか）、そういった制約もないのに、巨大なdoEverything()関数を作られても困ってしまう。分解できるものは分解する。（これはAPIの設計などでもそう思う）
これは、単純な可読性（？）だけではなくて、定義の参照などに影響する。修正後の実装であれば、例えばsomeFunction2()の中のhandleError()を直接定義参照できるが、修正前の実装では定義参照できない。stateがerrorの分岐を追うか...みたいになって探す労力が発生してしまう。

また、どうしてもこの構造になる場合、可能であればstateの型を、リテラルを用いて具体的に定めるようにする。マジックナンバーになりがち。

引数がめちゃくちゃ沢山ある

ライブラリのオプションみたいなケースではないのに、やたら引数が多い関数がある。このような場合は、そもそも無関係な処理が混在しているか、あるいはその引数たちのいくつかをクラスとしてまとめられる場合があるので、対応を検討する。
具体的な指針としては、引数の数が6以上になるとバグが有意に多くなると言われている。

全く同じ目的・概念の処理がいたるところにコピペされている

特定の処理を行うためのシーケンスのようなものがあったとして、それがいたるところにコピペされているような場合。なにか修正が必要になった場合に、それらのコピペの修正が漏れるとバグってしまう。
いわゆるDRYに違反しているケース。このような場合は、目的・概念を確認したうえで、同じ目的・概念の処理は共通関数などとしてまとめる。

一つのクラスのメンバー・メソッドが多すぎる（神、いわゆるゴッド）

業務アプリ等では、一つの画面に多くの機能性を持たせることが必要になるケースがあるが、画面の要素が適切にコンポーネント化されていないと、その画面を管理するクラスに無限にメンバー・メソッドが生えてしまう。これによって認知負荷が大きくなる。
ただし、子コンポーネントで必要になるデータがなにかを整理できないと、結局親コンポーネントですべてのデータを管理することになり、子コンポーネントに処理を委譲できず、画面を管理するクラスが神クラスになる事から逃れられない。神クラスを作らないようにするには、本質的にデータや処理委譲の構造を設計する必要がある。
この本質的な難しさと表裏一体になっているのが、コンポーネント分割し過ぎて必要な機能性が失われてしまう場合である。本来は兄弟や親が持つデータを使わないとうまく対応できないような処理があった場合に、思い切ってその処理が削られる、といった事が発生する。あるいは、現在のコンポーネント構造から、そのような兄弟や親が持つデータを使う修正は修正コストが高いので対応しない、といった判断が発生したりする。コンポーネント分割、あるいは単にコードを書くという事は、制約を増やす事でもあるのだ。

仮にコンポーネント分割された状態をきれいなコードとするとき、「汚いが追加要件を満たせるコード」と「きれいだが追加要件を満たせないコード」を比較すると、「比較的良いコード」は基本的には前者にあたるべき。こういった事情があって、技術力が十分でない場合には神クラスの方がマシという事になってしまう。神は常にすべてのデータを持っているので、やろうと思えば一応はなんでもできる。
このような状況で神にならないようにするには、目先のテクニックというよりは、構造・概念をきちんと整理する必要がある。
というのも、いま現在のコードの構造だけを見るとき、コードの構造としては色々な最適化の方法があるはずだが、適当に分割すると上述のとおり神の方がまだマシとなってしまう。そうならないように今後のソフトウェアの改修の方向に適した最適化を選ばないといけない。 そのために、概念的な整理が必要になる。
このとき、まず大前提として、名前を適切につけられる状態でないといけない。名前を適切に整理して、構造・概念を再整理して、分割可能な概念や機能を分割していく、という事をすることで、ようやく神クラスを"正しく"脱出することができる。
（この辺は、前の記事における「崩れにくい」と「積み上げやすい」が時に反する、という事象のひとつでもある）

実践編：ちょっとした問題の解答をどうリファクタリングするか

ここで、具体的にリファクタリングによってどうコードをきれいにしていくか、一つの関数をリファクタリングする例を挙げる。

問題

Leetcodeの問題
https://leetcode.com/problems/maximum-subarray/description/

整数の配列numsが与えられたとき、和が最大となるような部分列を探して、その和を返却せよ。
（原文： Given an integer array nums, find the subarray with the largest sum, and return its sum. ）
制約：
1 <= nums.length <= 10^5
-10^4 <= nums[i] <= 10^4

最初の解答

まず、候補になる部分列がどんな部分列か？という事を考えて、とりあえず符号が正の場合にはくっつけていけば良い（＝合計を取って置き換えればよい）、と考える。その後、同じ符号の場合をすべてくっつけて正負正負...という配列をつくっておいて、ある正にいるときに、この負を超えるべきか超えないべきか？という判定を繰り返して行けば答えが出る、という方針ができ、一旦何も考えずに方針通りに実装（10分ぐらい）
※以下、python

class Solution:
    def maxSubArray(self, nums: List[int]) -> int:
        left_max = max(nums)
        concat_values = [0]
        for num in nums:
            if concat_values[-1] <= 0:
                if num <= 0:
                    concat_values[-1] += num
                else:
                    concat_values.append(num)
            else:
                if num >= 0:
                    concat_values[-1] += num
                else:
                    concat_values.append(num) 
            loop = True
            while loop:
                loop = False
                if len(concat_values) > 1 and concat_values[-2] * concat_values[-1] > 0:
                    concat_values[-2] += concat_values[-1]
                    concat_values.pop()
                    loop = True
                elif len(concat_values) > 2 and \
                        concat_values[-2] < 0 and \
                        concat_values[-3] >= -concat_values[-2] and \
                        concat_values[-1] >= -concat_values[-2]:
                    concat_values[-3] += concat_values[-2] + concat_values[-1]
                    concat_values.pop()
                    concat_values.pop()
                    loop = True
                if left_max < concat_values[-1]:
                    left_max = concat_values[-1]
            if len(concat_values) > 1 and \
                    concat_values[-1] < 0 and \
                    concat_values[-2] > -concat_values[-1]:
                concat_values[-2] += concat_values[-1]
                concat_values.pop()

        return left_max

ただし、これはあまりにも冗長で無駄もあるので、ここからリファクタリングをしていく。

判定条件の簡素化

結合する・しないの条件は同じ符号か否かの判定でよいので簡素化できて、かつ結合した値を保持している配列のconcat_values[-3]を評価する必要はないことに気がつき、その部分を簡素化（5分）

class Solution:
    def maxSubArray(self, nums: List[int]) -> int:
        left_max = max(nums)
        concat_values = [0]
        for num in nums:
            if concat_values[-1] * num >= 0:
                concat_values[-1] += num
            else:
                concat_values.append(num)
            loop = True
            while loop:
                loop = False
                if len(concat_values) > 1 and concat_values[-2] * concat_values[-1] > 0:
                    concat_values[-2] += concat_values[-1]
                    concat_values.pop()
                    loop = True
            if left_max < concat_values[-1]:
                left_max = concat_values[-1]
            if len(concat_values) > 1 and \
                    concat_values[-1] < 0 and \
                    concat_values[-2] > -concat_values[-1]:
                concat_values[-2] += concat_values[-1]
                concat_values.pop()

        return left_max

concat_valuesの結合処理の削除

以前のconcat_valuesについてループで結合している処理がそもそも不要である事に気づいて、これも削除（10分）

class Solution:
    def maxSubArray(self, nums: List[int]) -> int:
        left_max = max(nums)
        concat_values = [0]
        for num in nums:
            if concat_values[-1] * num >= 0:
                concat_values[-1] += num
            else:
                concat_values.append(num)
            if len(concat_values) > 1 and \
                    concat_values[-2] > 0:
                concat_values[-2] += concat_values[-1]
                concat_values.pop()
            if left_max < concat_values[-1]:
                left_max = concat_values[-1]

        return left_max

最終解答

concat_valuesを保持する必要がないことに気づいて、最適化、変数名など含めて微調整（10分）
（この方法はカダネのアルゴリズムというらしい）
変数の名前なども、それによって変更されていることに注意する。

class Solution:
    def maxSubArray(self, nums: List[int]) -> int:
        max_sum = max(nums)
        current_sum = 0
        for num in nums:
            if current_sum < 0:
                current_sum = num
            else:
                current_sum += num
            if max_sum < current_sum:
                max_sum = current_sum

        return max_sum

ここで重要なのは、同じ人間が、最初のものと最後のものを出力すること。しかも、その間特に外部のデータを参考にしたりといった事をせず、何の調査もしなくても、ただ考えてコードを最適化するだけで、これだけアウトプットが変わり得る、ということ。
仮にコードをメンテナンスするとして、最初のコードと最後のコード、どちらをメンテナンスしたいかというと明らかに最後のコードだろう。
常にここまでの変化が起こる訳ではないにしても、業務のコードでも同じような質の差が実際に発生すると考える方が自然である。

まとめ

他にも汚いコードのバリエーションは沢山あると思うが、特に影響の大きそうなものを挙げてみた。ここに挙げたほとんどの内容は、しばらくの間それに注意してコードを書いていれば、ほとんど無意識でそのようなコードを書かなくなる、という種類のものである。つまり、練習すれば実装スピードが落ちるようなトレードオフはほとんどなく、なるべく汚くない方法を選べるようになる。
また、非同期処理やコンポーネント分割は本質的に難しいが、そうした難しい実装こそ、適切な名前のついた整理されたコードを見て考えることでより深い実装ができるようになる。難しい実装ほど、一旦仕上げた後に最適化する方法が無いか眺めて、その場でリファクタリングを検討する。
そういう行為の繰り返しで、自然と初手で書くコードが洗練されていき、短い時間で適切な実装ができる熟達者になっていく。
ただし、具体的な「汚い」の感覚についてはもっと細かいものもある。そうしたすべてを列挙することは難しく、実際にコードを書いたものを見てもらって指摘をしてもらうのが一番早い習得方法である。

おしまい。

前の記事：
https://zenn.dev/339/articles/4ba6794874d097

ほかの記事：
https://zenn.dev/339/articles/83caa21b9ad736

https://zenn.dev/339/articles/e3c174fdcc083e

https://zenn.dev/339/articles/ecc4986473ca88