NotImplementedExceptions in FeatureCollection

[ardalis]

Why does FeatureCollection have NotImplementedExceptions when it is implementing IFeatureCollection, for which one would expect perfect parity? If there are parts of that interface that FeatureCollection doesn't use, why are they on the interface?

[davidfowl]

NotImplementedException is most likely wrong and should be not supported exception.

[ardalis]

I'm not sure that's an improvement. Why not adjust IFeatureCollection, which is literally in the same project and folder, to match FeatureCollection's capabilities (or vice versa). Yes, IFeatureCollection inherits from a collection interface, but if that collection interface has too many features, choose a different collection interface to inherit from. Otherwise this is violating Liskov Substitution Principle and resulting in extra bloat in FeatureCollection.

[Tratcher]

Those methods are all inherited from IDictionary. They weren't implemented because nobody needed them yet. We never attempt to consume the collection as an IDictionary and I don't think we need anything beyond TryGetValue and Set/indexer.

Recommendation: Remove IDictionary from IFeatureCollection and just add the methods we need.

[ardalis]

@Tratcher +1 to your recommendation.

[muratg]

Makes sense to me too.

[davidfowl]

/cc @lodejard

[davidfowl]

@Tratcher We want to preserve enumeration also.

[lodejard]

Yep, it started as a get/set interface and dictionary was added when enumeration was needed. Out of curiosity, would it make sense to implement the idictionary interface instead of declaring a get/set/enumerable subset?

[Tratcher]

IDictionary has 10x more functionality/complexity than we need. This collection is also never exposed directly to the application, it only exists between the server and Hosting/DefaultHttpContext. I'm drafting a subset API list now, but so far the only APIs we actually use are Get and Set. Enumeration is a nice to have, but it's not actually used anywhere or available to the app.

Keeping the surface area to a minimum also makes implementing #353 so much easier and more efficient.

[lodejard]

I agree, but lets keep enumeration (of Type) available to support diagnostic scenarios of displaying all features and writing out all reflected properties.

Sent from mobile device

---

From: Chris Rmailto:[email protected]
Sent: ‎7/‎25/‎2015 9:56 PM
To: aspnet/HttpAbstractionsmailto:[email protected]
Cc: Louis DeJardinmailto:[email protected]
Subject: Re: [HttpAbstractions] NotImplementedExceptions in FeatureCollection (#339)

IDictionary has 10x more functionality/complexity than we need. This collection is also never exposed directly to the application, it only exists between the server and Hosting/DefaultHttpContext. I'm drafting a subset API list now, but so far the only APIs we actually use are Get and Set. Enumeration is a nice to have, but it's not actually used anywhere or available to the app.

Keeping the surface area to a minimum also makes implementing #353#353 so much easier and more efficient.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/339#issuecomment-124945910.