Are there any problems with this foreach? (Error, performance, ...)

Question

I found the code below in an old project and I feel that it's maybe not good performance but I am not sure about that. Can anyone explain it for me? Thank you very much.

I edited the code below. I mean it has 2 objects.

var obj = ...; // Object; 
var obj.ListObject_1  = ...; //List<Object>;

var obj2 = ...; // Object; 
var obj2.ListObject_2 = ...; //List<Object>;

foreach (var value in obj != null ? obj.ListObject_1.OrderBy(x => x.displayOrderField).ToArray() : obj2.ListObject_2.OrderBy(x => x.displayOrderField).ToArray()) {

}

Answer 1

When talking about performance it is not enough to just "talk" you need to measure things. So lets just do that.

I did fairly simple benchmark of looping using different constructs and finding the sum of all numbers inside. Notice how the "extra work" (finding sum of collection) does not interfere with the measure since it is doing the same thing in each benchmark.

---

[SimpleJob(RuntimeMoniker.CoreRt31)] // .net core 3.1
public class StackOverflowPerformance
{
    private const int Size = 1_000_000; // 8MB cache, 1 mil ints * 4 byte each = 4MB
                                        // hopefully this will fit into the processor cache
    private List<int> _sut = new List<int>(Size); // assuming you work with List

    [GlobalSetup]
    public void Setup()
    {
        // achieve worst case scenario for our OrderBy
        for (int i = Size; i > 0; --i)
        {
            _sut.Add(i);
        }
    }

    [Benchmark]
    public int ForeachToArray()
    {
        var result = 0;
        foreach(var value in _sut.OrderBy(x => x).ToArray()) // change the underlying structure type
        {
            result += value;
        }

        return result;
    }

    [Benchmark]
    public int ForeachToList()
    {
        var result = 0;
        foreach (var value in _sut.OrderBy(x => x).ToList()) // preserve the underlying structure type
        {
            result += value;
        }

        return result;
    }

    [Benchmark]
    public int Foreach()
    {
        var result = 0;
        foreach (var value in _sut.OrderBy(x => x)) // OrderBy returns IEnumerable<int> this will
                                                    // most likely introduce virtual calls

        {
            result += value;
        }

        return result;
    }

    [Benchmark(Baseline = true)]
    public int For()
    {
        var result = 0;
        _sut = _sut.OrderBy(x => x).ToList(); // preserve the underlying type of the structure
        for (int i = 0; i < _sut.Count; i++)
        {
            result += _sut[i];
        }

        return result;
    }

}

---

And here are the results:

---

So my suggestion is:

If you are worried about performance you should think about converting this foreach loop into for loop whenever this is possible and keep in mind that this is not a silver bullet, sometimes working with IEnumerable will produce better results than casting to array or list.

Really nice in depth analysis of loop performance in C#

The next thing is like others said, don't call ToArray() or ToList() unless you need to use the concrete type for something related to it, for instance you can call Find only on List<T>, because this will introduce additional overhead like looping over the loop one more time.

Answer 2

I feel that it's maybe not good performance but I am not sure about that

You could improve the performance of this by removing the call to ToArray() - you're essentially iterating the collection once in your call to ToArray(), and then iterating it again in your foreach loop.

In terms of performance the ternary operator is fine, it will evaluate the condition obj != null, and then depending on the value of that will evaluate one of of the other operands - it does not evaluate both obj.ListObject_1.OrderBy(x => x.displayOrderField).ToArray() and obj.ListObject_1.OrderBy(x => x.displayOrderField).ToArray().

Edit: complete re-write following @Luân Đình fixing key typo in OP