Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

usage of IList interfaces in collections. #9

Closed
vpenades opened this issue Dec 6, 2023 · 1 comment
Closed

usage of IList interfaces in collections. #9

vpenades opened this issue Dec 6, 2023 · 1 comment

Comments

@vpenades
Copy link
Contributor

vpenades commented Dec 6, 2023

Hi, I've recently found your WavefrontObj library, and I'm impressed of its level of completeness, I think it's the first time I've seen a wavefront library to handle all the primitives of the format!

I have a small issue with the API, though:

Internally, the library uses List<T> , but public API uses IList<T>

the difference may look trivial at first sight, but there's some critical usability Issues;

.Net has two interfaces for accessing lists: IReadOnlyList<T> and IList<T> , both are implemented by List<T>

At the time these interfaces where introduced, it would had made sense that IList<T> also implemented IReadOnlyList<T> because after all, it has all the methods required to do so, but this didn't happen. and as a consequence of this ill design, there's lots of limitations of what you can do with these interfaces. There's a heated debate at dotnet about how to fix this issue in .Net , but I don't think they're going to find a solution for this any time soon.

The practical consequience is that the properties exposing IList<T> cannot be used on methods requiring IReadOnlyList<T> even if the internal List<T> does implements it.

So my suggestion would be this:

Fully expose List<T> in the public API on those methods and properties that expect you to modify the list. This change would be transparent to current users of the library because List<T> has the same funcionality of IList<T>, with the advantage of also exposing IReadOnlyList<T>.

Conversely, if there's properties exposing IList<T> in a way that the users are not supposed to modify the list in any way, just return IReadOnlyList<T> to better reflect the intentions of the API.

@JeremyAnsel
Copy link
Owner

Hello,
Thank you for the suggestion.

I've updated the library:

  • replaced IList with List
  • replaced IDictionary with Dictionary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants