Improve public API #1
Conversation
src/Faithlife.Diff/DiffUtility.cs
Outdated
| /// <returns>A sequence of pairs that indicate the differences between the lists.</returns> | ||
| /// <remarks><para>Each pair identifies a range of items in each list. The items before and after each range | ||
| /// are the same in both lists; the items in each range are the difference. One of the two ranges can be | ||
| /// empty, but the index of the range still indicates where the missing items are.</para> | ||
| /// <para>EqualityComparer<T>.Default is used to compare items.</para></remarks> | ||
| public static IEnumerable<ValueTuple<IndexRange, IndexRange>> FindDifferences<T>(IList<T> listFirst, IList<T> listSecond) | ||
| public static IReadOnlyList<(IndexRange FirstRange, IndexRange SecondRange)> FindDifferences<T>(IList<T> first, IList<T> second) |
There was a problem hiding this comment.
PascalCase is the preferred convention for tuple members?
There was a problem hiding this comment.
I figured that might start some debate...
I'm not arguing for a global convention for all tuples, but in the case where they're a return value, it's consistent with .NET property naming in general and with .Item1 and .Item2 in particular. It seemed like the natural choice for a return value although it does look odd in the method signature.
There was a problem hiding this comment.
No objection here. I started with PascalCase, switched to camelCase, but now I'm coming back around...
src/Faithlife.Diff/DiffUtility.cs
Outdated
| /// <returns>A sequence of pairs that indicate the differences between the lists.</returns> | ||
| /// <remarks><para>Each pair identifies a range of items in each list. The items before and after each range | ||
| /// are the same in both lists; the items in each range are the difference. One of the two ranges can be | ||
| /// empty, but the index of the range still indicates where the missing items are.</para> | ||
| /// <para>EqualityComparer<T>.Default is used to compare items.</para></remarks> | ||
| public static IEnumerable<ValueTuple<IndexRange, IndexRange>> FindDifferences<T>(IList<T> listFirst, IList<T> listSecond) | ||
| public static IReadOnlyList<(IndexRange FirstRange, IndexRange SecondRange)> FindDifferences<T>(IList<T> first, IList<T> second) |
There was a problem hiding this comment.
No objection here. I started with PascalCase, switched to camelCase, but now I'm coming back around...
src/Faithlife.Diff/DiffUtility.cs
Outdated
| /// <param name="comparer">The comparer.</param> | ||
| /// <returns>A sequence of pairs that indicate the differences between the lists.</returns> | ||
| /// <remarks><para>Each pair identifies a range of items in each list. The items before and after each range | ||
| /// are the same in both lists; the items in each range are the difference. One of the two ranges can be | ||
| /// empty, but the index of the range still indicates where the missing items are.</para> | ||
| /// <para>If comparer is null, EqualityComparer<T>.Default is used.</para></remarks> | ||
| public static IEnumerable<ValueTuple<IndexRange, IndexRange>> FindDifferences<T>(IList<T> listFirst, IList<T> listSecond, | ||
| public static IReadOnlyList<(IndexRange FirstRange, IndexRange SecondRange)> FindDifferences<T>(IList<T> first, IList<T> second, |
There was a problem hiding this comment.
If IList<T> implemented IReadOnlyList<T>, I'd recommend changing it. Alas...
But maybe we should change it anyway, and introduce an easy way to cast/convert an IList<T> to an IReadOnlyList<T> in Faithlife.Utility. AsReadOnlyList() extension method?
There was a problem hiding this comment.
Arguably, IReadOnlyList<T> is the better argument type, as the implied contract is that the method won't mutate its arguments. I think it's worth changing for that alone.
The only problem with that is a caller who has an IList<T> and can't easily call the method? A new .AsReadOnlyList extension method sounds good. (Try to cast to IReadOnlyList<T>; failing that, execute .ToList()?)
There was a problem hiding this comment.
Wouldn't a shim be cheaper than .ToList()? I guess it's hard to say for sure. An indirection vs. copying the items.
There was a problem hiding this comment.
It would be uncommon, so maybe the shim isn't worth the effort.
There was a problem hiding this comment.
Turns out I need a shim for JSON patching in Faithlife.Json: JToken is IList<T> but not IReadOnlyList<T>. Adding this in Faithlife/FaithlifeUtility#5.
|
@bgrainger, did you ponder replacing |
Not really. I'd prefer to use a framework type. |
|
You know you want to... |
* Return IReadOnlyList so the caller knows the result isn't lazy. * Take IReadOnlyList because the collections aren't mutated. * Name the items in the returned tuples.
Simplify code. Remove unnecessary tests of ValueTuple (a framework type).
This project only uses simple .NET APIs.
bgrainger
force-pushed
the
api-refactor
branch
from
99a8f5a
to
4e7bbc3
Compare
IReadOnlyListso the caller knows the result isn't lazy.I didn't change the input parameters from
IList<T>toICollection<T>orIEnumerable<T>because the returned tuples assume the ability to index into the lists, so the caller should already have a non-lazy, indexable collection.I want to stabilise the API for v1.0 so it can be used by JsonPatch in Faithlife.Json.