【C#バグ探し】IList<T> は便利だけど

お題

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 というメソッド名があいまいで、ブラックボックスすぎる
• 読み取り操作と変更操作が同じメソッド内にある

正解は具体的な要件や文脈によります。
これらの点を踏まえ、リファクタリングを検討しましょう。

と放り投げたような形ですが、潜在バグを見つけるのが記事の趣旨ですので、ここまでとさせていただきます。