関数の引数が多すぎる問題
いろんなプロジェクトで関数の引数が多いコードを見かける。人間が理解できるのはせいぜい７個までという話はあるし、減らすことを考えてほしい。
何より重要なのは処理の分割をちゃんと考えていれば引数が多くなることは、ほとんどないという点である。
実際の例を元にサンプルコートを書いてみた。
サンプルコード
画面に8個のテーブルからデータを取得して画面に表示することを考える。
void Main(int id)
{
// 画面の初期表示処理
var table1Rows = GetTable1Rows(id);
var table2Rows = GetTable2Rows(id);
var table3Rows = GetTable3Rows(id);
var table4Rows = GetTable4Rows(id);
var table5Rows = GetTable5Rows(id);
var table6Rows = GetTable6Rows(id);
var table7Rows = GetTable7Rows(id);
var table8Rows = GetTable8Rows(id);
InitializeView(
table1Rows,
table2Rows,
table3Rows,
table4Rows,
table5Rows,
table6Rows,
table7Rows,
table8Rows);
}
/** 初期表示処理 **/
private void InitializeView(
List<Table1Row> table1Rows,
List<Table2Row> table2Rows,
List<Table3Row> table3Rows,
List<Table4Row> table4Rows,
List<Table5Row> table5Rows,
List<Table6Row> table6Rows,
List<Table7Row> table7Rows,
List<Table8Row> table8Rows)
{
// 画面へ設定する。
SetView(
table1Rows,
table2Rows,
table3Rows,
table4Rows,
table5Rows,
table6Rows,
table7Rows,
table8Rows);
}
/** 初期表示処理で呼ばれる謎の読み替え処理 **/
private void SetView(
List<Table1Row> table1Rows,
List<Table2Row> table2Rows,
List<Table3Row> table3Rows,
List<Table4Row> table4Rows,
List<Table5Row> table5Rows,
List<Table6Row> table6Rows,
List<Table7Row> table7Rows,
List<Table8Row> table8Rows)
{
SetTable1(table1Rows);
SetTable2(table2Rows);
SetTable3(table3Rows);
SetTable4(table4Rows);
SetTable5(table5Rows);
SetTable6(table6Rows);
SetTable7(table7Rows);
SetTable8(table8Rows);
}
/** 各DTOを画面に設定する処理。設定するコードそのものは省略した。 **/
private void SetTable1(List<Table1Row> table1Rows)
{
}
private void SetTable2(List<Table2Row> table2Rows)
{
}
private void SetTable3(List<Table3Row> table2Rows)
{
}
private void SetTable4(List<Table4Row> table2Rows)
{
}
private void SetTable5(List<Table5Row> table2Rows)
{
}
private void SetTable6(List<Table6Row> table2Rows)
{
}
private void SetTable7(List<Table7Row> table2Rows)
{
}
private void SetTable8(List<Table8Row> table2Rows)
{
}
/** サンプルのため空インスタンスを返しているが、
* 実際にはTable1,...,Table8までDaoが存在してDBから取得している。
**/
private List<Table1Row> GetTable1Rows(int id)
{
return new List<Table1Row>();
}
private List<Table2Row> GetTable2Rows(int id)
{
return new List<Table2Row>();
}
private List<Table3Row> GetTable3Rows(int id)
{
return new List<Table3Row>();
}
private List<Table4Row> GetTable4Rows(int id)
{
return new List<Table4Row>();
}
private List<Table5Row> GetTable5Rows(int id)
{
return new List<Table5Row>();
}
private List<Table6Row> GetTable6Rows(int id)
{
return new List<Table6Row>();
}
private List<Table7Row> GetTable7Rows(int id)
{
return new List<Table7Row>();
}
private List<Table8Row> GetTable8Rows(int id)
{
return new List<Table8Row>();
}
/**
* サンプルのため単純化しているが、実際にはもっと複雑なテーブル群。
* テーブル設計が悪いなどは、ここでは考えない。
**/
public class Table1Row
{
int Id { get; set; }
string Value { get; set; }
}
public class Table2Row
{
int Id { get; set; }
string Value { get; set; }
}
public class Table3Row
{
int Id { get; set; }
string Value { get; set; }
}
public class Table4Row
{
int Id { get; set; }
string Value { get; set; }
}
public class Table5Row
{
int Id { get; set; }
string Value { get; set; }
}
public class Table6Row
{
int Id { get; set; }
string Value { get; set; }
}
public class Table7Row
{
int Id { get; set; }
string Value { get; set; }
}
public class Table8Row
{
int Id { get; set; }
string Value { get; set; }
}
一番目を引くのは８個の引数を持った関数が２個あり、片方は存在理由が不明な点である。おそらく、より分かりやすい関数名にしたかったのだろうと推測している。また引数が多いだけでなく、無駄に長い変数スコープのせいで、SetTable関数の引数をどうやって取得しているか追うのが大変である。
ちなみに、実際のコードでは、この関数を他で呼んでいるということもない。
変数スコープを短くするだけで、スッキリする。
リファクタリング後のコード
void Main(int id)
{
// 画面の初期表示処理
InitializeView(id);
}
/** 初期表示処理 **/
private void InitializeView(int id)
{
var table1Rows = GetTable1Rows(id);
SetTable1(table1Rows);
var table2Rows = GetTable2Rows(id);
SetTable2(table2Rows);
var table3Rows = GetTable3Rows(id);
SetTable3(table3Rows);
var table4Rows = GetTable4Rows(id);
SetTable4(table4Rows);
var table5Rows = GetTable5Rows(id);
SetTable5(table5Rows);
var table6Rows = GetTable6Rows(id);
SetTable6(table6Rows);
var table7Rows = GetTable7Rows(id);
SetTable7(table7Rows);
var table8Rows = GetTable8Rows(id);
SetTable8(table8Rows);
}
関数の引数の数だけを解決すればいいわけでもない
ついでに、引数は少ないけど問題がある例も出しておく。
private List<Table1Row> _table1Rows = null;
private List<Table2Row> _table2Rows = null;
private List<Table3Row> _table3Rows = null;
private List<Table4Row> _table4Rows = null;
private List<Table5Row> _table5Rows = null;
private List<Table6Row> _table6Rows = null;
private List<Table7Row> _table7Rows = null;
private List<Table8Row> _table8Rows = null;
void Main(int id)
{
// 画面の初期表示処理
_table1Rows = GetTable1Rows(id);
_table2Rows = GetTable2Rows(id);
_table3Rows = GetTable3Rows(id);
_table4Rows = GetTable4Rows(id);
_table5Rows = GetTable5Rows(id);
_table6Rows = GetTable6Rows(id);
_table7Rows = GetTable7Rows(id);
_table8Rows = GetTable8Rows(id);
InitializeView();
}
/** 初期表示処理 **/
private void InitializeView()
{
// 画面へ設定する。
SetView();
}
/** 初期表示処理で呼ばれる謎の読み替え処理 **/
private void SetView()
{
SetTable1(_table1Rows);
SetTable2(_table2Rows);
SetTable3(_table3Rows);
SetTable4(_table4Rows);
SetTable5(_table5Rows);
SetTable6(_table6Rows);
SetTable7(_table7Rows);
SetTable8(_table8Rows);
}
見た目はスッキリしたが、バグの発生率は格段にあがる。private変数を使用している点が問題で、この変数がいつ更新されたのかを理解するのは非常に難しい。サンプルでは代入箇所が一箇所なので分かりやすいが、大抵の場合、別のイベント処理でprivate変数を更新してバグが発生することが多い。
まとめ
スコープを短くすると引数を短くすることが可能なため、実践してほしい。また引数が多くなった場合、減らす方法を考えるべきであるが、private変数を使用するのも良くない。
引数をクラスにまとめるという方法もあるが、変数スコープを考えてからやるほうが良い。今回の場合、クラスにまとめる必要すらないからだ。
補足）サンプルの元となった実際のコードは現役で動いており、調査の必要がでるたびに担当者を悩ませている。