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

Separate core algo into a more generic version #12

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

atifaziz
Copy link
Contributor

@atifaziz atifaziz commented May 4, 2020

I would have opened an issue first but some ideas and benefits are easier to demonstrate as a PR and working example, so here goes…

This PR primarily separates the diff algorithm from supporting types ListDiffAction<,>, ListDiffActionType and ListDiff<,> and renders it even more generic so that it can stand and be used on its own; this leads to many benefits.

First and foremost, the core function becomes very generic:

List<R> Diff<S, D, R> (IEnumerable<S> source, IEnumerable<D> destination,
                       Func<S, D, bool> match,
                       Func<S, D, R> updateResult,
                       Func<D, R> addResult,
                       Func<S, R> removeResult,
                       out bool containsOnlyUpdates)

The 3 functions updateResult, addResult and removeResult project the result elements such that the function can return a simple list (List<>) of those results. The shape of R is now up to the caller based on relevant inputs.

The constructor of ListDiff<,> has been updated to use it as follows:

Actions = Diff (source, destination, match,
                (s, d) => new ListDiffAction<S, D> (ListDiffActionType.Update, s, d),
                d      => new ListDiffAction<S, D> (ListDiffActionType.Add, default, d),
                s      => new ListDiffAction<S, D> (ListDiffActionType.Remove, s, default),
                out var containsOnlyUpdates)

The are several other overloads added for convenience. For example, when source and destination elements are the same some type T then you get a simpler signature:

List<R> Diff<T, R> (IEnumerable<T> source, IEnumerable<T> destination,
                    Func<T, T, R> updateResult,
                    Func<T, R> addResult,
                    Func<T, R> removeResult,
                    out bool containsOnlyUpdates)

This uses the default comparer (EqualityComparer<T>.Default) so a function to determine a match isn't needed. This also solves the boxing/performance problem highlighted in issue #8.

Since we have tuples, we can go one step further for simpler scenarios with the following signature:

List<(A, S, D)> Diff<S, D, A> (IEnumerable<S> source, IEnumerable<D> destination,
                               Func<S, D, bool> match,
                               A update, A add, A remove,
                               out bool containsOnlyUpdates) =>

That is, given 3 tags to represent an update, add or remove node, the function will return a list of triplets where the source and destinations elements are annotated with the appropriate tag.

In its current design, the stand-alone Diff method is part of a class called DiffModule. This permits using the method directly via a static import (using static ListDiff.DiffModule).

I have also placed DiffModule in its own file and made the definition partial and private by default. Then in ListDiff.cs, the partial definition has the public modifier to make it public. The idea behind this seemingly bizarre approach is that it enables someone to just take Diff.cs and run with it in their project without inheriting any dependencies or types of the project. DiffModule will automatically become a private artifact of the user's project. However, when included together with ListDiff.cs as part of this project, it is exposed publicly.

If you like what you see here, I am happy to put polishing touches like doc comments and argument validation.

Looking forward to your review and thoughts.

@atifaziz
Copy link
Contributor Author

atifaziz commented May 4, 2020

@praeclarum I am afraid I am unable to see the details of the Bitrise check to understand what caused it to fail. Locally, dotnet build works and all tests pass.

@praeclarum
Copy link
Owner

The Bitrise errors are just missing XML comments. Hopefully you'll see them with a release build.

This is really great. I like it a lot, especially if the public API doesn't break.

I am a tiny bit concerned with performance only because complex generic uses can cause heavy trampolining on iOS and other AOT scenarios. It's one of the reasons I didn't include the generic equality comparer in the first place.

That said, nothing about your re-architecture should be inefficient. I think it will behove the library to have some microbenchmarks. I can try to write some in master for comparison.

I still need to review the code, but I like everything from your description.

@atifaziz
Copy link
Contributor Author

atifaziz commented May 6, 2020

The Bitrise errors are just missing XML comments. Hopefully you'll see them with a release build.

Yes, I see them with dotnet build -c Release.

This is really great. I like it a lot, especially if the public API doesn't break.

That's great to hear!

I am a tiny bit concerned with performance only because complex generic uses can cause heavy trampolining on iOS and other AOT scenarios. It's one of the reasons I didn't include the generic equality comparer in the first place.

I see. I wasn't aware of those as I don't do much mobile development. Anywhere I can read up on that?

I still need to review the code, but I like everything from your description.

Awesome and looking forward to your review. Thanks for your feedback in the meantime.

This is just to fix release builds for now and will be fleshed post
initial PR review.
@atifaziz
Copy link
Contributor Author

atifaziz commented May 6, 2020

I've put doc comments as to-do with 9e67da4 for now and the Bitrise check is passing now. I'll put the actual doc comments when you give me the go to move forward.

@atifaziz
Copy link
Contributor Author

atifaziz commented Jun 4, 2020

Anything I can do to help move this forward?

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

Successfully merging this pull request may close these issues.

2 participants