長すぎる関数を短くするヒント

　長すぎる関数は、関数の理解を損ねるし、関数のテストも改良もしづらいものになってしまうがちだ。仕様も十分明快というわけでもなく、何をしているのかなぞめいていて、長すぎる関数を短くするのには気がめいってしまいがちだ。
　そんな中、関数の中で処理している内容について深い理解をすることなしに、関数の抽出をするためのヒントに気づいたのでここに記す。

その1：長すぎるif文・for文の対応する{}を見つける方法

　極度に入れ子になって、しかも長すぎるif文・for文・while文があって、エディタで対応するカッコを容易に見つけることができていないとき、次のようにする。
ここから、ここまでは同じif文の範囲だと思う部分を
#if 0 と　#endifで囲みます。

{
    if(){
#if 0
        for(int i=0; i< iend; i++{
            if(a < b){
                for(int j=0; j < jend ; j++){
                    //
                } 
            }else if (a > c){
                for(int k=0; k < kend ; k++){
                    //
                }
            }else{
                //
            }
       }
#endif
   }
}

で挟み込んでみます。
そして、その囲まれた範囲を非表示にしたときに対応関係が崩れていないかをチェックします。（Visual Studioのような統合環境の場合には、その場で教えてくれるし、buildするとそれによって、if文の対応関係でエラーが出ていないかでチェックできます。）そうすると、大きな勘違いをしていないことがわかります。

その２：まずfor文の制御構文を整理する。

for (int i = istart; i < iend; i++){
// i は右辺式としてだけ登場すべきこと
// for()の記述の中でのiのインクリメントの他の理由で
　　// iを変化させてはならないこと
}

まずそのようにfor文を整理する。
for文の()内の記述が標準的な記述と違っているときは、なんらかの問題がある。

その３：for文を含む範囲を関数化する際の引数を機械的に見つける手法

forブロックが長いときには、forブロックを含む直近の領域が１つの関数に表せるべきではないか疑ってみる。

引数なしの状態からコンパイラに必要な引数を見つけてもらう

たとえば、戻り値の型がvoidである場合、
void extractedFunc(){
//関数化のためにコピーしたコード
　//(まだ長いソースコードで、仮引数にする変数が見つけにくいコード)
}

を、既存のコードに加えて、コンパイルしてみます。
そうすると、コンパイラがエラーを返します。
宣言がないとしてコンパイラにしかられた変数を
参照渡し(&)の仮引数として追加します。
追加して再度コンパイルしなおします。
コンパイルエラーがなくなったら、仮引数のリストが正しいことがわかります。
仮引数にconst属性をつけてみて、コンパイルにエラーが生じなければ、仮引数にconst属性をつけておいてください。
仮引数を並べる順序を、他の関数の流儀や部署のコーディングスタイルにそろえます。
@param[in], @param[out]などで引数の説明を書いていきます。
そうして、@briefで関数の説明を書いていくことができます。
(注意：グローバル変数と同一の名称のlocal変数が、抽出元の関数に含まれていると、このやり方は解釈を間違えます。そんな邪悪なコードでないことを祈ります。)

置換機能を利用して機械的に変数を絞り込む

　forブロックを別ファイルにコピーして、その中からforブロック内部でしかスコープを持たない変数を１つ１つ削除していく。
for文の外部にスコープを持つ変数が残る。
どれが関数化の際の引数となるべき変数。
forブロックの中で左辺式になり、forブロックの後にも参照されるならば@param[out]
右辺式としてだけ現れるならば@param[in]
右辺式、左辺式の両方として現れるならば@param[in,out]
そして、Doxygen用のドキュメンテーションコメントはこんな感じになるのではという記述を増やしていく。
　入出力がどういったものか、出力値がどう利用されるのかをつかんでいくと、関数に書き換える部分と、関数を呼び出す側として残る部分として書き換えることができる。
　そして、関数だけを作成してみる。関数への引数は参照渡しにする(ポインタ渡しにしてしまうと、呼び出し側と関数内部で表記が異なってしまうので避ける。コピー渡しは、データ構造のコピーでオーバーヘッドを生じてしまうので避ける。)　。　関数の引数でconst属性をつけることができるものには極力つける。　そして追加作成した関数を含めただけで呼び出しをしていないコードをビルドしてみる。関数の引数が不足していたりすると、この時点で判明する。
　次に、関数の呼び出しを行って、従来のべた書きのコードを置き換えてみる。それによってビルドし、実行してみてうまく動作するか確認する。抽出した関数は、入出力と内部の動作が何をどうするべきかがより明快になっているはずなので、単体テストがしやすくなっているはずです。

長すぎる関数から関数を抽出するに、この方法はいかがでしょうか。

長すぎる関数から抽出すべき関数

for文のブロックが50行を超えていたり、インデントが６つを超えているようなfor文は明らかに関数として抽出すべき範囲を含んでいる。for文のループの制御変数に着目してきれいにしていく。for文がbreakで抜けているときには、for文が終了したときにどのような条件を満たしているのか整理して、ドキュメンテーションコメントを書いてみよう。

関数の抽出で引数が多すぎる場合

長すぎる関数を短くするために関数の抽出を行ったときに、関数の引数のリストがやたら長いときには、引数のデータ型を再検討してみよう。
次のような標準的なデータ構造を使って、複数の引数を１つの引数に置き換えられる可能性がないか考えてみてください。
　double xmin;
double xmax;
double ymin;
double ymax;
の範囲を指定するには,
cv::Rect2dを使うことで、１つの引数に置き換える。
点の指定もcv::Point型を使うこと。

int x0;
int x1;
int x2;
int x3;
と複数のデータをint x[4]とすることで単純化してデータの受け渡しをすることはできないか。

一つの意味のまとまりは、実は構造体やクラスで表すべきものを別々の変数として受け渡しをしていないか。

最後に

あなたの見るコードに数百行の関数がありませんように。

付記：
　コードの臭いには、「長すぎるメソッド:メソッド、関数、手続きが長くなりすぎている。」があげられている。

追記：for文の抜けたときの満たしている条件を明示してみよう。

for文は、break文で抜けたり、continue文でループを次に進んだりします。そのため、for文を終了したときには、一定の条件を満たしている場合があります。その場合、その条件を明示的に書き記してみると、何をどうしているのかが見えやすくなる場合があります。
　for文のループカウンタの変数が、for文の終了後に意味を持って使われている場合があります。それらの条件がある場合、明示的にコメントで示してみると、何をどうしているのかがわかりやすくなる場合があります。
　for文のループ回数が０回だった場合にはどうなるのかとか、見逃している条件とかがないかも考えてみることが、潜在的なバグを減らすことにつながるかもしれません。

for文というのは、明らかに１つの意味のまとまりの単位に見える。その前の初期化処理とループ後の後処理を組み合わせて、言葉にして分かりやすい意味のまとまりになるはずだ。
　もし、そうならないとしたら、１つのfor文の中でいろんな処理をしすぎていないか見直してみよう。ループの中で処理することを別々に分けて１つのfor文を複数のfor文にすることもありだと思う。そうすると単純化した側のfor文が、既存の関数やメソッドで記述できるものになることもあるだろう。OpenMPなどのマルチコアに対応したライブラリを呼び出しやすくなるという場合もあるだろう。

深すぎるif文for文の入れ子

長すぎる関数という症状が出ている場合には、たいてい併発していそうな問題について、記事を書きました。
深すぎるif文for文の入れ子を書き換える

追記：データメンバーをpublicとしてカプセル化をしない

データ構造が十分に一般的なデータ構造の場合には、そのデータメンバーを操作する関数をgetSomething()関数やsetSomething()関数を作るのではなく、データメンバーをpublicにしておいて、その標準的なデータ構造に予め容易されている関数やメソッドを明示的に使う。そうすることでプログラムの見通しは改善するし、コードの理解やメンテナンスが簡単になる。クラスを作ることを推奨する考えではカプセル化を推奨することが多いけれども、ここではあえてカプセル化を推奨しない。それは、データメンバーのデータ構造が十分によく知られたデータ構造であることを前提としているからです。また、CppUnitを使って単体テストをしようとする場合には、カプセル化してメソッドにもprivateを付ける考えでは、CppUnitが使えないこともあります。何をpublicとして何をprivateにすべきかは、そのライブラリがどういう使い方をされるのかが十分に分かっていない段階では、なかなか適切な設計ができなかったりします。開発の初期の段階では、ずぼらなプログラミングとして、全てをpublicとして書き始めて、とにかくその設計を使い出してみるという手があります。そうして使い方が十分に安定してくると、何をprivateにするのがいいのかが見えてきます。

追記：一般性を損なっているのは何か？

　長すぎる関数は、ちょっとした理由で一般性を失っているだけではないか。余分な条件を加えたために、自作のライブラリで対応するしかなくなっているのではないか。よくある似た機能の関数と実は大半は同じだったりしていないか。デバッグ目的のために追加の処理をしているコードを削除してみたら、一般的な処理とそんなにかわらないということはないか。
　今抱えている課題が、世の中にとって本質的に新しい問題であることは極めてまれです。数値計算の問題は、最適化問題として定式化されることが多かったりします。どのような量を最小化するものとしてとらえているのか、最小化するための手法として何を用いているのかによって、見かけ上違っていることが広い分野で使われています。
　線形SVMが何をどう最小化することによって、機械学習をするのかは、たとえば次のサイトに書かれています。
　http://www.neuro.sfc.keio.ac.jp/~masato/study/SVM/SVM_2_2.htm
ラグランジュの未定乗数法
https://ja.wikipedia.org/wiki/%E3%83%A9%E3%82%B0%E3%83%A9%E3%83%B3%E3%82%B8%E3%83%A5%E3%81%AE%E6%9C%AA%E5%AE%9A%E4%B9%97%E6%95%B0%E6%B3%95
も、広い範囲に応用がある計算手法です。一般的な考え方は、とても有効なのです。