引き継いだコードは、メンテナンスしやすい状況になっていないことが多い。
「引き継いだソースコードを改変する前に」
http://qiita.com/nonbiri15/items/47e25c2d5fb46f3495df
で引き継いだコードの可読性を少し改善したとする。
しかし、まだまだメンテナンスしやすいものにはなっていない。
そこで、メンテナンスしやすくするためのリファクタリングについて私の現状の理解を述べようと思う。
残っている課題
・グローバル変数が残っている
対策：
グローバル変数と、そのグローバル変数を書き換える関数群を、１つのモジュールファイルの中に押し込めよう。
そのとき、そのグローバル変数に無関係な関数や変数を、そのモジュールファイルに含めないようにする。
グローバル変数を参照する側、extern で参照できるようにしておく。
そのようにファイルを分割することで、グローバル変数の実体がある側のファイルにだけ、その変数が左辺式としてでてくる。
そのことで、クラスを用いないなりのオブジェクト指向に近い状況が生まれてくる。
グローバル変数 → クラスのデータメンバー
グローバル変数を書き換える関数 → クラスのメソッド
グローバル変数を参照する関数 → クラスのメソッド
のように、近い将来書き換えることができるだろう。
部署の方針などにより、C++のコンパイラを使っていても、クラスを設計しない状況があるだろう。それでも、このような書き換えは、メンテナンスやテストを容易にするだろう。
追記：
その後の記事で書いたように、「ソースの分割は論理構造の分割」なので、
そのソースコードの中にあるものを全て名前空間を導入して、その中のものであることを明示することも可能だ。いやおうなしに、その論理構造の中で何を実現するのか(どのような入力に対して、何を出力するのか）を明示的に考えることにつながる。
さらに追記：
その特定のモジュールに対して名前空間を使うようにしていきます。そうすると、そのグローバル変数のスコープが狭くなっていきます。データを隠して、関数インタフェースだけで利用するようにすること、値の初期化をどうするのかを考えていけば、class を用いた実装に置き換えていくのは簡単です。
・グローバル変数に併発している課題
グローバル変数を多用しているプログラムの場合、長すぎる関数という問題も併発しているはずです。
グローバル変数へのアクセスが関数の引数に現れていない。
長すぎる関数なので、変数へのアクセスを全て明示したら関数の引数リストがあまりにも長くなってしまうという状況が多いはずです。
追記
・多すぎる#ifdef, #endif
多すぎる#ifdef, #else, #endif は、ソースコードの理解を難しくする。ときとしてこれらの条件付コンパイルが入れ子になっている場合がある。
影響する#defineの組み合わせ全てについてテストしないと、コンパイルが通るソースコードかどうかもわからない。
対策：if文による記述に置き換える。そうすることで、viewDataがTRUE, FALSEのどちらでも確実にコンパイルできるようになる。しかも適切なインデントがされるようになり、可読性も向上する。
const bool viewData = True;
if(viewData){
//some_fuctions();
}
追記
・ソースコード中に残る無効化したマクロ宣言の不安
//#define MACRO_CONST
を見るときに不安を感じないか、「この行の//は削除して有効にすることがあるのだろうか？ それとも、とっくに古くなって削除してよい行なのだろうか？」
対策：
MACRO_CONSTをソースコード全体の中で検索して、他に出現箇所がなければ、その行はさっさと削除してしまおう。他に記述がある場合には、コードを共有しているメンバーに質問すること。それ以外に解決の方法はない。
条件付コンパイルをしないこと、それが解決の糸口です。
・長すぎる関数
様々な要因が込み入った関数は、コード自体が長くても短くても、理解をしにくくするし、メンテナンスやテストをしにくくする。
対策：
意味合いが明確な関数を抽出できないかを検討し、それを抽出することでメンテナンスしやすくする。「関数の抽出」、「メソッドの抽出」と後述の書籍の中にも書いてある。
長すぎる関数から「関数の抽出」をすると、前述のグローバル変数の値を書き換えている関数も、短くなります。グローバル変数の明示的な引数に追加します。
もちろん引数にconst修飾子をつけられる場合はつけます。
抽出した関数での構造体やクラスの引数の渡し方が参照渡し(&data)かポインタ渡し(*data)になっていることは、無駄なコピーを生じさせないために必須なことです。
・抽出した関数は関連性の強いモジュールに含めるようにする。
各c++のソースコードごとのデータや関数の依存性は、Doxygenで生成したhtmlファイルを読むことで確認がしやすくなる。さらには、DoxygenのDoxyfileの設定でソースコードを表示するオプションを有効にしておくことで、チェックがしやすくなる。Doxygenで生成したhtmlファイルの中には、検索用の領域が画面の右上にあり、それを利用して検索することで、その識別子がどのソースコードに含まれているかがわかる。
・必然性のない独自データ形式
標準のライブラリや事実上の標準のライブラリがよくなっているときに、必然性のない独自データ形式を使わないこと。OpenCVのcv::Mat型が充実しているときに、必然性のない独自データ形式の画像データの構造体やクラスを使わないこと。画像データ形式であれば、補間処理や回転処理、部分画像の処理など関連するデータ処理が必要になってしまう。そうすべき必然性がないかぎり独自データ形式を使わないこと。XMLデータの読み書きでさえ、OpenCVに含まれている。
・事実上の標準のデータ形式とライブラリは改善が継続している。
IplImage型の画像データを画素単位でfor文で処理をするのは、OpenCV3がリリースされている状況では現実的なアプローチとは言えない。OpenCV2以降で、cv::Mat型のデータに対する処理がマルチコアの処理が強化されていることを無視してはいけない。OpenMPへのサポートやARMのNEON命令へのサポートがあるのでOpenCVのライブラリのビルドの条件だけ確認すれば、自力でNEON命令を書かなくてよいという利点があります。
RaspberryPi2でさえマルチコアが使えるのです。(追記：IntelのTBBはなんとARMコアのRaspberry Pi 2でも使えるようです。また、C++11で追加になったスレッドライブラリを使うこともできます。) 10年前のライブラリと10年前のＣＰＵとを基準に考えるのは実際的ではないのです。
・こりすぎないこと
自分の与えられた開発課題を実装する上で、必要な範囲が十分に明確になったときには、それ以上の作業をするよりも、与えられた開発課題を実装しよう。
ソースコードのリファクタリングを進めても、評価してくれるのはソースコードを共有する同僚だけです。あなたの給与を確保する経営者には、あなたが実装した新しい機能の方が重要です。まがりなりにも動いているコードを部分的に書き換えてきた理由は、メンテナンスしやすいコードに書き換えなければ、新機能の実現が極めて困難だったからです。
これまでのコードの分析と少しずつの改変で、新機能を実装するまわりの整理ができて、外部仕様が明確になっているはずです。
その外部仕様にしたがって、テストするコードを書くことです。
コードを共有する同僚とコードのメンテナンスの方針について、合意できている範囲から少しずつ行っていくこと。
たいがいの場合、同僚もその部分はきれいな設計に書き換えたくていたりするものです。
他にもメンテナンスしやすくするための留意点は多くあると思いますが、ここに書いた範囲だけでも、だいぶメンテナンスしやすくなるのではないでしょうか。
まとめ
- グローバル変数が残っている
- 多すぎる#ifdef, #endif
- ソースコード中に残る無効化したマクロ宣言の不安
- 長すぎる関数
- 必然性のない独自データ形式
- 事実上の標準のデータ形式とライブラリは改善が継続している。
- こりすぎないこと
「テスト駆動開発による組み込みプログラミング」
追記
以下の文章は、私が経験的に作り上げてきた考えと一致しています。
「既存のシステムに手を加えれば、必ず元より良い物になるはずと考えがちですが、実は何も良くならないこともあるし、もとより悪くなることもあり得るのです。既存のコード、テストを十分に検証しなければ、過去の失敗に学ぶことが出来ません。」 出典[06] リファクタリングの際に注意すべきこと
追記：
バグか仕様か手法の限界か？を書きました。
nonbiri15です。
リファクタリングに関することとして、少しずつ書き加えています。
「C/C++の多次元配列は使うべき理由がない。」
http://qiita.com/nonbiri15/items/a5c05eefbb297716b3a5
nonbiri15です。
試行的リファクタリングに関連して、次の記事を書きました。
「「野放しのグローバル変数」からクラスを見つけ出す」
http://qiita.com/nonbiri15/items/d4c85bf586ed694e58b1
やっかいを抱えているプログラムの問題は、ソースコードが単体テストしやすいモジュールの集まりとして設計されていないことによる場合がある。単体テストしやすい意味の明確なモジュールは、問題の切り分け・問題の定式化に成功している。問題の定式化をし、問題を切り分ければ、自然と単体テストしやすい構造になっていく。そうすればチームによる作業の分担も進めやすくなる。とりあえず、単体テストしやすい部分は何かを考えて、どうやって、プログラム全体の性能を出しやすいのかを考えよう。
デバッグ用のファイル出力にしても、いい加減なコーディングにしない。#if 1 #endifに囲まれたデバッグ用のソースコードでも雑なコーディングをしないほうがいい。#if 1 #endifに囲まれた部分が関数として実装して意味があるようなものならば、#if 1 #endifの外側にふつうに関数として実装しておいて、それを場合によっては#if 1 #endifの中で実行するように書き換えたほうがいい。
#ifdef 0
将来のための実装;
#endif
の場合には
#ifdef FUTURE
将来のための実装;
#endif
などと書いて、不要な実装とは区別してしておこう。
ソースコードを条件付きコンパイルのための#defineをするには、
#define SOME_CASE
などと#defineする行を書くよりは、
コンパイラに対するオプションとして設定すれば、何箇所も#defineを書かなくてすむ。
標準的なデータ構造と標準的なライブラリを使うことで独自実装を減らそう。そうすることで、自前のソースコードは短くなって、メンテナンスをしやすくなる。
単一責務の原理にしたがって、処理の重複を減らしていくと、行数が少なくなることと、同じ変更を複数個所にする手間がなくなるので、メンテナンスがしやすくなる。