「人を憎まず、コードを憎む。」

そうは言われても誰もが一度ならずこう思った経験をお持ちではないでしょうか？
海○先生はどんなコードを見てお怒りになられたのでしょうか。
そしてどうしたらいいでしょうか。

そんなコードを書いた私が改めて自分のコードと行動を振り返り、○原先生を怒らせないようなコードが書けるよう、まとめてみたいと思います。
皆さんにとっては当然のことをつらつらと書いておりますが、ご容赦いただければ幸いです。

読みにくいコードって？

一つの処理を読むのにスクロールが必要なほど長い。その行数、100行以上。
if文の条件式が長い。
if文の分岐が多い。
if文の中にif文があってその中に・・・無限ループって怖くね？
メソッド名や変数名が抽象的。

だいたいこんな感じではないでしょうか。

読みにくいコードを書いてなにが悪い！

まず読みたくない。
仕事だから仕方なくコードに目を通す。それもすごく集中して。
「わけがわからないよ」と言い、疲れてコーヒーを飲みに行きます。目が痛いです。そして意を決してもう一度目を通します。
実装したいのにコードを理解することに時間がかかり、余計な時間が浪費されていきます。
いざ実装に取り掛かろうとすると怖いです。そしてもう二度とこのコードに関わりたくないと感じます。
困ったことにこの現象はコードを書いた本人も同様です。
書いた本人が他チームに異動してたらこの怒りをどこにぶつけたらいいか分からない（ｲﾔﾀﾞﾅｧ...ｼﾞｮｳﾀﾞﾝﾃﾞｽﾖ？ｼﾞｮｳﾀﾞﾝ...）

本来は少なく済む工数が、「複雑怪奇で読みにくいコードを理解する」という余計な作業のために膨らみ、機会損失が生じてしまうわけです。
さらには、追加した機能と既存機能が化学反応を起こして新たなバグを生み出しかねません。
そしてそのバグを治すためにまたコードをじっくり読み直して・・・まさに負のスパイラル！！

個人の趣味で書くのなら別に自己の満足が満たされればいいのですが、チームで開発したり、納品後は顧客が管理するのであれば、読みにくいコードの罪は非常に深いですね。

読みにくいコードはどうやってできるの？

例えばスマホゲームですとクエストがありますが、クエストを開始する処理では

期間中のクエストを開始しようとしているか？
このクエストをプレイする条件をクリアしているか？
どんな敵が出るか？
敵を倒すとどんな報酬が出るか？
クエストメンバーに不正はないか？
クエストに持っていくアイテムに不正はないか？

といったことを行います（ゲームによっては他にもあるかと思いますが）。
これら色々な処理を一つのメソッドにそのまま書けば読みにくいコードができます。
実は、これらの処理を一つのメソッドに書くのはありではありますが（クエストコントローラー的な役割にして、これをAPIで呼び出せばいいので）、あまりに素直に書きすぎるのは危険です。

読みにくくないコードを書くには？

自分が書いたコードを負の遺産としないように私が現在行っている修正作業はこんな感じです。

機能ごとにメソッドを作成する

コードが長いのは様々な機能を実装しているからです。
なので、素直にそのままコードを書かず、機能ごとに

期間中のクエストか？ => check_quest_period()
クエストプレイ条件を満たしているか？ => is_playable_quest()
どんな敵が出現するか？ => select_enemy()

のようにメソッドを作成して、それを呼び出します。

こうすることで、例えば「敵の抽選方法に新しい仕様を追加」する必要があった場合、他の処理は読まずにselect_enemy()だけをひとまず見ればいいので素早く作業に取り掛かれます。
select_enemy()が長ければおそらくこの中でも複数の機能がそのまま実装されている可能性が高いので、機能ごとにメソッドに分割してあげましょう。

if文をメソッドにする

if文の条件が長い・分岐が多いのは、仕様によっては仕方がないものもあります。
その場合はif文をメソッドにすれば、メソッド名を見ただけで何をしているのか理解できるので、if文の解読の手助けになります。

例えばクエストの処理ですが

quest_start.py

class QuestStartManager(object):
    @classmethod
    def quest_start(cls, player, quest, now_datetime)
        # クエストが期間中かチェック
        if quest.start_datetime > now_datetime or now_datetime > quest.end_datetime:
           raise OutOfPeriodError

        # クエストのプレイ条件を満たしているかチェック
        if player.level < quest.need_level or quest.need_clear_quest_id not in player.clear_quest_ids or （書ききれないので省略）:
           raise PlayerQuestStatusIsNotPlayableError

        （以下ズラズラとしたコード省略）

これを見ただけでなんかもう読みたくないですね・・・たった数行なのに！
if文・・・恐ろしい子！

quest_start

class QuestStartManager(object):
    @classmethod
    def quest_start(cls, player, quest, now_datetime):
        check_quest_period(quest, now_datetime)
        is_playable_quest(player, quest)

これだといかがでしょう？
if文が直接書かれているよりも読みやすいのではないでしょうか。
各メソッドの中ではif文が書かれておりますが、メソッド名から「期間チェックだな」とか「クエストをプレイできるかチェックしてるのかな」っていうのが想像できるかと思います。

具体的なメソッド名や変数名を書こう

下記のような短いコードであれば許容されるとは思います（これはあくまでも例であって、実際はtry...exceptで書きます。念のため弁解しておきますｗ）。

quest.py

class Quest(models.Model):
    @classmethod
    def get_quest(cls, quest_id):
        obj = cls.objects.get(quest_id)
        if obj:
            return obj
        else:
            return None

しかしこのobjという変数名を長い処理のなかで書いたらどうなるでしょう？
スクロールしていくうちにこの「obj」が何箇所にも出てくると不安になってきますよね。
「どのobjだよ！！」
ってツッコミを入れたくなりますよね。

メソッド名もただ単に get_obj() とか update_obj() とかだと、どのobjを取得、更新しようとしているかわからず、また、このコードの信頼性を疑ってしまいますね。

名前を決める際、名前が異様に長い（動詞が複数ある）とか名前を考えているが、どういう名前がしっくりくるか悩む場合はほぼメソッドに機能を詰め込みすぎていますので、機能を切り出しましょう。

コメントをなんだか詳しく描きたい！！って場合は手を止めよう

コメントをやたら詳しく書きたい症候群が希に良く発生します。
これは自分の書いたコードの内容を他の担当者が読んでも理解できないだろうなぁという心理状態が表に出ているためだと分析します（私がそうです）。

それをそのままにしておくなんてとんでもない！！

是非機能ごとにメソッドに切り分けられないか検討してみましょう。

デザインパターン

複雑な処理なんだから読みにくくなっても仕方がないではなく、その場合はデザインパターンを利用できないか検討してみましょう。
そこに解決策があるかもしれません。
そう。デザインパターンならね。

見積にリファクタリングの工数を入れる

学生時代の試験を思い出してください。
先生が「見直ししろよ～」って言ってませんでしたか？
どんなに成績が優秀な生徒でもミスはします。
それを防ぐのが見直しでしたよね。

これはコード実装でも同じです。
設計して実装した時は良かれと思っていたことも、作業が進むにつれそれが良くないモノに変わってしまっていることもあります。
酷いと書いた本人ですら「あれ？これって何してるんだっけ？」なんてコードになっていることも。

見積もりの実装工数には見直しの工数も入れてディレクターさんに伝えましょう。

「不具合なく動けばいい」は個人の趣味の範囲だけ許されることだと思います。（自分に言い聞かせる（血涙））
サービスが続く限り機能拡張やバグ修正は発生します。
そのコード、いつまでも自分が面倒見ると言い切れますか？

見直し大事。

テストコードを書こう

最初から読みやすいコードを書ける方であればこういった作業はいらないかも・・・いやいや、それでも試験どうよう、手直しは発生するでしょう（と信じたい）。

メソッドに処理を切り分ける。
if文の処理をメソッドにする。

と、いままで動いていた処理に手を加えると、そのためにバグが発生する可能性があります。

でもテストコードを用意しておけば安心でしょう。
テストを実行するとエラーが返ってきますから！！
そしてテストが通れば安心してデバッガーさんにデバッグをしてもらえますね。

単体テストについては弊社AdventCalendarに投稿されたこちらでも取り上げられておりますので是非こちらも参考にご覧下さい。

単体テストを自動化できるようになるまで

あと、自分の投稿もｗ
テストについて

最後に

リリース後は施策を実装しなければならないので、この取り組みはなかなか大変になってきます。
また、リリース前は読みやすかったコードも、時間を経て徐々に読みにくくなりがちです。
サービスを長く、健全に提供したいのであれば、リリースした後でもリファクタリングの時間と単体テスト実装の時間を工数に入れてスケジュールを立てるべきかと思います。
そして徐々に改善していきましょう。

顧客にシステムを納品した結果、不具合修正が必要になった場合、読みにくいコードのままだと見積もりが不正確になります（怖いので余分な工数をとっておきたいけど顧客はきっと異議を唱えるでしょう。そうなると営業担当の精神がガガ・・・）。
読みにくいコードとは実に罪深いわけです。

以上のことを噛み締め、引き続き自分の書いたコードがせめて「読みにくくないコード」になるよう仕事したいと思います。

読みにくいコード改善メモ