レガシーコードメンテで感じたC#のアンチパターン実例
発端
先日、人から引き継いだ（特に引き継ぎがあったわけではなくリポジトリを教えてもらっただけ）プロジェクトのコードの調査をする必要が生じました。7,8年前のプロジェクトです。
今でこそ自分の所属するチームではプルリクエスト駆動開発がベースになっていて、まったくコードレビューされないソースはほぼありませんが、昔はそのような体制ではありませんでした。そのため、最終的な品質確保については、コードの質は問わず人力テストを通せればOKというプロジェクトがほとんどでした。
そのため、「今だったらコードレビューで指摘するだろうな」というコードが多くみられました。そのため、「なぜこういう書き方ではないほうがいいのか、どう苦労するのか」という実例としてちょうどいいなと思い、記録に残すことにしました。
プログラミング言語はC#ですが、ほかの言語でも当てはまる内容だと思います。
なお、技術的に特に目新しい話はないので、そんなに期待しないでください。
アンチパターン1: for, foreachの多用
今回のプロジェクトでは、以下のような構成が多用されていました。
もっとネストが深いところもありました。
var listData = new List<string>();
foreach( var property in properties ) {
var targetEntities = entities.Where(entity => entity.PropertyId == property.Id);
foreach( var entity in targetEntities ) {
foreach( var position in positions ) {
var result = entity.GetResult( position );
listData.Add(result);
}
}
}
実際のコードはもっと複雑で、それぞれのforeachの合間で様々な処理が行われています。
実際の処理を大まかに書くと以下のような感じです。
var listData = new List<string>();
foreach( var property in properties ) {
if( /* 条件に一致したら除外する処理 */ ) { continue; }
var targetEntities = entities.Where(entity => entity.PropertyId == property.Id);
foreach( var entity in targetEntities ) {
var positions = new List<PositionType>();
// Entityの状態によってpositionsを追加
// 略
foreach( var position in positions ) {
if( /* 条件に一致したら除外する処理 */ ) { continue; }
var result = entity.GetResult( position );
listData.Add(result);
}
}
}
今回出くわした現象は、「出力データが思ったように出力されてくれない」という問題でした。
全面的にうまくいっていないわけではなく、特定のエンティティに対して不具合と思われる現象が発生しているため、ブレークポイントを設けて一つずつどういう値が入っているかを確認していく必要がありました（ユニットテストがしやすいコードにはなっていませんでした）。
実際にはif 文も1行の簡単な条件ではなく、いろんな変数や値を組み合わせて判定していましたので、ひとつずつ追いかけるだけでも相当な時間を要しました。
好みもあると思いますが、メソッドを分割したり、LINQを使って以下のように構成してもらっていたら、感覚的にはもう少し理解しやすかったのではないかと思います。
void AddData( Entity entity, PositionType position, List<string> listData )
{
var result = entity.GetResult( position );
listData.Add(result);
}
List<PositionType> GetPositions( Entity entity )
{
var positions = new List<PositionType>();
// Entityの状態によってpositionsを追加
// 略
// さらに絞り込む条件があればここで記述
return positions;
}
List<string> GetPropertyDataList( Entity[] entities, Property property )
{
var listData = new List<string>();
foreach( var entity in entities.Where(entity => entity.PropertyId == property.Id) ) {
foreach( var position in GetPositions( entity ) ) {
AddData( entity, position, listData );
}
}
}
void Main()
{
var properties = Model.GetProperties();
var entities = Model.GetEntities();
var listData = new List<string>();
foreach( var property in properties.Where(x => /* フィルタ条件 */) ) {
listData.AddRange( GetPropertyDataList( entities, property ) );
}
}
また、メソッド分割により副作用の範囲を各メソッド内に閉じ込めることにより、改修しようとしたときにほかに想定外の影響を及ぼさないという安心感があります。
もちろん、ダウングレードの可能性はあるので慎重に確認する必要はありますが、数十行のコード内の状態を把握しながら変更しないといけないという状況からは脱することができると思います。
なお、私は関数型っぽく書くのが好みなので、今書くとしたらさらに以下のように書くと思います。
List<PositionType> GetPositions( Entity entity )
{
var positions = new List<PositionType>();
// Entityの状態によってpositionsを追加
// 略
// さらに絞り込む条件があればここで記述
return positions;
}
List<string> GetPropertyDataList( Entity[] entities, Property property )
{
return
entities.Where(entity => entity.PropertyId == property.Id)
.SelectMany(entity => GetPositions( entity ).Select(position => entity.GetResult( position ))
.ToList();
}
void Main()
{
var properties = Model.GetProperties();
var entities = Model.GetEntities();
var listData =
properties.Where(property => /* フィルタ条件 */)
.SelectMany(property => GetPropertyDataList( entities, property ))
.ToList();
}
LINQを多用したコードなので好き嫌いあると思います。ただ、LINQがいいのは、なんとなくLINQのどの関数を使おうとしているかで意図が透けて見える（for, foreachは汎用過ぎて深読みしないとわからない）ところにあると思っています。それにより、コードを読み解くときの理解が速いように感じています。
アンチパターン2: 一つのメソッドでいろんなことをしすぎる
改修するときに頭を悩ませたコードの一つが以下のようなものでした。
void MainMethod()
{
double result = GetSomeData( entity, isSpecialCondition );
/* 続く */
}
double GetSomeData( Entity entity, bool isSpecialCondition )
{
double x;
double y;
double z;
if( isSpecialCondition ) {
x = /* 何らかの計算をする */;
y = /* 何らかの計算をする */;
z = /* 何らかの計算をする */;
} else {
x = /* 何らかの計算をする */;
y = /* 何らかの計算をする */;
z = /* 何らかの計算をする */;
}
// classのフィールドに途中経過をセットする
this.X = x;
this.Y = y;
this.Z = z;
result = Calculate( x, y, z ); // x, y, z の値から計算する
return result;
}
計算結果を得るための GetSomeData メソッドを呼ぶと、その時の計算途中経過がプロパティにセットされるような仕様でした。
ここで問題をややこしくしているのは、引数のisSpecialConditionによって結果が変わることです。
isSpecialConditionは固定値ではなく、文脈によって違う状態で計算します。isSpecialCondition=trueで計算値が欲しい場合と、isSpecialCondition=falseで計算値が欲しい場合があります。
そのため、既存のコードでは以下のようなコードがありました。
var result = GetSomeData( entity, true );
var dummy = GetSomeData( entity, false ); // プロパティが書き換わってしまうので戻しておく
これは個人的には衝撃でした。
Getという一見値をとってくるだけのメソッドで、プロパティを更新しているということが理解を難しくしています。
ただ、実際のGetSomeDataの内容は数百行に及び、for文やifによる条件式が多用されていたため、怖くてリファクタリングできませんでした。。
もしリファクタリングできるとしたら、少し冗長にはなりますが、せめて下記のような構成にしたいなと思いました。
一つのメソッドでいろんなことをやりすぎると変更したくなった時に非常に困るので、できるだけ細かく分離することが望ましいです。
void MainMethod()
{
double xSpecial;
double ySpecial;
double zSpecial;
GetParameters( true, out xSpecial, out ySpecial, out zSpecial );
var resultSpecial = GetSomeData( xSpecial, ySpecial, zSpecial );
double xNormal;
double yNormal;
double zNormal;
GetParameters( false, out xNormal, out yNormal, out zNormal );
var resultNormal = GetSomeData( xNormal, yNormal, zNormal );
// isSpecialCondition=false のときの値をプロパティに設定しておく
this.X = xNormal;
this.Y = yNormal;
this.Z = zNormal;
}
void GetParameters( Entity entity, bool isSpecialCondition,
out double x, out double y, out double z)
{
if( isSpecialCondition ) {
x = /* 何らかの計算をする */;
y = /* 何らかの計算をする */;
z = /* 何らかの計算をする */;
} else {
x = /* 何らかの計算をする */;
y = /* 何らかの計算をする */;
z = /* 何らかの計算をする */;
}
}
double GetSomeData( dobule x, double y, double z )
{
result = Calculate( x, y, z ); // x, y, z の値から計算する
return result;
}
アンチパターン3: 引数が多すぎる＋オーバーロードされている
今回出くわした例のイメージです。
void getCsvData( double a, int b, List<object> c, List<object> d, Dictionary<string, object> e, Dictionary<string, object> f, bool g, bool h)
{
/* 100行以上のコード */
}
void getCsvData( double a, int b, List<object> c, List<object> c2, List<object> d, Dictionary<string, object> e, Dictionary<string, object> f, bool g, bool h)
{
/* 上に似ているようでだいぶ違う100行以上のコード */
}
呼び出し元からコードをたどっていったので、オーバーロードされている異なるメソッドがあることになかなか気づけませんでした。
ブレークポイントを設けても思ったように当たらなかったり、直したと思っても挙動が変化せず、だいぶ時間を使ってしまいました。
メソッド名を変えるか、オブジェクト化してポリモフィズムで表現するか、いろいろ解決策は考えられると思いますが、オーバーロードはそれ以外に選択肢がないときのみ使う方がいいのかなと思いました。
昔のC#だとあまりとれる手段がいまほど多くなかったのでこのような選択をしたのかもしれません。
今回の教訓
コーディングとしてのアンチパターンを今回取り上げましたが、今回本当に言いたかったことは、「こういうコードを書くべきです」ということではありません。
今回得た教訓は、
- コードレビューは大切
- コードレビューの中で「なぜこのようにしたほうがいいか」という思想を伝えるのも大切
ということです。
いまのチームではコードレビューが定着していて、私が関数型言語的な書き方を啓蒙しているので、このような頭を悩ますコードが上がってくることはほとんどありません。当然今のメンバーも、最初の方は保守性の低いコードを書いてきていましたが、コードレビューを繰り返すうちに安定したコードを書くようになってきました。
今回の経験は、古くてコードレビューされていなかった頃のプロジェクトのメンテでつらさを感じた反面、「あぁ、やっぱりコードレビューは思い切って導入してよかったなぁ」と強く感じた出来事となりました。
Discussion