callumbwhyte / super-value-converters

A collection of powerful property value converters for cleaner code in Umbraco
MIT License
14 stars 12 forks source link

Add return value of Intersect method. #17

Closed lkoruba closed 3 years ago

lkoruba commented 3 years ago

Add missing assignment of return value method for intersect.

This causes issue when for 2 different Document Types got Last Interface of first allowed type.

It could be also changed to :

return intersection.Intersect(value);

aochmann commented 3 years ago

Or we can do it different: https://stackoverflow.com/a/1676684

With Aggregate and HashSet:

internal static class IEnumerableExtensions
    {
        public static IEnumerable<T> IntersectMany<T>(this IEnumerable<IEnumerable<T>> values)
        {
            if (!values.Any())
            {
                return  Enumerable.Empty<T>();
            }

            return values
                       .Skip(1)
                       .Aggregate(
                          new HashSet<T>(values.First()),
                          (h, e) => { h.IntersectWith(e); return h; });
        }
    }

https://dotnetfiddle.net/UErj6h

Or changing intersect variable type to HashSet and do IntersectWith

internal static class IEnumerableExtensions
    {
        public static IEnumerable<T> IntersectMany<T>(this IEnumerable<IEnumerable<T>> values)
        {
           HashSet<T> intersection = null;

            foreach (var value in values)
            {
                if (intersection == null)
                {
                    intersection = new HashSet<T>(value);
                }
                else
                {
                    intersection.IntersectWith(value);
                }
            }

            return intersection != null  
                 ? intersection.ToList()
                 : Enumerable.Empty<T>();
        }
    }

https://dotnetfiddle.net/vETbCn

callumbwhyte commented 3 years ago

Hey @lkoruba!

H5YR for contributing to SuperValueConverters - glad to see you lot are still using this.

Someone made this exact same change to the V7 version (#13) but for some reason this was never merged up into V8.

Thanks @aochmann for your input here too - I was also curious if this could be made more efficient. A HashSet<T> wasn't used here because in this case maintaining order is important, but I'm going to profile some other options and see what can be done.

This fix is now out on NuGet in V2.1.0! 🎉

Callum

aochmann commented 3 years ago

Hi @callumbwhyte, I did tests on HashSet. According to the documentation (https://docs.microsoft.com/en-us/dotnet/api/system.collections.generic.hashset-1?view=net-5.0#remarks) it stores data in an unstructured way, but SortedSet (https: //docs.microsoft.com/en-us/dotnet/api/system.collections.generic.sortedset-1?view=net-5.0#remarks) with a sorted order.

I did tests and the order of the output collection depends on the first collection (in both cases) taken as the base for the intersect operation.

I wonder if there is another case that I did not take into account that could work in a different way.

Example: https://dotnetfiddle.net/cfRpua

But in general, both approaches do the same 😄