お題
public void DoSomething(IList<int> numbers)
{
foreach (int number in numbers)
{
Console.WriteLine($"処理1:{number}");
}
foreach (int number in numbers)
{
Console.WriteLine($"処理2:{number}");
}
numbers.Add(1);
}
ヒント
引数をより抽象的な型で受け取ることは、一般的には推奨されます。
かといって IEnumerable<T>
型で受け取ると、メソッド内で複数回列挙の可能性が出てしまいます。
ビルドは通ります。
単体テストも、ケースによっては通るでしょう。
それでも安心はできません。
できればコードレビューで拾いたいポイントです。
呼び出す側に思いを馳せ、嗅ぎ分けてみましょう。
問題ケース
var numbers = new int[] { 0 };
foo.DoSomething(numbers);
NotSupportedException: コレクションは固定サイズです。
Add(T)
で例外が発生します。
配列は IList<T>
インターフェイスを実装しますが、変更操作をサポートしない、読み取り専用コレクションです。
ではどうすればいいのか?
まず応急処置からです。
IList<T>
インターフェイスには IsReadOnly
プロパティが用意されています。
これが false
のときだけ変更操作を行うことで、ひとまず例外は回避されます。
ただ、それではメソッドの要件が実現できないと思います。
呼び出す側は漠然と DoSomething
してくれることを期待していますので、配列を渡したときだけ処理が完遂されないのは想定外です。
この分岐がロジック的に正解だとしても、判断は中ではなく外でやる方が明示的でしょう。
引数型を List<T>
に変えてしまえば、例外も上の懸念も解消されます。
しかし、安直なのは否めません。
根本的な問題がほかにあるかもしれません。
気になる点がいくつかあります。
-
DoSomething
というメソッド名があいまいで、ブラックボックスすぎる - 読み取り操作と変更操作が同じメソッド内にある
正解は具体的な要件や文脈によります。
これらの点を踏まえ、リファクタリングを検討しましょう。
と放り投げたような形ですが、潜在バグを見つけるのが記事の趣旨ですので、ここまでとさせていただきます。