今日のコードレビューでこんなコードを見たので、指摘して修正してもらいました。
例１（戻り値の変数を最初に宣言）
public Result GetValue(){
var r = new Result();
r.A = 10; // 実際は何かの計算 ...
r.B = "XXX"; // 実際は何かのデータを取得 ...
return r;
}
戻り値の Result クラスのインスタンスを、メソッドの先頭で初期化しています。その後で、各プロパティ（AやB）を設定して、最後に return で戻り値を返しています。よくある構造で、特に問題があるように思いません。
しかしこのコードは、実際のエンタープライズ開発の現場では、どんどん次のようになってしまうことがあります。
例2（機能が追加された後の、良くないコード）
public Result GetValue(int c){
var r = new Result();
var a = 0;
var b = "";
if( c < 0 ){ return r; }
a = Other.GetValue(c) * 10; // 引数を使った何かの計算 ...
r.A = a;
if( c > 100 ){
r = new Result();
}else{
b = Other.GetData(c); // 引数を使ってデータを取得 ...
r.B = b;
r.C = c;
if( c < 10 ){
r = new Result();
}else{
r.D = "YYYY"; // 追加されたプロパティの何か計算やデータ取得 ...
}
}
return r;
}
機能が追加されるうちに、条件分岐とプロパティが追加されています。実際のコードは、こんな構造が数十行から数百行になることがあります。機能に応じた他のクラスのメソッド（Ohter.GetValueやOther.GetData）が作成されるのではなく、このメソッドの中にそのまま計算やデータの取得処理が実装される場合が多いからです。例えば、画面とそこで使われる機能にフォーカスした詳細設計書では、クラス設計が十分でなく、その中の計算やデータの取得が明言されていない場合があるからです。
そして変数 r を一番上に宣言していたのに習って、他の変数も全てメソッドの一番上に宣言されているようです。その何が一番悪夢かというと、変数 r を最後に return するという構造をそのままにするために、途中で r に new Result() を再設定している部分があるということです。このメソッドは、どうやら引数が条件に合わないときは、Resultクラスの初期値を返すようです。それが良いかどうかという設計は別にしても、rは何度も初期化されるという点は、実行速度やメモリの点で無駄になります。
この例をパッと見ただけでは秒単位での違いが起きるようには見えませんが、例えば Result クラスのコンストラクタで数秒かかるデータの読み取りなどをしていると、目に見えて実行速度が遅くなります。エンタープライズ開発で良く起きる「遅い」や「メモリが足りなくなる」はこういうプログラムが数百、数千とコピペされて量産されるために起きていることがほとんどです。しかし、一度このような条件分岐がネストされた複雑なプログラムコードができてしまうと、リファクタリングはかなり大変になります。単体テストコードがないとやりたくないのですが、エンタープライズ開発では単体テストが作られていないことも多く・・・
しかしこれは直さないと、いつか途中までプロパティを詰めた r をそのまま途中で return してしまってバグになるやつですね。他に途中でaやbの変数を変えてしまってバグになるかもしれません。パフォーマンスとは別の観点でも、できればリファクタリングしてしまいたいです。
例3（シンプルな実装 or リファクタリング後）
public Result GetValue(int c){
if( c < 10 ){ throw new new ArgumentOutOfRangeException("C"); }
if( c > 100 ){ throw new new ArgumentOutOfRangeException("C"); }
return new Result{
A = Other.GetValue(c) * 10, // 引数を使った何かの計算 ...
B = Other.GetData(c), // 引数を使ってデータを取得 ...
C = c,
D = "YYYY" // 追加されたプロパティの何か計算やデータ取得 ...
};
}
本当はリファクタリングではな機能を全く変えるべきではないのですが、引数のチェックは最初に行いArgumentOutOfRangeException を投げるようにすべきでしょう。もし引数がnullになる可能性がある型ならチェックは必須です。これは静的コード分析を有効にしていると必ず最初に警告がでる内容（使って大丈夫か？確認していない引数を使用しない）です。
また、実は条件が c < 0 と c < 10 でかぶっていたので、c < 0 を削除しています。実際のコードでも、かなり複雑な条件の時に、重複した無駄なチェックを繰り返していることが多々あります。このようなシンプルな構造にするとこのような無駄に気づくことができます。
引数チェックの後で最後に、変数を取ることなく、戻り値のクラスのインスタンスを初期化しながら、全てのプロパティを一度に詰めてしまいます。もしここまでシンプルにできない場合には、return の前の行で値を計算・取得しておいて、最後に戻り値に詰めるようにします。
例4（戻り値の変数を宣言しない）
public Result GetValue(){
return new Result{
A = 10, // 実際は何かの計算 ...
B = "XXX" // 実際は何かのデータを取得 ...
};
}
これは最初の例1の、変数を宣言しない書き方の例です。最初の段階でこのように実装してもらえれば、途中で変数の値が書き換わるなどでバグになる心配がありません。これに機能を追加していくのであれば、さっきのような複雑な構造にはなりにくいのではないでしょうか（もちろんそのように心がけておく必要がありますが、前の人のコードを踏襲するという意味で）。
そして変数を追加するときにも、ブロックの中から外へ、出来るだけ近くに宣言して、必要に応じて遠くしていくように、スコープを広げるように。変数の生存期間を出来るだけ短くしてほしいのです。