リファクタリングの手順
普段自分がリファクタリングをどうやっているのか簡単にまとめます。
名前を正す
リファクタリングの基本中の基本。
命名規則なども規定されていると思いますが基本的には以下のことを問いながらrenameしていく。
クラス名
お前はどういう『モノ』なのか?
オブジェクト指向を知っているものなら誰しも躓いた事があるクラスの概念。
厳密にはクラスは『モノ』そのものではない(『モノ』自体と対応するのはインスタンス)が、名前をつける上ではこの感覚でいい。
名詞で名付ける。
メソッド名
お前は『何を』するやつなのか?
処理の変更なので頻繁に仕様変更がかかるため、一番ひどい有様になっている。
動詞もしくは動詞+名詞で命名をつけるようにとあらゆる命名規則で散々言われているのに忘れてしまいがち。
ちなみに動詞(+名詞)で名前をつける理由は、大概の言語がvar_name.method_name()
の構文で呼び出すからだ。
正しく命名すれば 主語+動詞(+目的語) と一般的な英語の構文になる。
(var_name.method_name(status)
ならSVOC構文になるようだ。よく出来てるな)
一番外部から参照される回数が多い名前なので、リファクタリングには結構勇気がいる。
きちんと命名できていればコード全体が英文に近くなり読みやすくなる。
(ただしJava君、君はちょっとやり過ぎ)
変数名
お前は『何』なのか?
メンバ変数もローカル変数も環境変数も同じ。(グローバル変数?死ね)
クラス名と同じ名前をつけがちだが、わざわざ変数に突っ込んでいるということは目的があるはずである。
名詞に拘ったり、型名とか付与するより(ハンガリアン記法、テメーのことだよ)、目的を書いてあげたほうが良い。
特に引数名はDocをきちんとかけばIDEなどで表示されるので真面目に命名しておきたい。
メソッドを分割する
メソッドの名前が決めがたい場合がある。
そういう場合、メソッドは一度に多くのことをやりすぎだ。
最初はスタイリッシュなメソッドでも機能追加して行く中で丸々と太ってしまう場合もある。
分割すると言ってもデブメソッドの一部を無節操に分割したところであまり意味はない。
メソッドを幾つか種類分けしてそれに沿うように分割して命名している。
getter/setter
メンバ変数に対してget/setすることのみを行うメソッド。
基本的にはそれ以外行ってはいけない。
このメソッドはメンバ変数と命運をともにする。 そのためクラスの構造をいじらない限り滅多に変更することはない。
publicのみならずprotectedも実装すると、protectedのメンバ変数を根絶できる。
変数のスコープは小さいほど読みやすいので是非とも導入していただきたい。
一般的にはgetXXX / setXXX
という命名をする。
bool代数の場合は enableXXX / disableXXX
、全メンバを纏めて操作するなら import / export
、データを消すならclearXXX
のほうが読みやすい。
言語によっては専用の構文をサポートしている場合もある。(C#, PHPなど)
過激派?の意見
クラス内でのメンバ変数参照も全てgetter/setterを通すべきという意見がある。
実際試してみた感覚では面倒なだけで得るものが少ないし、コードが冗長になって却って読みづらい。
ファイル内を検索かければ全て洗い出せるのでそこまで神経質にカプセル化する必要はないだろう。
一応setterを実装しないことでset禁止のコントロールが可能というメリットがあるが、多くの言語は読み取り専用変数を宣言できる。
(PHPくんはいつやるのかな?)
データ操作メソッド
getter/setterより特殊なデータ操作をするメソッド。
データの操作手順を一気に切り出せば長いメソッドはだいぶ見通しが良くなる。
UnitTestでこのメソッドをきちんとテストしておけば、デバッグの際にロジックに注力できる。
キュー操作の enque / dequeue
、スタック操作の push / pop
、データを逆順にする reverce
など名前は様々。
判定関数
引数やメンバ変数情報をもとにbool代数を返す関数。
メンバ変数に変更を加えない。
超ややこしい条件判定文などに効果を発揮する。
IF文の中に&&
や||
を大量に詰め込むのなら判定ロジックだけ切り出してしまおう。
境界値条件が変わったり判定条件が変わったりした場合はこのメソッドだけいじればいい。
これと言って命名規則は無いがisXXX
とかいう名前が多い。
checkXXX
などという名前にすると「どっちがTrueだっけ・・・」と悩むことになるので避けよう。
メンバ変数を変更しない方法
C++ではメソッド宣言時にconstをつけることでメンバ変数に変更を加えないことを保証できる。
宣言と実装がバラバラであるC++ならではの仕様だ。
変換処理
データを別のデータに変換する処理。
変換にのみに注力しメンバ変数へアクセスしない。
変換元か変換先のデータ構造を変更しない限り変更することはない。
アルゴリズム的なロジックを切り話すと業務ロジックに専念できる。
クラスメソッドにするべき?
メンバ変数にアクセスしないのでメソッドである必要がない。
こういうメソッドはstatic宣言をつけてクラスメソッドにすることができる。
staticメソッドはUnitTestでテストしにくい。
またメンバ変数にアクセスしないのにそのクラスに所属する意味はあるのか?という問題が有る。
正解としては変換元のデータをオブジェクト化して、そいつのメンバにするべきだろう。
ただしクラスを作るとなると大げさな話になるので一旦切り出し元のクラスに置いておく。
業務関連メソッド
残ったコードが、本当に実装したいロジックだ。 仕様変更&機能追加の影響を一番受ける。
クラスを分割する
クラスに対して「結局お前何者やねん」とツッコミを入れたくなったら、そのクラスは仕事のしすぎだ。
メソッドとメンバの数が多くなっていないだろうか?
has-a関係の切り出し
前述のようにメソッドが正しく分割されていれば、特定のメンバ変数しかアクセスしないメソッド群が見えてくるはずだ。 そいつらを纏めてクラス化して、has-a関係にしてしまおう。
データの塊の切り出し
等価に扱われる大量のメンバ変数が並ぶようなら、そいつらは1つのデータとして扱ったほうが良い。
連想配列や構造体を使って纏めてしまおう。
構造体
全てのメンバがpublicで、コンストラクタしか持たないクラス。
例外をエラーに変える
処理の流れが追いにくい?
途中でやたらと例外を投げていないだろうか?
throw/try/catchを見かけたら誰だってうんざりする。catch句内で例外の投げ直しなんてしてたら殺意を持っていい。
例外とは節度を持ったgoto文なのだから使えば使うほど読みにくくなるのは当然だ。
(実際例外を言語仕様に含まないC言語はgoto文で例外処理を実装する)
それは本当『例外的な』処理だろうか?
例外処理はエラー処理ではない。
DB接続エラーやネットワーク接続エラー、ハードウェア故障などのプログラム以外の要因で引き起こされるものは仕方ない。
それ以外は極力エラーコードを返すべきだ。
例えばInvalid Argument例外をわざわざ投げるなら、それはシステム設計上本当にあり得ない入力に対して「ありえない、なんてことはあり得ない」の気持ちで実装すべきだ。
まとめ
とりあえずいろいろ語りましたが長くなったのでここで切ります。
コードをどんどん綺麗にしていきましょう。