今回は、リファクタリングって何？というところから解説します。
確かにWikipediaを見ても、何がしたいのかよくわからないかと思います。
簡単な変更から、複雑なものまで、順に説明していきましょう。
改善点は多岐に渡りますので、まずは手短に改善できるものから始めましょう。
- リファクタリングとは？
- リファクタリングの最終目標(ゴール)は？
- 開発はスピード重視で、共通化は二の次でも良い
- なんでもかんでもリファクタリングすれば良いというものではない
- リファクタリングのメリット・デメリット
- リファクタリング初級編
- リファクタリング中級編
- リファクタリング上級編
- 取り扱い注意なプログラミング手法は避けたほうが無難かも
リファクタリングとは？
リファクタリングについては、インターネット上でも多くの解説記事・ブログが存在しますが、現在の意味合いをざっくり説明すると、下記のとおりです。
保守をするうえで、機能改善以外で、ソースをわかりやすく修正すること。』
「ソースが汚い」という言葉はよく聞かれますが、それは開発時にはなかなかわからないものです。
(一般的に納期が短いということが大きいかもしれません。)
特に、開発環境がリッチになると、多少無理なコーディングでもなんとかなりますが、導入後にメンテンナンスのし辛さに四苦八苦すると思います。
ただ、闇雲に共通化を叫んでも、共通化という生きづらさが待っているだけです。
昨今の言語の進化の力も借りつつ、わかりやすく表現することが大切です。
もし、過去のソースを見て、穴があったら入りたい！ような感覚を生じてしまった場合、それはすなわちリファクタリングのチャンスです！
頑張ってトライしてみましょう！
リファクタリングの最終目標(ゴール)は？
理想的には、「DRY」のような二重定義の排除や、「OAOO」といった共通箇所の部品化が最終目標だと思います。
が、まずは、ソースのみやすさからトライしていきましょう。
開発はスピード重視で、共通化は二の次でも良い
ご使用の言語・フレームワークによって、かなりの差が出ますが、共通化が自然にできるようなフレームワークでもない限り、最初からベテランプログラマが共通化！共通化！と叫んでしまうと、若手プログラマーの手が止まってしまいます。
まずは、ベタ書きでもいいので、個性が出てもいいので、スピード重視で不具合の無いシステムを構築しましょう。
なんでもかんでもリファクタリングすれば良いというものではない
特にJavascriptに多いのですが、Web上に転がっているコーディング例を、ほぼそのまま拝借するようなシチュエーションは多々あると思います。
gemやpipではなく、ブログのソースをコピペしたようなシチュエーションでしょうか。
その際、ソースの見栄えがちょっと気に入らないな…と手を入れたくなると思いますが、ソースを変えてしまうと、後日、そのサイトに、最新版のソースがアップされていたり、バグ修正がなされていた場合に、手がつけられないこともあるかと思います。
ですので、借用したソースは触らないほうが無難です。
リファクタリングのメリット・デメリット
メリット・デメリットをざっくりと取り上げます。
リファクタリングのメリット
- ソースが見やすくなる
- 保守がしやすくなる
- 最新の記法により、処理が簡潔になる
- 他の言語に移植しやすくなる
リファクタリングのデメリット
- 余分なテストを実施するため、開発者からは面倒くさがれる傾向がある
- 「障害があった箇所のみ修正」といった監査主義的な運用にはなじまない
IT監査バリバリのシステムの場合、不具合・仕様変更ではない修正がやりにくいことがあります。
この場合は、何らかの根回しが必要でしょうね。
リファクタリング初級編
まずは、お手軽リファクタリングです。
リファクタリングマニア的には「これってリファクタリング以前の問題だろ！」と思うものも掲載しています。
過去のコメント化されたロジックを除去
まずは、過去にコメント化したロジックで、復活の予定がないもの(「消さないでください」と明示していないもの)については、行削除を行いましょう。
改修が完了するまではとりあえず取っておこうと考えていたり、A案B案のソースを作成し、どちらかが採用された後に、没になったロジックがそのまま残っているというケースもあります。
(コメントされずに残っているケースもある。)
gitやSVNで管理する場合、以前のソースは消しても差し支えないと思います。
タブによるインデントを半角スペースに変更し、インデントを揃える
タブによるインデントは、テキストエディタによっては見た目の大きな差になり、メンテナンスのし辛さを感じることがあります。
また、ブラウザで見る場合も、アプリによっては予想外の状況になることがあります。
今どきの若いプログラマはタブによるインデントをしないとは思いますが、ベテランプログラマによっては、タブが癖になっている人もいます。
私が経験したシステムでは、半角2文字分のスペースを一単位とするインデントを行っているところが多かったです。
エディタによっては、if文やloop系の制御文を閉じたり開いたりできますが、これはインデントが狂っていると実現できないケースが多々あります。
インデントもきれいにしておきましょう。
複雑なロジックはリファクタリング前にコメントを書く
もし、メンテナンス中に複雑(怪奇)な処理に出くわし、直ちにリファクタリングが困難な場合は、少なくともコメントで、どんなことをやっているか、後回しする理由や改善ポイントを記載しておくと、今後に役立ちます。
似たようなロジックや、改善したほうが良さそうなロジックにはマーカーを付ける
似たようなロジックを見つけるたびに、マーカー的なコメントを付けておくのも、後々役に立ちます。
定数的な変数は英大文字に統一し、初期処理としてまとめる
これは言語によって異なりますが、もし、大文字小文字を判断しない言語がであった場合は、(値を変えることのない)定数的な変数は(他の言語に見倣い)英大文字で統一されることをおすすめします。
マジックナンバーの除去
前項の「定数的な変数」の具体例です。
フラグ・区分的な項目で分岐することは多々あると思います。
その際に、実際の値でコーディングしていませんか？
(これを「マジックナンバー」と呼びます。)
可能であれば、前項の定数的な変数で代用すべきです。
例(変更前)：
if (strStatus == '1') { 処理１ } else if (strStatus == '2') { 処理２ }
リファクタリング後：
if (strStatus == ini.KOJIN) { 処理１ } if (strStatus == ini.HOUJIN) { 処理２ }
ini.～は、iniファイル等から取得した定数値だと思ってください。
こういった初期値については、テキストで管理する場合と、DBにコントロールテーブルを用意し管理するパターンがありますので、そのシステムの仕様・ポリシーに従ってください。
以下のメリットがあります。
- ’1’が個人(事業主)、’2’が法人であることがわかる
- 将来的にこの項目が2桁になった場合も、容易に対応できる
ハードコーディングの除去
SQLのDBに入っている名称を取得し、表示しないといけないのに、ソース上に直接名称を書いていませんか？
こういった項目が要注意です。
例えば性別欄が代表格でしょうか。
1文字変数を避ける
ブログでの例題はせいぜい数行～十数行レベルのソースですが、実態は相当長くなります。
また、ローカルとグローバルが曖昧な言語もあります。
1文字変数は、そういった環境で不具合を誘発するケースが多々あります。
また、1文字変数は、貧弱なテキストエディタで検索が困難なため、避けたほうが無難です。
(インストール先に満足なエディタが存在しないケースは多々あります。)
2文字以上、あるいは可能な限り3文字以上としてください。
1文字変数の具体例としては、主に配列の添字(i/j/k/x/y/z)や、例外処理の引数(e)でしょうか。
下記は、添字のxをidxに、yをidyにした例です。
例外のeはex等にしておけば、検索に引っかかりやすくなります。
idy = 0; arrBar = []; for (idx = 1; idx <= arrFoo.len(); idx++) { if (arrFoo[idx] > 0) { idy += idy; arrBar[idy] = arrFoo[idx]; } }
複合演算子を利用する
集計や文字連結の場合、左辺と右辺に同じ変数を記述することになりますが、変数名が長いと横スクロールが生じるなど、かなりウザく感じるかもしれません。
また、コピペミスによる障害を誘発しやすくなります。
複合演算子でそれらを解決しましょう。
例(変更前)：
numTotalAmountQ3 = numTotalAmountQ2 + numAmount;
上記例は、第3四半期の合計に加算するはずが、第2四半期の合計に加算しています。
こういった不具合は多々ありますよね。
リファクタリング後：
numTotalAmountQ3 += numAmount;
これで解決できました。
また、文字の連結にも使用可能です。
strName &= "様";
さらに、昨今のトレンドではインクリメント演算子を嫌う(人が多い)傾向もあるようで、その場合はこのように記述しましょう。
numCount += 1;
リファクタリング中級編
より実践的なリファクタリングのテクニックになります。
多重カッコはメソッドチェーン(メンバー関数)ですっきりさせる
メソッドチェーンを使用することで、多重カッコの煩わしさから解放されます。
言語の中には、従来の関数以外に、メソッドチェーン用の関数を用意しているものがあります。
その場合は、積極的に活用し、多重カッコのわずらわしさから開放してあげましょう。
例(変更前)：
strHoge = ucase(trim(replace(strHoge,"¥n","","all")));
カッコの内側から外側へ処理されます。
リファクタリング後：
strHoge = strHoge.replace("¥n","","all").trim().ucase();
左から右へ処理されます。
また、引数も1つずつ減ります。
メソッドチェーンのデメリットは、その変数の型が不定の場合、その型に対応する同メソッドがないと、異常終了してしまうリスクがあります。
その場合は(メソッドチェーンではない)一般的な記法とすると、型変換が自動的に行われます。
anyHoge = 1 / 5; // 実数はそのままtrim()を実行するとエラーとなる strHoge = trim(anyHoge); // メソッドチェーンではないtrimは型変換してくれる
メソッドチェーンは不具合を誘発しやすいのでは？避けるべき？…もし、その変数に違う型が代入される可能性がほとんどない場合、メソッドチェーンを利用することで、例外処理で異常データを捕捉しやすくなる…という発想の転換もできます。
例外処理を避けつつ、あいまいな変数のままメソッドチェーンを利用したい場合は、間にtoString()やto_sといった型変換の関数を挿入すると上手くいきます。
anyHoge = 1 / 5; // 実数にtrim()をメソッドチェーン化するとエラーとなる strHoge = anyHoge.toString().trim(); // toString()で文字列型に変換する
添字は極力避ける
不具合で比較的目立つのは、配列の添字エラーかと思います。
「1文字変数を避ける」の例、
idy = 0; arrBar = []; for (idx = 1; idx <= arrFoo.len(); idx++) { if (arrFoo[idx] > 0) { idy += idy; arrBar[idy] = arrFoo[idx]; } }
は、下記のようにできます。
arrBar = []; for(item in arrFoo) { if (item > 0) { arrBar.append(item); } }
さらに、言語によっては、
arrBar = arrFoo.filter(function(item, idx) { // idxは省略可能 return (item > 0); });
とすることも可能でしょう。
無用なネストは避ける
submitやAPIの更新処理で下記のような処理があるとします。
例(変更前)：
// エラーチェック群 if (stuJson.error == 0) { // ノーエラーの場合 更新処理群 (数百行程度続く) } // スクロールを繰り返していくと、この「}」は一体何の「}」なのかわからなくなる // 終了処理 return serializeJson(stuJson);
リファクタリング後：
// エラーチェック群 // エラーの場合はここで終了 if (stuJson.error != 0) { return stuJson.to_json; } 更新処理群 // 終了処理 return serializeJson(stuJson);
この手法は「早期リターン」と言い、リファクタリングの代表的な手法のひとつです。
もし、
function hoge() { 条件1前処理 if (条件1) { 条件2前処理 if (条件2) { 条件3前処理 if (条件3) { 条件4前処理 if (条件4) { 条件5前処理 if (条件5) { 条件一致処理 } } } } } } // function
このような処理があり、各条件がand or等で複雑だった場合、
function hoge() { 条件1前処理 if (!(条件1)) return; 条件2前処理 if (!(条件2)) return; 条件3前処理 if (!(条件3)) return; 条件4前処理 if (!(条件4)) return; 条件5前処理 if (!(条件5)) return; 条件一致処理 } // function
といったように、not状態に反転させてreturnさせても十分に見やすくなります。
言語によってはif notに相当するunlessという制御文が存在します。
もしRubyを使用しているのなら、if文やunlessを後ろに書いて、returnを強調させましょう。
def hoge() 条件1前処理 return unless 条件1 条件2前処理 return unless 条件2 ～中略～ 条件一致処理 end
無用なネストは避ける(ループ編)
ループの場合は下記のような見直しをします。
変更前：
for (row in qryHoge) { if (条件1) { 条件1の処理; } else { if (条件2) { 条件2の処理; } else { 条件1でも2でもない処理; } }
リファクタリング後：
for (row in qryHoge) { if (条件1) { 条件1の処理; continue; // 言語によってはnext } if (条件2) { 条件2の処理; continue; // 言語によってはnext } 条件1でも2でもない処理; }
さらに、具体例を提示します。
変更前：取得した配列(群)の先頭10行を画面に表示する(但し空白行は除く)
idx = 0; numCount = 0; for (item in arrHoge) { idx += 1; if (item != "") { numCount += 1; if (numCount <= 10) { 処理群１ if (arrOption[idx] == "1") { 処理群２ } } } }
リファクタリング後：
idx = 0; numCount = 0; for (item in arrHoge) { idx += 1; // 未入力行は次の行へ if(item == "") { continue; } // 10件を超える場合は画面表示しない numCount += 1; if(numCount > 10) { break; } // 画面表示 処理群１ // オプション表示をしない場合は次の行へ if (arrOption[idx] != "1") { // == "0"とした方が良いのかは別途判断する continue; } // オプション表示 処理群２ }
リファクタリング前ではbreakすべき条件とcontinueすべき条件が一緒くたになっておりましたので、分離しています。
リファクタリング上級編
パイプライン処理のようなコーディングを心掛ける
難しい説明はさておき、具体例を説明します。
前項の最後の例をさらにリファクタリングしてみます。
// 10件に集約 arrDisplay = []; arrHoge.each(function(item, idx) { var stuWork = {}; if (arrDisplay.len() > 10) continue; // 10件を超えたら処理しない // 空白でない場合、構造体化した配列にセット if(item != "") { stuWork.hoge = item; stuWork.option = arrOption[idx]; // (他にも構造化したい項目があれば加える) // 構造体ごと配列に追加する arrDisplay.append(stuWork); } }); // 結果を表示する arrDisplay.each(function(item, idx) { // 画面表示 処理群１ // オプション表示 if (item.option == "1") { 処理群２ } });
配列がバラバラになっていることに着目し、最初のループでは構造化された配列にまとめ、2度目ループでは表示ロジックに集中できるようにしています。
このように段階的にシンプルにループを繰り返す手法です。
ループ件数があまりにも大きい場合は厳しい手法ですが、手ごろな大きさであれば、シンプルなコーディングが可能です。
これはいわゆるパイプライン処理の理念を拝借したものです。
リアルなパイプライン処理にしてしまうと、ソースが逆に複雑化するかもしれませんので、ほどほどのコーディングにとどめます。
パイプライン処理的な発想で見直していくと、共通部品化が見えてきます。
長い処理や、共通する箇所はファンクション・部品化する
もし、前項のarrDisplay.each()以降の処理が他のプログラムにも存在し、共通化出来そうであれば、ファンクション化あるいはモジュール・クラス化するきっかけにもなるでしょう。
arrHoge.each()も長いのであれば、ファンクション化しましょう。
// 最新10件に集約 arrDisplay = selectData(arrLine=arrHoge, arrOption=arrOption); // 結果を表示する displayData(arrDisplay=arrDisplay);
参考までに、このように記述することも可能です。
// 最新10件に集約し、結果を表示する displayData(arrDisplay=selectData(arrLine=arrHoge, arrOption=arrOption));
ただ、カッコがネストしてしまうのと、引数は渡されてしまうので、後述する「何度も登場する引数は、インスタンス変数で渡す」を参考に、クラス化し、インスタンス変数に保持したほうが良いかもしれません。
その場合は、こうなります。
hoge = Class.new("Hoge"); // 10件に集約 // 集約したデータはhoge.arrDisplayにセットされる hoge.selectData(arrLine=arrHoge, arrOption=arrOption); // 結果を表示する hoge.displayData();
スイッチ変数は避ける
例を挙げて説明します。
function hoge(param) { strSw = 0; // スイッチ変数 条件1のための前処理 (前処理は結構な行数) if (条件1) { strSw = 1; } 条件2のための前処理 if (条件2) { if (条件2A) { strSw = 2; } else { strSw = 1; } } 条件3のための前処理 if (条件3) { strSw = 3; } 条件4のための前処理 if (条件4) { strSw = 0; } // 処理 // strSw=0は処理しない if (strSw == 1) { 処理群1 } else if (strSw == 2) { 処理群2 } else if (strSw == 3) { 処理群1 処理群3 } return; } // function
このスイッチ変数、大昔に大流行した手法なのですが、実は最近、私が経験したシステムにもありました。
スイッチ変数は時と場合により、せっかく条件付けたのに、ご破算になってしまうケースが多々あり、非常にメンテナンスがし難いプログラムが出来上がってしまいます。
各条件は排他的ではないため、例えば、条件4や条件3は条件1に勝ちます。
「早期リターン」の考え方を応用し、これをリファクタリングします。
function hoge(param) { // 条件4に合致する場合は処理終了 strReturn04 = checkFunction04(param); // 条件4のための前処理をファンクション化 if (strReturn04) return; // 条件3に合致する場合 strReturn03 = checkFunction03(param); // 条件3のための前処理をファンクション化 if (strReturn03) { doFunction01(param); // 処理群1をファンクション化 doFunction03(param); // 処理群3をファンクション化 return; } // 条件2Aに合致する場合 strReturn02 = checkFunction02(param); // 条件2のための前処理をファンクション化 strReturn02A = checkFunction02A(param); // 条件2Aのための前処理をファンクション化 if (strReturn02 && strReturn02A) { doFunction02(param); // 処理群2をファンクション化 return; } // 条件1または条件2に合致する場合 strReturn01 = checkFunction01(param); // 条件1のための前処理をファンクション化 if (strReturn01 || strReturn02) { doFunction01(param); // 処理群1をファンクション化 return; } // それ以外も処理終了 return; } // function
この例では、条件4が最優先のため、先に処理しています。
「早期リターン」は例外を先に記述し、いらないものはさっさと除外し、必要なもののみを処理する考え方です。
もし、条件2のための前処理と条件2Aのための前処理に依存性がない場合、
～省略～ // 条件2Aに合致する場合 strReturn02A = checkFunction02A(param); // 条件2のための前処理をファンクション化 if (strReturn02A) { doFunction02(param); // 処理群2をファンクション化 return; } // 条件1または条件2に合致する場合 strReturn01 = checkFunction01(param); // 条件1のための前処理をファンクション化 strReturn02 = checkFunction02(param); // 条件2のための前処理をファンクション化 if (strReturn01 || strReturn02) { doFunction01(param); // 処理群1をファンクション化 return; } ～省略～
とした方が良いかもしれません。
スイッチ変数絡みは難易度が高いのですが、リファクタリングの効果は大きいです。
何度も登場する引数は、インスタンス変数で渡す
下記の場合はクラウドDBサービスに、レコードが存在する場合は更新、なければ追加するといったシチュエーションです。
アクセスする際には、アクセストークンを取得する必要があります。
form.～はフォームパラメータを示します。
改善前
hoge_api = Class.new("Hoge"); // HOGE APIのトークンを取得 strToken = hoge_api.getToken(); // HOGE APIにデータが存在するか確認 strRtn = hoge_api.checkData(strToken=strToken, strKey=form.key); // キー存在チェック(ブランクの時は何もしない) if (form.key != "" && strRtn == "1") { // "1"はレコードが存在する // 存在する場合データ更新 hoge_api.updateData(strToken=strToken, strKey=form.key, strData=form.data); } else { // キー未設定、またはHOGE側に見つからない場合、データ追加 // (HOGE APIのユニークキーは再設定) form.key = hoge_api.insertData(strToken=strToken, strData=form.data); writeOutput("ユニークキー" & form.key & "が(再)発行されました！"); }
といったシチュエーションで、何度も同じパラメータを渡す場合、インスタンス変数をクラス側に定義します。
改善後(クラス側)：
Class Hoge { // インスタンス変数定義 this.strToken = ""; // アクセストークン this.strkey = ""; // APIのユニークキー(追加時は自動生成) ～以下略～ }
改善後(利用側)：
hoge_api = Class.new("Hoge"); // HOGE APIのトークンを取得 hoge_api.getToken(); // 取得したアクセストークンはインスタンス変数に保持 // HOGE APIにデータが存在するか確認 hoge_api.strkey = form.key; // 更新時のみキーセット strRtn = hoge_api.checkData(); // キー存在チェック(ブランクの時は何もしない) if (hoge_api.strkey != "" && strRtn == "1") { // "1"はレコードが存在する // 存在する場合データ更新 hoge_api.updateData(strData=form.data); } else { // キー未設定、またはHOGE側に見つからない場合、データ追加 // 生成されたキーはhoge_api.strkeyにセットされる hoge_api.insertData(strData=form.data); writeOutput("ユニークキー" & hoge_api.strkey & "が(再)発行されました！"); }
例題ではアクセストークンと、DBのユニークキーを、インスタンス変数として、何度も受け渡しをすることを避けています。<
メモリを食うこともあるので、賛否はあるかもしれませんが、状況によってはform.dataもインスタンス変数化すれば良いでしょう。
エルビス演算子やor演算子による初期化
左辺が未定義の場合、?:以降のデフォルト値が設定されます。
例(変更前)：
if (!isDefined("strType")) { strType = "0" }
リファクタリング後：
strType = strType ?: "0";
惜しいのは複合演算子化していない言語があることです。
つまり…
strType ?:= "0";
とするとエラーになる言語があります。
注意したいところです。
参考までに、他の言語ではor(||)演算子がその役割を担うことも多々あります。
(undefinedがエラーとはならずfalseと同義となる言語では、||演算子がエルビス演算子と同じ役割を持つことができる。)
特にgetパラメータのデフォルト値セットでよく見かけるイディオム(慣用句)です。
strType ||= "0"
取り扱い注意なプログラミング手法は避けたほうが無難かも
以降は、プログラミングテクニックではありますが、取り扱い注意案件です。
第一級オブジェクトは、逆に見づらくなる？
ファンクション内でifやcase等で分岐するのではなく、ファンクション名を引数で渡すことで、任意のファンクションを内部で動作させるなど、データの他に固有のファンクション名(やロジック自体)をファンクションの引数で渡す手法らしいのですが、そこまで凝る必要はないと思います。
幸い、私は、そこまでやるプログラマーを見たことはありませんが、見つけた場合は、これってどうなの？と、議題に上げたほうが良いでしょうね。
モンキーパッチは避ける？
Rubyですと、例えば配列や文字列型にオリジナルのメソッドを取り付けたくなる衝動に駆られるかもしれません。
しかし、その動作は将来的に保証されません。
もし、そういったロジックを見つけた場合は、そもそもそれってOKなの？かを、議題に上げたほうが良いでしょうね。
