Make it possible for third-party libraries to access the underlying collection of instances of ReadOnlyNonGenericCollectionWrapper

[logiclrd]

Initial implementation: Per @dennisdoomen in comments on #1893, this PR marks the ReadOnlyNonGenericCollectionWrapper<TCollection, TItem> type public so that it can be referenced by an extension package. An XMLDoc comment is added to advise perusers that the type is an internal implementation detail, despite its access modifier.

Take two: Instead of altering the ReadOnlyNonGenericCollectionWrapper<TCollection, TItem> type, an interface IHasUnderlyingCollection<TCollection> is introduced that permits consumers to interact with types that have an UnderlyingCollection of a specific type without needing access to the actual implementation.

IMPORTANT

• The code complies with the Coding Guidelines for C#.
• The changes are covered by unit tests which follow the Arrange-Act-Assert syntax and the naming conventions such as is used in these tests.
• If the PR adds a feature or fixes a bug, please update the release notes with a functional description that explains what the change means to consumers of this library, which are published on the website.
• If the PR changes the public API the changes needs to be included by running AcceptApiChanges.ps1 or AcceptApiChanges.sh.
• If the PR affects the documentation, please include your changes in this pull request so the documentation will appear on the website.

[jnyrup]

What members do you need access to?
Hyrum's Law applies to everything we expose.

At least I'd like to change the interface methods to explicit interface implementations instead.

[logiclrd]

The proposed extension library needs to be identify the type, that's all. Explicit interface implementations should work just fine.

[logiclrd]

Oh, I'm mistaken, the UnderlyingCollection property is needed as well.

Should I just make that one property public or would you like another interface to be made to expose that property so that it can be accessible but not visible by default?

[logiclrd]

Hmm, in fact, if an interface were introduced for UnderlyingCollection, then the consuming code would only need that, not the concrete type, so that can go back to being internal.

[logiclrd]

Okay the implementation has been switched to this. The concrete type is not exposed at all.

[logiclrd]

@dennisdoomen Implementation has been changed to expose a new interface instead of the concrete type. This interface serves the same purpose for identifying the wrapper and providing access to the underlying non-generic collection.

[jnyrup]

I had on my todo to change the methods to explicit interface implementations anyways, but since we're about to expose an internal construct, I just wanted to get those changes in before exposing it.

I guess the interface is to hide additional implementation details of ReadOnlyNonGenericCollectionWrapper?
I'm fine with exposing the concrete class instead of adding an interface.
Disclaimer: I think interfaces are heavily overused.

public class ReadOnlyNonGenericCollectionWrapper<TCollection, TItem> : System.Collections.Generic.ICollection<TItem>, System.Collections.Generic.IEnumerable<TItem>, System.Collections.IEnumerable
    where TCollection : System.Collections.ICollection
{
    public TCollection UnderlyingCollection { get; }
}

public class ReadOnlyNonGenericCollectionWrapper<TCollection, TItem> : ICollection<TItem>
    where TCollection : ICollection
{
    public TCollection UnderlyingCollection { get; }

    internal ReadOnlyNonGenericCollectionWrapper(TCollection collection)
    {
        Guard.ThrowIfArgumentIsNull(collection, nameof(collection));

        UnderlyingCollection = collection;
    }

    int ICollection<TItem>.Count => UnderlyingCollection.Count;

    bool ICollection<TItem>.IsReadOnly => true;

    IEnumerator<TItem> IEnumerable<TItem>.GetEnumerator() => UnderlyingCollection.Cast<TItem>().GetEnumerator();

    IEnumerator IEnumerable.GetEnumerator() => UnderlyingCollection.GetEnumerator();

    bool ICollection<TItem>.Contains(TItem item) => UnderlyingCollection.Cast<TItem>().Contains(item);

    void ICollection<TItem>.CopyTo(TItem[] array, int arrayIndex) => UnderlyingCollection.CopyTo(array, arrayIndex);

    void ICollection<TItem>.Add(TItem item) => throw new NotSupportedException();

    void ICollection<TItem>.Clear() => throw new NotSupportedException();

    bool ICollection<TItem>.Remove(TItem item) => throw new NotSupportedException();
}

[logiclrd]

That's an interesting take. The point of an interface is to disconnect consumers from one specific concrete implementation. Without requiring that additional implementations be made, it makes the consuming code more general and allows it to focus on the details it actually needs & uses. I personally think this is not overuse of interfaces but rather exactly what they were designed for -- ReadOnlyNonGenericCollectionWrapper is a highly-specific internal implementation detail, and interfaces allow it to be kept internal and for the feature that actually matters to be exposed.

I can change this implementation to expose the internal detail instead of using an interface -- it's your project ultimately -- but it feels like objectively worse code to me personally. ¯\_(ツ)_/¯

[dennisdoomen]

I agree with @logiclrd here. An interface would be much better.

[jnyrup]

I like the observations done in https://blog.ploeh.dk/2010/12/02/Interfacesarenotabstractions/ and especially this part comes close to how I look at IHasUnderlyingCollection.

Header Interfaces
Since a higher number of members increases the risk of unexpected side effects and temporal coupling it should come as no surprise that interfaces mechanically extracted from all members of a concrete class are poor abstractions.

As always, Visual Studio makes it very easy to do the wrong thing by offering the Extract Interface refactoring feature.

We call such interfaces Header Interfaces because they resemble C++ header files. They tend to simply state the same thing twice without apparent benefit. This is particularly true when you have only a single implementation, which tends to be very likely for interfaces with many members.

If we compare the differences in our public API, which is the contract to our consumers about the shape of the publicly exposed types.

public interface IHasUnderlyingCollection<TCollection>
    where TCollection : ICollection
{
    TCollection UnderlyingCollection { get; }
}
    
public class ReadOnlyNonGenericCollectionWrapper<TCollection, TItem> : ICollection<TItem>
    where TCollection : System.Collections.ICollection
{
    public TCollection UnderlyingCollection { get; }
}

The differences I see are:

• The interface is named IHasUnderlyingCollection, the class ReadOnlyNonGenericCollectionWrapper
• IHasUnderlyingCollection hides that ReadOnlyNonGenericCollectionWrapper is also generic over TItem and implements ICollection<TItem>.
• Consumers can pattern match on the bit shorter Subject is IHasUnderlyingCollection<DataColumnCollection> instead of Subject is ReadOnlyNonGenericCollectionWrapper<DataColumnCollection, DataColumn>.
• Consumers can create their own implementations of IHasUnderlyingCollection and it will still work inside FA.
  • I do not expect consumers to create other implementations.
• We avoid exposing that we don't properly implement ICollection<T> as some methods throw a NotSupportedException.

Of all these points, only the last one is an improvement to me, but also enough for me to warrant an interface.
We could get rid of that by only letting ReadOnlyNonGenericCollectionWrapper implement IEnumerable<TItem> or IReadOnlyCollection<TItem>, but could would hurt performance a tiny bit in some cases.
So for that particular reason I agree adding an interface here is justified.

Now for the naming :)
I know Dennis likes the IHas prefix for interfaces, but (as teenager of the 00's) I read every single of those names as ICanHasCheezburger.
I don't have a good name off the top of my head (and I won't be giving it more thoughts tonight)

[logiclrd]

I'm on the periphery here but my €0.02 is that the I in an interface name definitely is not a personal pronoun, and I cringe every time I see interfaces named as though the I is the interface talking about itself.

IAddAClientURL, ICreateUserDetails, IProvideBackendAddresses -- ouch, make it stop

When reading the name of an interface, the I isn't a part of the name. That's my view on it. :-P

[logiclrd]

Regarding the differences you see, I posit that the second one is also valuable:

• IHasUnderlyingCollection hides that ReadOnlyNonGenericCollectionWrapper is also generic over TItem and implements ICollection<TItem>

This is an important abstraction, in my opinion. You show what the type looks like if all of its implementations are explicit, but that's a false equivalence; the implementations are effectively still public features of the type because the list of implemented interfaces is a public feature. They are just as discoverable. A better comparison would enumerate the explicit implementations as well, even though they're not public:

public interface IHasUnderlyingCollection<TCollection>
    where TCollection : ICollection
{
    TCollection UnderlyingCollection { get; }
}

vs.

public class ReadOnlyNonGenericCollectionWrapper<TCollection, TItem> : ICollection<TItem>
    where TCollection : System.Collections.ICollection
{
    public TCollection UnderlyingCollection { get; }

    int ICollection<TItem>.Count { get; }
    bool ICollection<TItem>.IsReadOnly { get; }
    IEnumerator<TItem> IEnumerable<TItem>.GetEnumerator() { get; }
    IEnumerator IEnumerable.GetEnumerator() { get; }
    bool ICollection<TItem>.Contains(TItem item);
    void ICollection<TItem>.CopyTo(TItem[] array, int arrayIndex);
    void ICollection<TItem>.Add(TItem item);
    void ICollection<TItem>.Clear();
    bool ICollection<TItem>.Remove(TItem item);
}

(I am assuming you'd have the constructor be made internal.)

[dennisdoomen]

I might be a bit pragmatic, but I don't have the time to dive into a philosophical discussions about whether an interface or a class is whether and what naming convention to use. The fact that we use this ReadOnlyNonGenericCollectionWrapper is just a trick to avoid having to duplicate tens of assertion methods. It's an implementation detail, albeit a bit hacky. If Jonathan wants to extend this with custom assertions, he'll need to hook into this hacky solution. If the only thing he needs to know if a collection is a read-only wrapper with an underlying collection, I prefer to have an interface like IReadOnlyWrapper or IHaveUnderlyingCollection (yes, I do like role-based fluent interface names)

[logiclrd]

I do like role-based fluent interface names

Well that's unfortunate :-D

But, the existing interfaces within FluentAssertions seem to be named more conventionally:

• IParsingStrategy
• IClock
• IReflector
• ITimer
• IAssertionContext<TSubject>
• IEquivalencyAssertionOptions
• IEquivalencyStep
• IMemberInfo
• IMemberMatchingRule
• etc.

Based on these names, I would suggest that either IHasUnderlyingCollection is acceptable, or it should be changed to something like ICollectionWrapper. I think the emphasis should be on the fact that it wraps a collection and not the fact that it's read-only, since the proposed interface doesn't provide access at all. The class is a read-only wrapper, but the interface is an underlying-collection-accessor only.

[jnyrup]

I'm sorry for the bike-shedding.
My previous comment should probably just have been my internal notes for realizing the justification of an interface.
I agree with everything both of you wrote in the subsequent comments.

I'm absolutely fine with ICollectionWrapper.

One last question, do you think it be useful or irrelevant/leaking of abstractions to let ICollectionWrapper implement IEnumerable<TItem>?

[logiclrd]

I don't think it would be useful at present, and it would mean that it would have to take a TItem generic argument too. I think it would be preemptively solving a problem that doesn't exist at present.

[coveralls]

Pull Request Test Coverage Report for Build 2835202715

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+1.0%) to 96.655%

---

Totals
Change from base Build 2822953981:	1.0%
Covered Lines:	12303
Relevant Lines:	12569

---

💛 - Coveralls