はじめに

「先輩からリファクタリングしろって言われたけどどこを直したらいいか分からない...」
とか
「自分の書いたコード全てがクソコードに見える...」

といった悩みを、誰しも一度は感じたことがあると思います。
自分もその1人で、1年間エンジニアとして働く中で、たくさんのコード修正と格闘してきました。
今回は、先輩から聞いたり、本で勉強したりして、自分なりに身につけた「こういうコードはやばい！」や「このタイプのコードはこう直せ！」といった感覚をまとめてみました。

誰かがリファクタリングする際に少しでも参考になると嬉しいです。拙い記事ですがよろしくお願いします。
1記事にまとめて書くと長過ぎるので、前編・後編に分けます。

前編では

破滅的な命名
汎用的すぎる命名
複雑な条件分岐
長すぎるメソッド

後編では

再代入を繰り返す変数
重複したコード
統一化されてないコード
不要なコメント

について説明します。

注意事項

以下のソースコードは全てRubyで記述しています。
また今回のリファクタリングが最善とは限りません。あくまでも参考として留めていただければ嬉しいです。

リファクタリングをする前に、まずはテストコードを書こう

「え、リファクタリングの話するって言ってたのにいきなりテストの話？」

と思った方がいるかもしれませんが、リファクタリングにおいてテストは非常に重要です。
そもそもリファクタリングとは

ソフトウェアの外部の振る舞いを保ったままで、内部の構造を改善していく作業(*1)

です。

特に『ソフトウェアの外部の振る舞いを保ったままで』の部分が非常に重要で、
「リファクタリング頑張ったおかげで、コードが読みやすくなったし、拡張性も上がったけど、仕様漏れやバグが出ちゃってるなあ」
となってはなんの意味もありません。

その行動はリファクタリングではなく、ただコードを破壊しているだけです。

自分のリファクタリングでコードが壊れていないかを確認するために、仕様を充分にみたしているか確認してくれるテストコードが必要不可欠です。

1箇所リファクタリングしたら、テストコードでバグが起きていないか確認しましょう。
この作業をサボると、後で泣くハメになります。

リファクタリングは価値のあるツールですが、単体で成り立つものではありません。なんらかの誤りは必ずあり、それらを指摘してくれる堅牢なテストツールが、適切なリファクタリングには必要です。(*1)

(*1)リファクタリング既存のコードを安全に改善する（第2版）から引用

リファクタリングの必要性を感じるコード

以下から、怪しい香りのするコードについて具体的に解説していきます。

破滅的な命名

class Class1
  def method1
    # 何らかの処理
  end

  def method2
     # 何らかの処理
  end

  def method3
     # 何らかの処理
  end
end

最悪です。読む気が完全に失せるコードと言っても過言ではありません。

流石にここまで全ての命名に気を配らない人は見たことないですが、一部コードに似たような命名をしてしまったことがある人もいるのではないでしょうか。
自分も過去にやらかしたことがあります。

上記のような連番命名は、処理の内容や意図が全く読み取れないので絶対にやめましょう。
名付けが簡単ということ以外メリットが皆無です。
何をしている処理なのか、何に使われるのか、何のためのコードなのかなどが伝わる命名が理想です。
名前だけで、処理や利用方法、目的が伝わる命名を目指しましょう。

汎用的すぎる命名

def get_data
  data = User.find(1)
  info = data.name
  number = data.age
  # 何らかの処理
end

上記の内容と少し被りますが、命名に汎用的な単語をつけるのも避けた方が良いです。
汎用的な単語からは読み取れる情報が少ない上、誤解を招きやすいです。

できるだけ具体的な単語を使うことで、処理の目的や変数の中身が明確になります。

スコープが非常に短いローカル変数など、特殊な場合を除いて汎用的な命名は避けしょう。

複雑すぎる条件分岐

if (条件A)
  # 
  # なんらかの長い処理
  #
  if (条件B)
    # 
    # なんらかの長い処理
    #
    if (条件c)
      # 
      # なんらかの長い処理
      #
    else
      # 
      # なんらかの長い処理
      #
    end
  end
end

読む気が完全に失せるコードpart2です。
条件分岐のネストを深くすることや、各ブロックの中で長い処理を書くことはコードの読みやすさを大きく損ないます。

また、条件分岐のロジック理解も困難になるため、コード修正に無駄に時間がかかったり、変更漏れや予期せぬバグを生むことにもつながります。

このような場合はガード節を利用し、早めに処理を抜けることでネストを浅くすることができます。(*2)
それによりコードの可読性が上がるだけでなく、条件分岐への理解が簡単になり、修正しやすくなります。

(*2)ガード節については以下の記事が非常にわかりやすいのでおすすめです。

長すぎるメソッド

def long_method
  # 処理を説明するコメント1
  # ~
  # 処理数十行
  # ~
  #
  # 処理を説明するコメント2
  #
  # ~
  # 処理数十行
  # ~
  #
  # 処理を説明するコメント3
  #
  # ~
  # 処理数十行
  # ~
end

ダラダラとロジックがベタ書きされたメソッドは、コードの可読性を大きく損ないます。
またメソッドが長くなると、どこからどこまでがどんな処理をしているかを把握しづらくなります。
それゆえ、修正する際に該当箇所を見つけることが困難です。

このようなコードは、意味のあるまとまりでメソッドとして切り出しましょう。

上記コードのように処理に関するコメントが必要だと感じたタイミングで、コメントする代わりにわかりやすい命名をしたメソッドに分割するとだいぶ読みやすくなります。

また、メソッドの命名を適切に行なっていれば、その目的外の処理を関数として分割すると考えても、メソッドの切り出しがやりやすいかもしれません。

終わりに

今回はリファクタリングの必要性がある怪しいコードと、それらを直す際の考え方についていくつか紹介しました。

後編ではまたいくつか別パターンの怪しいコードを紹介するので、読んでもらえたらありがたいです🙏