JeremyAnsel / JeremyAnsel.Media.WavefrontObj

A .Net library to handle Wavefront Obj .obj and .mtl files.
https://jeremyansel.github.io/JeremyAnsel.Media.WavefrontObj/
MIT License
10 stars 7 forks source link

usage of IList interfaces in collections. #9

Closed vpenades closed 10 months ago

vpenades commented 11 months ago

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 commented 11 months ago

Hello, Thank you for the suggestion.

I've updated the library: