1
0

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?

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

Posted at

お題

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

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

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

1
0
2

Register as a new user and use Qiita more conveniently

  1. You get articles that match your needs
  2. You can efficiently read back useful information
  3. You can use dark theme
What you can do with signing up
1
0

Delete article

Deleted articles cannot be recovered.

Draft of this article would be also deleted.

Are you sure you want to delete this article?