FallbackIfEmpty: preserve source and fallback collections if possible

[fsateler]

No description provided.

[atifaziz]

Consider turning these into test cases.

[fsateler]

I did this on ccca164 , but I'm not sure it ended up being better, as it is a bunch of extra code noise.

[atifaziz]

I agree that ccca164 isn't any better. What I had more in mind was this:

[TestCase(new[] { 12                     })]
[TestCase(new[] { 12, 23                 })]
[TestCase(new[] { 12, 23, 34             })]
[TestCase(new[] { 12, 23, 34, 45         })]
[TestCase(new[] { 12, 23, 34, 45, 56     })]
[TestCase(new[] { 12, 23, 34, 45, 56, 67 })]
public void FallbackIfEmptyPreservesSourceCollectionIfPossible(int[] fallback)
{
    var source = new[] { 1 };
    Assert.AreSame(source.FallbackIfEmpty(fallback), source);
}

[fsateler]

But those test cases don't add much value, as they all test the same overload. The idea is to test that all overloads preserve the source collection.

[atifaziz]

Duh! My bad. All the overloading made it very opaque at the call site. Let's revert ccca164 then and ignore the remark about breaking it down into test cases.

[atifaziz]

This local method could be called just _, as we have done in practically all other cases. It is the most default case whereas the rest is just mere optimizations.

[atifaziz]

For sake of consistency with the rest of the code base, please use the BDS style for placement of braces (with the exception of the do…while loop which looks fine as-is since the do block is a simple one-liner).

[fsateler]

Arrgh. I really need to finish up the editorconfig so that this stops becoming an issue.

[atifaziz]

Why not make this as simple as predicating on fallback being null or not?

IEnumerable<T> fallbackChooser() => fallback ?? instancesFallback();

[fsateler]

Because we don't check that (count == 0 || count > 4) => (fallback == null). I didn't want to introduce a nullness requirement, when previously all that was required was that it be non-null when it was to be used.

[atifaziz]

Not sure I understand you here. Could you re-phrase?

[fsateler]

In this method, we never assert that fallback must be null when count is between 1 and 4 (inclusive). We only assert that it must be non-null if count is not within that range.
Now, the callers do indeed pass null in that case, but I saw no reason to impose that as a requirement.

[atifaziz]

I see. Do we want to add the assertions? I guess what bothers me is having to worry about the range and seeing the 4 hard-coded. This is one way to get rid of that:

IEnumerable<T> FallbackChooser()
{
    return count == null ? fallback : InstancesFallback(count.Value);

    IEnumerable<T> InstancesFallback(int c)
    {
        if (c < 0) throw new ArgumentOutOfRangeException(nameof(count));
        if (c > 0) { c--; yield return fallback1; }
        if (c > 0) { c--; yield return fallback2; }
        if (c > 0) { c--; yield return fallback3; }
        if (c > 0) { c--; yield return fallback4; }
        if (c > 0) throw new ArgumentOutOfRangeException(nameof(count));
    }
}

The downside is that the last throw won't happen until after the last yield so yet another way is:

IEnumerable<T> FallbackChooser()
{
    switch (count)
    {
        case null: return fallback;
        case int n when n == 1: return Fallback1();
        case int n when n == 2: return Fallback2();
        case int n when n == 3: return Fallback3();
        case int n when n == 4: return Fallback4();
        default: throw new ArgumentOutOfRangeException(nameof(count));
    }

    IEnumerable<T> Fallback1() { yield return fallback1; }
    IEnumerable<T> Fallback2() => Fallback1().Concat(fallback2);
    IEnumerable<T> Fallback3() => Fallback2().Concat(fallback3);
    IEnumerable<T> Fallback4() => Fallback3().Concat(fallback4);
}

[fsateler]

I like the last version check. I made the switch is slightly different though as I preserved InstancesFallback. What do you think?

[atifaziz]

This could be further localized to your fallbackChooser.

Local functions, like methods, should still use the PascalCase naming convention.

[atifaziz]

Local functions, like methods, should still use the PascalCase naming convention.

[atifaziz]

What about reverting ccca164? Then I think we're done. 🏁

[fsateler]

What about reverting ccca164? Then I think we're done. 🏁

Awesome. Just pushed a revert of that commit.

[atifaziz]

Did you forget to push the revert commit? I don't see it yet in the PR branch.

…

On 10 Aug 2017, at 20:17, Felipe Sateler ***@***.***> wrote: What about reverting ccca164? Then I think we're done. 🏁 Awesome. Just pushed a revert of that commit. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[fsateler]

Did you forget to push the revert commit? I don't see it yet in the PR branch.

Oops. The push was rejected due to me not having pulled your commits first. Now its there.