1

Let's have a method with the following signature:

A[] Translate(IEnumerable<B> bees)

I wonder if one of the following snippets would result in better execution, in any practical way.

if (bees == null || !bees.Any()) // Let's be super expressive
{ 
    return null;
}

var translated = bees.Select(b => (...)).ToArray();
return translated; // Intermediate var so that I can debug easier

vs.

if (bees == null) // Let's be very expressive
{ 
    return null;
}

var translated = bees.Select(b => (...)).ToArray();
return translated; // Intermediate var so that I can debug easier

vs.

return bees?.Select(b => (...)).ToArray();

Update 16/09: I've made the ifs return null, instead of a new array (which wasn't material to the spirit of my question)

tymtam
  • 31,798
  • 8
  • 86
  • 126
  • 2
    You shouldn’t care about execution time, which aside from some nanoseconds difference (which doesn’t really matter) is the same. More important is which could is better readable (and maintainable). The last one seems to be most clear as you can see with one glance what happens. The intermediate var is not required for debugging as the locals debugging window will show you each result of each method call as if there were an intermediate variable. – ckuri Sep 16 '19 at 06:04
  • 1
    All three parts will not behave in the same way. The last will return null in case bees is null, the others will return an empty array – Sir Rufo Sep 16 '19 at 06:06
  • 1
    Simplest rule, don't enumerate more than once, the rest is up to you – TheGeneral Sep 16 '19 at 06:11
  • Compile them all in a simple hello world app (one class) with different named methods, then look at the program in ildasm.exe - it will tell you what each looks like in compiled form – Caius Jard Sep 16 '19 at 06:12
  • 1
    The equivalent short version would be `return bees?.Select(b => (...)).ToArray() ?? new A[0];` – Oliver Sep 16 '19 at 06:13
  • The second will (as a general rule) be safer (and often faster) than the first. Don't use the first since it will enumerate twice (potentially involving, for example, hitting the database twice). – mjwills Sep 16 '19 at 06:19
  • @mjwills: If it really enumerates depends on the underlying implementation. If it implements `ICollection` too the `.Any()` will check the count property, which is cheap for `List<>`, but could be expensive for linked lists. – Oliver Sep 16 '19 at 06:22
  • 1
    @Oliver I know that. The parameter is of type `IEnumerable` - so I (and you) can't make any assumptions (it was `ICollection` or `IReadOnlyList` etc then sure, it would be a different matter entirely). The second will be safer since it will _always_ work and won't enumerate twice. The first is faster for an edge case where both versions will be **very fast** anyway (i.e. empty). The second will have consistent performance characteristics and will be **much** faster for some scenarios (e.g. `IQueryable`). – mjwills Sep 16 '19 at 06:22
  • 1
    You should use `Array.Empty()` not `new T[]` when you want an empty array. And use `is null` rather than `==` in case someone has overridden `==`. – Ian Mercer Sep 16 '19 at 06:32
  • `I've made the ifs return null, instead of a new array (which wasn't material to the spirit of my question)` Ouch, please don't do that. For the second it is bad (inconsistent with LINQ - for `null` inputs I'd expect an exception thrown, and `null` should never be returned) and for the first it is catastrophic (you aren't just returning `null` (which is not consistent with how LINQ works) but you are returning it for a **perfectly legitimate input** (empty)). But, with your question as stated, use the second or third option. – mjwills Sep 16 '19 at 06:36
  • https://resharper-support.jetbrains.com/hc/en-us/community/posts/206012289-what-does-Possible-multiple-enumerations-of-IEnumerable-mean- and https://stackoverflow.com/questions/8240844/handling-warning-for-possible-multiple-enumeration-of-ienumerable may be worth a read. – mjwills Sep 16 '19 at 06:39
  • *"better for compilation and execution"* - is new, first one doesn't make sense in given context and for latter as usual [race your horse](https://ericlippert.com/2012/12/17/performance-rant/). I like third one liner, but it's my opinion. – Sinatr Sep 16 '19 at 07:10
  • @IanMercer You cannot override `==`, only overload it. And you can be sure no one has overloaded `==` on `IEnumerable<>` – Joren Sep 16 '19 at 12:21

1 Answers1

1

Assuming that Any() is fast, the first should be slightly less expensive than the second as there will be fewer heap allocations when bees is empty (in particular, the Select enumerator and the lambda expression). The third would be equivalent to the second option.

However, if bees is something with a large startup cost like a database query, then calling Any() might actually be slow and the gains made in avoiding Select(...).ToArray() for empty queries would likely be outweighed by the losses made by calling both Any() and Select(...).ToArray() for non-empty queries.

I would recommend writing this the most readable way unless you have a compelling reason to make this performance optimization and the data to back up that it actually improves performance for your use case.

Joren
  • 14,472
  • 3
  • 50
  • 54
  • 1
    The third doesn't throw , cause `?.` stops and returns null on first occurence, so `?.ToArray()` isn't needed. – Oliver Sep 16 '19 at 06:19
  • Haha, you made me panic about the `ToArray()` and quickly add `?.`, but, yeah, it's not needed :) – tymtam Sep 16 '19 at 06:29
  • @Oliver fixed. This wasn't the main point of my answer though. – Joren Sep 16 '19 at 07:19