SoftCircuits / OrderedDictionary

.NET library that implements an ordered dictionary.
MIT License
11 stars 2 forks source link

Return Keys and Values as an IList instead of ICollection #4

Closed Evengard closed 1 year ago

Evengard commented 1 year ago

ICollection makes no guarantees about the order of the elements, hence a lot of other interfaces (example - BouncyCastle's X509Name class) requires an IList instead, which does have that kind of guarantee. Now, the actual returned values are already Lists, and IList implements ICollection, so that shouldn't be a problem. Would it be possible to change the return type of Keys and Values properties to IList instead of ICollection? Thanks.

SoftCircuits commented 1 year ago

Possibly. I just don't remember why I made those ICollection to begin with. I'll take a look when I get a minute.

Evengard commented 1 year ago

I'd guess you just mimicked the Dictionary's contract. Thanks for looking into it.

SoftCircuits commented 1 year ago

@Evengard So, I looked into this. The reason I used ICollection instead of IList is because I'm implementing the IDictionary interface, and that interface specifies ICollection. If I change it to IList, I get errors.

1>C:\Users\jwood\Source\Repos\SoftCircuits\OrderedDictionary\SoftCircuits.OrderedDictionary\OrderedDictionary.cs(16,93,16,118): error CS0738: 'OrderedDictionary<TKey, TValue>' does not implement interface member 'IDictionary<TKey, TValue>.Keys'. 'OrderedDictionary<TKey, TValue>.Keys' cannot implement 'IDictionary<TKey, TValue>.Keys' because it does not have the matching return type of 'ICollection<TKey>'.
1>C:\Users\jwood\Source\Repos\SoftCircuits\OrderedDictionary\SoftCircuits.OrderedDictionary\OrderedDictionary.cs(16,93,16,118): error CS0738: 'OrderedDictionary<TKey, TValue>' does not implement interface member 'IDictionary<TKey, TValue>.Values'. 'OrderedDictionary<TKey, TValue>.Values' cannot implement 'IDictionary<TKey, TValue>.Values' because it does not have the matching return type of 'ICollection<TValue>'.
1>Done building project "SoftCircuits.OrderedDictionary.csproj" -- FAILED.
1>C:\Users\jwood\Source\Repos\SoftCircuits\OrderedDictionary\SoftCircuits.OrderedDictionary\OrderedDictionary.cs(16,93,16,118): error CS0738: 'OrderedDictionary<TKey, TValue>' does not implement interface member 'IDictionary<TKey, TValue>.Keys'. 'OrderedDictionary<TKey, TValue>.Keys' cannot implement 'IDictionary<TKey, TValue>.Keys' because it does not have the matching return type of 'ICollection<TKey>'.
1>C:\Users\jwood\Source\Repos\SoftCircuits\OrderedDictionary\SoftCircuits.OrderedDictionary\OrderedDictionary.cs(16,93,16,118): error CS0738: 'OrderedDictionary<TKey, TValue>' does not implement interface member 'IDictionary<TKey, TValue>.Values'. 'OrderedDictionary<TKey, TValue>.Values' cannot implement 'IDictionary<TKey, TValue>.Values' because it does not have the matching return type of 'ICollection<TValue>'.
1>Done building project "SoftCircuits.OrderedDictionary.csproj" -- FAILED.
1>C:\Users\jwood\Source\Repos\SoftCircuits\OrderedDictionary\SoftCircuits.OrderedDictionary\OrderedDictionary.cs(16,93,16,118): error CS0738: 'OrderedDictionary<TKey, TValue>' does not implement interface member 'IDictionary<TKey, TValue>.Keys'. 'OrderedDictionary<TKey, TValue>.Keys' cannot implement 'IDictionary<TKey, TValue>.Keys' because it does not have the matching return type of 'ICollection<TKey>'.
1>C:\Users\jwood\Source\Repos\SoftCircuits\OrderedDictionary\SoftCircuits.OrderedDictionary\OrderedDictionary.cs(16,93,16,118): error CS0738: 'OrderedDictionary<TKey, TValue>' does not implement interface member 'IDictionary<TKey, TValue>.Values'. 'OrderedDictionary<TKey, TValue>.Values' cannot implement 'IDictionary<TKey, TValue>.Values' because it does not have the matching return type of 'ICollection<TValue>'.
1>Done building project "SoftCircuits.OrderedDictionary.csproj" -- FAILED.

I felt implementing IDictionary was important so I dont' feel good about breaking that. Did you find any actual issues with ICollection?

Evengard commented 1 year ago

I see the problem. That's solvable by explicit interface implementations, I think. Let me make a PR. As I said, it makes wrong assumptions about the actual returned values. We have an ordered dictionary, hence we should provide an explicit way to tell that the keys and values are ordered too. And some other interfaces relies on theese assumptions.

SoftCircuits commented 1 year ago

@Evengard Seems to work. Published as 3.0.1. Thanks for your input!

Evengard commented 1 year ago

Thank you too!

SoftCircuits commented 9 months ago

@Evengard So I've had a request for the class to implement IReadOnlyDictionary<TKey, TValue>, which seems like a reasonable request. However, in order to do this, the Keys and Values properties must return type IEnumerable<T> instead of IList. So I'm looking at changing this back, but thought I'd run it by you in case you still think there is a case to be made for not doing this.

I'm not sure I really understood your original request. IEnumerable<T> should guarantee the order of the results. Do you have an example where this is not the case?

Any other points you think should be considered here?

Thanks,

Evengard commented 8 months ago

IEnumerable doesn't guarantee any ordering. Heck, even different enumerations of an IEnumerable may yield completely different results! IEnumerable by itself make basically no guarantees whatsoever other than this may be enumerated by foreach or linq methods (if we don't dive in details too much).

There is an IOrderedEnumerable though which is supposed to guarantee that. This is the actual type you get when you do an .OrderBy on an IEnumerable.

If you want, I can look into supporting IReadOnlyDictionary the same way we did that patch as well. I recon there won't be too much trouble?

SoftCircuits commented 8 months ago

@Evengard Again, can you give an example where IEnumerable<T> alters the order?

I know that, when coming from a database, the results are not ordered unless you use OrderBy, which returns a IOrderedEnumerable<T>. But that is the database and not IEnumerable<T>. So the order depends on the underlying data. As far as I'm concerned, IEnumerable<T> will not alter the order of the sequence on its own. And if you know an example where it would, that's what I would be interested in seeing.

Supporting IReadOnlyDictionary is easy. The only problem is that the Keys and Values properties are expected to return IEnumerable<T>.