リファクタリングで何を優先すべきか（うれしかったかのは何か）

　引継いだコードのリファクタリングを行ってきた。その中で優先すべき作業と優先すべき必要性が乏しかったと感じた作業がある。その経験に基づいて個人的な見解を述べたいと思う。

　リファクタリングで、使うべきインターフェースと使う分に意識しなくていい関数とが明確になった、機能のテストがしやすくなったなどの効果をもたらしたが、一方でリファクタリングで必要になった工数も無視できない。そこで、いまさらながら、優先する必要性が乏しい作業、効果が大きいと思ったリファクタリング作業について示す。

＜優先する必要が乏しい作業＞

データ構造の置き換え

　使用しているライブラリでの推奨のデータ構造が変更になっているからといって、使用している箇所が多い場合には、その作業の優先順位は上げないほうが無難である。
　いたずらに、最新のデータ構造にあわせるための作業に時間を使ってしまい、本来の実装がその分先送りになってしまう。
　同様にGUIツールの置き換えも、使用している箇所が多いことが予想されるので、急ぐべき理由がない限り、後回しにするのが無難そうである。
　

マクロ定数をconstへの置き換え

　マクロ定数は、ソースコード上で文字列の置換を行うので、ソースコード全体で共通のconstのグローバル変数を参照しているようなものである。そのため、スコープが不明確になる点などで推奨されていない。マクロ定数をconstの変数として置き換えるとスコープ狭めることができる。constの変数で配列の大きさを指定して配列を宣言することもできるなど、マクロ定数を使うべき理由はなくなってきている。
　しかしながら、多数のマクロ定数のconstへの書き換えは、それなりに作業量が大きくなる。
マクロ定数を定義している#defineのヘッダファイルを分割して、それぞれを本当に必要な範囲でだけincludeするようにすれば、(事実上の）スコープは狭くなる。そうすれば、マクロ定数のもつ望ましくない点は減らすことができる。マクロ定数による大きさとする配列の宣言をするのはよくあり、これを可変の変数として置き換えるのは、急ぐ度合いが少なければ後回しにするのが得策である。
マクロ定数をヘッダファイルにしなければならないとは、必ずしも同意しかねる。マクロ定数が、そのモジュールファイルの中でしか使わないとすれば、そのモジュールのスコープのグローバル変数としてみなすことができる。そのマクロ定数を参照する部分が、他のモジュールファイルにあって、変数としてgetなんとか関数()で値を参照できれば十分な場合には、マクロ定数自体を直接外部にincludeしてもらう必要はなくせる。

　注：一連のマクロ定数をenum型に改変するのはこの限りではない。enum型にすることは、一連の値が同一のカテゴリーに属する異なる値だということを明確にする。

追記：設定ファイルで可変にしたい場合
　#defineで固定値を与えているけれども、本来は利用者の環境によって変えたい項目の場合、std::map型を使って見通しのよい設計をしよう に書いたように、map型を使って設定ファイルを読み書きする方法がある。利用環境ごとに再コンパイルしなくて済むのがよい。

関数のインタフェースの変更

void func(const IplImage *imgIn, IplImage* *imgOut);
を
IplImage *func(const IplImage *imgIn);
とするような書き換えは、意味を明快にするという意味で効果的と私は思うが、該当する行数が多いときには、必ずしも優先させる必要は少ないと感じた。十分にconst属性をつけたり,
@param[in], @param[out]を十分に書いてあって、理解しやすいのであれば、無理に書き換える必要はない。

＜効果が大きいと思ったリファクタリング作業＞

意味のまとまりを明確にすること

そのために名前空間やクラスを導入すること。
　意味のまとまりが、namespaceやclassによって、関連する処理・定数などが明確になって、変更が生じたときに影響する範囲が明確になった。

関数の引数のconst属性とドキュメンテーションコメントの改善

・関数の引数が@param[in], @param[out], @param[in,out]の区別を理解するにいたったし、const属性が付けられる引数にはconst属性をつけることで、プログラムの保守性が向上した。

物理量には単位を付ける

　あなたにとって常識の単位が、引継ぐ人には常識ではない。インチなのかcmなのかmmなのか、他の単位なのか。必ず明らかにしなければ安心して使えない。

・（これから書く関数を中心に）ポインタ渡しを参照渡しに変える。

　ポインタ渡しにともなって、NULLである可能性を考える必要がなくなる分だけ、呼び出し側・呼ばれる側の記述が簡単になります。
　既存のコードでポインタ渡しを参照渡しに変える改変も、それほど手間がかからないので、急ぐ必要もないし、先送りすべき理由ともないと感じた。

単体テストを容易にする関数の抽出

　長すぎたり短くても意味が込み入っている関数から、関数の抽出をしてメンテナンスしやすいようにすることは、明らかに意味がある。

・関数の抽出をすることで、コードの意味が明確になったこと。
　その関数の働きを１つの文で簡潔に言えるようになっていることが望ましい。
・関数の抽出をすることで、テストが容易になったこと。
　いままでの関数のインターフェースでは不十分で、関数の抽出をする必要があった。それが関数の抽出によって利用可能になった。
　

関数の構造で見えるべき変数と隠れてよい変数

・関数の引数は、関数内部で十分に見えるようにすること。
　もし、その内部で関数の引数は使っていない、しかし上部の階層の都合上引数が存在するような場合には、そのことを明示しつつ、関数のテストのしやすさを上げるために、引数なしの関数を作成して
推奨しないコーディング

void funcA(var a){
//とても長い関数
//仮引数 aが使われているかどうかは、すぐには判別できないほど長い。
}

推奨するコーディング

void funcA(var a){
//API設計上、仮引数aが必要になっている。
//仮引数は、この関数の階層の中で目立つ変数であるべき。
  funcB();
}

void funcB(){
//とても長い関数
//funcAの仮引数は使われていない。
}

に置き換えるのがよいのではないかと考える。
　つまり、見えるべき変数には、仮引数、グローバル変数、戻り値となる変数、ロジックの決め手となる制御変数などがあるのではないかと考える。
　逆に、見えなくてよい変数があって、それらが適度に隠れるようにした方が、理解がしやすくなる。見えなくてよい変数は、一時的な作業用の変数、if文、for文のブロックの中にだけスコープを持つ変数（あるいはもつべき変数）などがある。

テストできていない書き換えは行いすぎない。

　プログラムの断片を書き変えているときには、それによって、バグを生み出している可能性は高くなる。
　

＜手をつけたくないが、ある段階で取り組む価値のある作業＞

静的な領域確保から動的な領域確保への書き換え　

・いままで、マクロ定数として実装されていた部分が、可変な値として扱う枠組みに変えるにはリファクタリングは必須だった。マクロ定数でdouble data[DATA_SIZE]などのように固定値で配列を確保している状況では、設定ファイルからデータを読み込むといったことに制限が生じる。
　データの数が、設定ファイルから読みとってから判明するという場合、動的に領域確保し、動的に値を設定する必要を生じる。そのためには、書き換えが必要になる。それなりの工数になることもあるので、本当に必要かどうか考えてから着手すべきである。「できたほうがいいよね」程度ならば、手をつけないのがよいと思う。「静的な領域確保から動的な領域確保への書き換え」は、メモリの動的な確保と解放というオーバーヘッドを生じるので、本当に必要でない限り、行わないほうがよさそうだ。

・意味の明快なデータ構造を使う。

　点の座標データを示すために、intの変数を２つ使うのではなくて、cv::Pointを使う(OpenCVの場合)。そうすることで、人が意味を読み違える可能性が減ります。特に引数にｘとyの両方を指定しなければならないときに、i,jという引数が使われていたら、意味をとりにくくなります。
　3次元の点であって、プログラムの中では、８つの点がそれぞれ配列ではなしに定義してあるときに、3x8で24個の変数が出てくるよりは、8個の変数で済んでくれる方が読みやすい。
　

現在動いていて、近い将来の範囲でそのモジュールの拡張を予定していない、枯れていると信じられているモジュール

　優先順位が高い作業があるはずです。それらが片付いてから考えましょう。

追記：#ifdefがない幸せ
　ソースコード中の#ifdefを削除して、条件付きコンパイルを減らしたことはメンテナンス性を向上させて、うれしい度合いが高い項目でした。#defineで設定していた部分をソースコード内の変数として管理しなおしたことで、再コンパイルなしに設定が変えることができるようになりました。このことで、従来は簡単に試せなかった組み合わせで、プログラムの動作確認ができるようになりました。従来は#ifdefで無効化されている部分に影響のある部分を改変しても、コンパイル時に無効化されているのでコンパイルエラーを生じないので見逃しやすかったのが、if(ブール変数)に置き換わっているので、コンパイル時に必ずチェックされるようになります。

追記：単一責任原則の原理
　「いいかげんにしてくれ！」と叫びたくなるコードの特徴の１つは、単一責任原則をこれっぽっちも考えたことがないような記述です。１つの関数は１つのことを単純に行い、その１つのことを行う関数は、ただ１つあって、あっちこちにばらついていて欲しくないということです。
　１つの関数は、１つのことを単純明快に行うべきであって、余分な操作を加えてはならないのです。その余分な操作を別の関数にすれば、関数がはるかに理解しやすくなる場合には、別な関数にしましょう。
　また、１つのcppファイルから、「１つの関数を除外すれば単体テストがしやすくなる」とか「１つの関数を除外すればヘッダファイルのincludeが減る」場合には、その関数は、そのcppファイルからファイルにしたほうがいいように思えます。

追記：リファクタリングをすることで、第三者に引継いでもらうことが可能になる。
　リファクタリングをしてうれしいことの最大のものは、コードのメンテナンス・改良を引継いでもらえたことです。名前空間を導入して、どの機能はどのソースコードファイルが責任をもっているのか、役割分担が明確になるようにリファクタリングしたので、引継いだ人が、一度に注意を向けるべきソースコードの範囲が明確になったことが大きいと思っています。
　仕様書とソースコード中のコメントと実装のコードとがどれも食い違っているときに、どうソースコードを修正していくべきなのかが分からなくなってしまいます。そうならないように、ソースコード中のドキュメンテーションコメントとソースコードの実装とを一致させるようにメンテナンスを続けることです。そのようにして、引継いでもらうことが可能なレベルになっていけば、手離れをしていって、自分が身軽になっていきます。リファクタリングして、メンテナンスしやすいコードになれば、人員の増員しても分業がしやすくなります。
　

追記:アルゴリズムの置き換えによる大幅改変への準備ができたこと
性能の上の問題で、アルゴリズムの置き換えをしたのがあります。その際に事前にリファクタリングをして、大幅改変の準備をしました。そのときの経験は、以下の場所に書きました。
引き継いだソースコードを大幅改変する

　

追記：メモリ管理が楽なデータ構造とライブラリへの書き換え
ソースコードの書き換えの中でうれしいことは、メモリ管理が楽なデータ構造とライブラリへの書き換えでした。ポインタを使ったり、malloc()、free()を使う必要があるデータ構造よりは、ポインタを使わずに済み、メモリの確保や解放を明示的に書かなくてもすむライブラリの方が保守性が向上します。IplImage型からcv::Mat型に書き換えることは、そのような利点をもたらしてくれました。