Returning a collection inside of an IEnumerable method

Question

The following scenario came out of a discussion I was having with a fellow developer on this project. He likes to execute an Ienumerable as soon as possible, hence why he’s doing it inside of the method. Whereas, I’m thinking that if you wait until the object is actually needed, using differed execution, you’re eliminating wasted objects in the memory.

Knowing that PricingInformation is a List, is my return statement consider best practice or not? Should I defer the execution to a later time? Or is it wise to ToArray() the Select during the return statement?

Which of these approaches do you guys think is more appropriate?

public IEnumerable<ProductPrice> GetPrices(IEnumerable<string> skus, string accountNumber)
{
    if (skus.Any())
    {
        TesscoModel.PricingQuery priceQuery = new TesscoModel.PricingQuery();
        foreach (string sku in skus)
            priceQuery.Skus.Add(new TesscoModel.SkuDetail() { Sku = sku, Quantity = 1 });
        priceQuery.Account = accountNumber;
        TesscoModel.PricingInformationBatch pricingInfoBatch = RestHelper.PostJsonAsync<TesscoModel.PricingInformationBatch>(Settings.PricingServiceEndpoint, priceQuery).Result;

        return pricingInfoBatch?.PricingInformation?.Select(pi => new ProductPrice { Sku = pi.Sku, Price = pi.OnSalePrice != default(decimal) ? pi.OnSalePrice : pi.UnitPrice, ListPrice = pi.ListPrice })?.ToArray() ?? Enumerable.Empty<ProductPrice>();
    }
    else
    {
        return Enumerable.Empty<ProductPrice>();
    }
}

This method then gets called by another method inside of a helper. Inside of which, I am using FirstOrDefault to find the first sku. FirstOrDefault is again an enumerator.

I think the differed execution is more appropriate here, because I shouldn't execute the pointer until I absolutely have to. I'm thinking that by using ToArray() too early, I'm keeping the object on memory for too long. Even though, it might not be needed.

//Get sku Pricing in bulk
IEnumerable<ProductPrice> productPrices = productService.GetPrices(skus, (!account.IsNull()) ? account.AccountNumber : string.Empty);

foreach (var line in lines)
{
    var productPrice = productPrices.FirstOrDefault(p => p.Sku.Equals(line.Sku));
    line.ListPrice = productPrice?.ListPrice ?? default(decimal);
    line.YourPrice = productPrice?.Price ?? default(decimal);
}

Answer 1

There is no 0 and 1 answer

X likes to execute an IEnumerable as soon as possible, hence why X is doing it inside of the method.

Whereas

Y is thinking that if you wait until the object is actually needed, using differed execution, you’re eliminating wasted objects in the memory.

They are both wrong. The answer isn't digital, this is, there is no 0 or 1 answer. It depends. Sometimes you must execute a collection and return a non-IEnumerable.

Materialize when working with databases

Such a case would be querying a database. In this scenario you'd open a database connection (or create a context if it's some ORM) and dispose it at the end. If you don't execute it with ToList or ToArray etc. you won't be able to do it later. The runtime would wrap your enumerator with an anonymous type containing the context but when the execution leaves the method, the context gets disposed and it'll throw an excepiton the moment you try to get the data. In this situation you should return a non-IEnumerable type.

The reason the return type shouldn't be an IEnumerable in this case is that IEnumerable tells me that the return type is lazy so if I wanted to use it multiple times I'd call ToList myslef (again) to get the result which would unnecessrily enumerate the collection again.

So, the bottom line is: if you don't have to materialize the result (because you won't be able to get it later when the service/provider is disposed etc), don't do it but be consistent with the API (return types).

---

When the result value it's materialized I expect you to communicate this fact clearly by using ICollection, IList or anything else but IEnumerable.

---

Materialize for easier debugging when calling resources

As far as your code is concerned you call a web-service there

RestHelper.PostJsonAsync(..)

So you should ask yourself: Is it ok to be done multiple times if someone else uses Any and then starts enumerating it again or is better to prevent such situations (for whatever reason)?

I would use IList here for one more reason: it's quite difficult to debug lazy code that is calling web-services, databases or other resources. When something goes wrong here you'll be having a lot of fun.

---

Consider materialized parameters

The same priciples apply to parameters. In your example you execute skus multiple times: with Any and with the foreach loop.

Here, I'd requested an ICollection or an IList to tell the caller: Look out! I'll be using your collection more then once (so you might consider materializing it for better performance before givit it to me).

Answer 2

Readability

You tend to cram a lot of code into very few lines. Some lines are just too long and complex, in my opinion. There's several things that I would do differently:

• var lets you remove duplicate type name occurrences.
• "" is more succinct than string.Empty. The same goes for 0m versus default(decimal). Personally, I much prefer "" and 0m, not just because they're shorter, but also because their actual value is immediately recognizable.
• For long method calls, you may want to put each argument on a line of its own.
• For chained (Linq) method calls, you may want to put each method call on a separate line.
• Ternary ifs aren't terribly readable, especially not when the same line also contains both Elvis and null coalescing operators. Another reason to split things up into multiple lines.
• Many of the classes involved appear to be missing useful constructors. Not only does that require more writing, it also makes it easy to create invalid objects (I assume that a SkuDetail object without a Sku isn't valid, for example?). If you like writing out property names for clarity's sake, that's also possible with argument names.
• Why do you write out the TesscoModel namespace (or outer class?) prefix everywhere?

Other notes

• You can introduce an early return by changing if (skus.Any()) into if (!skus.Any()), which should make the rest of the method a little easier to read (less indentation levels, no edge-case handling afterwards).
• Are you sure GetPrices shouldn't be an async method? Fetching a Result like that will block the current thread untill the result is available.
• Using default(decimal) (or just 0) to indicate 'has no value' looks quite brittle to me. It's easy to forget a check like that, and doing so might cause products to be given away for free (price = 0)... Using a decimal? instead lets you clearly signal that there may not be a value, and it forces calling code to check for that.

Deferred execution

As for your question, in this particular case deferred execution actually has a negative impact. Each time you iterate the result (which FirstOrDefault does) Select has to do work again - not just for the first matching item, but for all preceding ones as well. That work has to be repeated for each next FirstOrDefault call, and since the results aren't cached anywhere that likely means that a lot of the work will go to waste.

For example, if there's one sku per line, then instead of n you end up with (n^2 + n) / 2 ProductPrice objects being created. That's quadratic rather than linear!

Materializing the results immediately doesn't suffer from that problem. But if you expect a lot of skus then it's probably better to create a dictionary that maps skus to ProductPrice objects. It'll take more work than creating an array, but lookups will take constant rather than linear time, so you end up with a more scalable solution.