【きれい好きなエンジニアのリファクタリングTipsX選】こんなパターンで実践してます。

あ～お部屋をきれいにすることが好きならリファクタリングも好きになるよ / いしとさん

本職はWebエンジニアでしてきれい好きが高じてリファクタリングが大好きです。

リファクタリングをしてPull Requestを出すと「いや～こんな高度なリファクタリングは自分にはできないです。」と言われました。私自身は高度なリファクタリングという認識はありませんがそう言われるのは少しうれしいです。ということで私が普段やっているリファクタリングを思いつく限りサクッと書いてみました。

Tips1:前提として最初にテストコードを書く
Tips2:メソッドのスコープをちゃんと設定する
Tips3:Publicメソッドを呼び出し順に並び替える
Tips4:privateメソッドを呼び出し順に並び替える
Tips5:メソッド名に違和感がないかチェックする
Tips6:メソッドにdocstringで説明を書く
Tips7:定数化する
Tips8:定数を並び替える
Tips9:メソッド内で変数を修正する
Tips10:if文の条件を見直す
Tips11:メソッドの行数を極力減らしていく
だいたいこんな感じでリファクタリングしています。

Tips1:前提として最初にテストコードを書く

「テストコードをかかないとそもそもリファクタリング後の担保をとることができない。」

なぜなら、リファクタリングをしても処理が既存の動きと異なってしまったらバグの原因になるからです。「というよりも99%ぐらいでそれバグの可能性が高いです」そのため、さきにテストコードを書かないとリファクタリングの担保を取ることができません。

先にテストコードを書きましょう。
※テストコードの書き方やTipsについてはこの記事では触れません。

Tips2:メソッドのスコープをちゃんと設定する

「メソッドの責務をはっきりさせます。」

なぜなら、そのファイル内の責務をはっきりさせることで他のファイルからの参照も減らすことができますし、「Aの処理はこのファイルから、Bの処理はこのファイルから」と参照してしまうと保守運用時に処理が追いにくくて大変になります。

Privateメソッドとして定義していても、Publicとして定義していることも多々あります。そのため、「この処理はこのファイルだけの限定的なものだなぁ」と思ったらPrivateに変更します。またPrivateメソッドでも別ファイルに処理を飛ばしていたりするものも見かけたりします。この場合はPublicに戻したりします。

Publicメソッド
- 別ファイルから参照されていたりする
  - APIを実行していたりなど
Privateメソッド
- そのファイル内だけの処理
  - 合計を出したりとか
  - 特定の文字を削除してたりとか

Tips3:Publicメソッドを呼び出し順に並び替える

「Publicの呼び出される順番や処理の順番通りに並べましょう。」

なぜなら、そうすることでファイルを上から見ただけで処理のぜんたいてきな流れがわかりやすくなるからです。

現在の状態
- PublicC(3番目に呼ばれる)
- PublicA(1番目に呼ばれる)
- PublicB(2番目に呼ばれる)
リファクタリング後
- PublicA(1番目に呼ばれる)
- PublicB(2番目に呼ばれる)
- PublicC(3番目に呼ばれる)

これは本屋さんと一緒で、単行本が1～10まできれいに並んでいたら自然な感覚としてとらえることができますが「1,4,10,3,2,5,9,8,6,7」のように並んでいたら「なんか違和感がある」とか「きれいではない」という感想が思い浮かぶのではないでしょうか？

順番をきれいにしていきましょう。

Tips4:privateメソッドを呼び出し順に並び替える

「Privateメソッドも呼び出される順番や処理の順番通りに並べましょう。」

個人的には以下の並びのほうが読みやすいなぁと感じます。

Publicを最初に定義
- PublicA(1番目に呼ばれる)
- PublicB(2番目に呼ばれる)
- PublicC(3番目に呼ばれる)
現在のPrivateの状態
- PrivateB2(PublicBでつかうもの)
- PrivateC1(PublicCでつかうもの)
- PrivateC2(PublicCでつかうもの)
- PrivateA1(PublicAでつかうもの)
- PrivateA2(PublicAでつかうもの)
- PrivateB1(PublicBでつかうもの)
リファクタリング後の状態
- PrivateA1(PublicAの1番目につかうもの)
- PrivateA2(PublicAで2番目につかうもの)
- PrivateB1(PublicBで1番目につかうもの)
- PrivateB2(PublicBで2番目につかうもの)
- PrivateC1(PublicCで1番目につかうもの)
- PrivateC1(PublicCで2番目につかうもの)

以下のやりかたもあります。ただ、個人的にはPublicとPrivateが混じるのであんまり綺麗とは思わないなぁと思ってしまいます。

PublicA(1番目に呼ばれる)
- PrivateA1(PublicAの1番目につかうもの)
- PrivateA2(PublicAで2番目につかうもの)
PublicB(2番目に呼ばれる)
- PrivateB1(PublicBで1番目につかうもの)
- PrivateB2(PublicBで2番目につかうもの)
PublicC(3番目に呼ばれる)
- PrivateC1(PublicCで1番目につかうもの)
- PrivateC1(PublicCで2番目につかうもの)

Tips5:メソッド名に違和感がないかチェックする

「メソッドの命名規則のとおり、かつ、処理のとおりのお名前になっていたほうがいい。」

なぜなら、メソッド名と処理が異なっているとバグの原因になるからです。

メソッド名なのに名詞になっていたり、ドメインが変わっていくことでそもそも現在の処理とメソッド名が異なっていたりすることがあります。そのためメソッド名を修正します。このときはこ高確率でテストコードも修正が必要になります。

Tips6:メソッドにdocstringで説明を書く

「メソッドの概要を書いておくと良い。」

なぜなら、docstringを見るだけメソッドの概要をつかむことができるからです。

この処理はこんなことをしています
この画面からスクレイピングします
パラメータはこうでreturnはこんなです
エクセプションはこれが発生する可能性があります

などの記載があったほうが圧倒的にわかりやすいからです。Rubyだったらyardで綺麗に書くことができます。メソッドの説明、注意事項、Todo、Parameter、Raise、返り値などを詳細に書きます。コメントを編集するコストももちろん発生しますがそれでもコードを上からしっかり読むよりも理解するお時間を短縮できます。現実世界であれば「進研ゼミで出たやつ！」みたいな感覚でしょうかね。

Tips7:定数化する

「マジックナンバーや正規表現は定数化しましょう。」

なぜなら、その値の目的がわかりやすくなるからです。

例えばループ処理のなかで不意に「13」とかあったら「なんだこの13は！？」と思った利すると思います。コメントで目的などが書いてあれば良いのですが何もない場合はマジックナンバーになります。その処理を書いた人しか理解できません。退職していたりcommitメッセージやPRにそのことが書いていない場合はもうわからないです。

そのためマジックナンバーの根拠を調べまくり、ちゃんと定数化してあげましょう。わからない場合は仮説としてコメントを振って定数化しましょう。

正規表現などは「めっちゃ正規表現すきですぐになにが書いてあるかわかるよ！」という人もいるとは思うんですがだいたいの人は「この正規表現なんだっけ？」と思うことのほうが多数派だと思います。～REGEXみたいな感じで定数化しておいたほうがなにをやっているかわかりやすいです。

Tips8:定数を並び替える

「定数は集めて基準にそって並び替えていきましょう。」

なぜなら、わかりやすいからです。

定数は数字だったら、昇順に並び替えて宣言したほうがわかりやすいですし、正規表現も基準がなかったら定数名をアルファベット順に並び替えたりした方がわかりやすいです。なにかしらルールを決めて並び替えます。

型ごとに並び替える
- 1番目のグループには数値の定数
  - 数値なら昇順 or 降順 (だいたい昇順)
- 2番目のグループには正規表現の定数
  - 正規表現も定数のアルファベット順

String型なら定数のアルファベット順などですが、URLなどの長いものはしても良いとは思いますし、APIのベースの部分なども定数にした方が良いかもしれません。

String型の定数はあんまりしなくても良いと思っている派でして、とくにエラーメッセージなどは個人的には「定数にしなくても良いのはないかな？」と思います。「エラーメッセージの定数の値はそもそもあっているんだっけ？」みたいな感じになるからです。5行ぐらいの文字数の多いエラーメッセージであれば定数にすることもありますが、改行して定義することでやっぱり定数にする必要はないかもですね。