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

Redesign IFeature / IAttributesTable to better model the different use cases it solves #6

Open
airbreather opened this issue Jun 19, 2019 · 3 comments
Labels
breaking change Doing this would break either source or binary compatibility with existing versions.

Comments

@airbreather
Copy link
Member

airbreather commented Jun 19, 2019

During #8, I posited that it might be better to tweak IFeature and/or IAttributesTable to better model the two different use cases that IAttributesTable supports:

  1. Some IAttributesTable instances have their set of valid attributes defined by something external to the individual instance (e.g., shapefiles, GPX, relational databases).
    • With these kinds of tables, you can tell when certain attributes are missing, and what type of data is they would need to hold when set.
  2. Other IAttributesTable instances are more dynamic and can hold any data, or at least the system isn't supposed to reject anything outright (e.g., GeoJSON).
    • With these kinds of tables, you can add just about anything without errors.

From a reader's perspective, there's no difference between the two, but a writer who consume just the interface(s) would appreciate knowing what the rules are at compile-time.

Perhaps the interface(s) only need to support reading, and writing could be handled by using the concrete type directly. Alternatively, perhaps we can take a similar approach to what .NET did with its read-only collection interfaces, and expose a "read-only" base interface with different sub-interfaces that support the different kinds of writing.

Original question that sparked this issue:

Can IAttributesTable / AttributesTable be replaced with just IDictionary<string, object> directly on the Feature class?

That's basically all this is, anyway...

@airbreather airbreather added question Further information is requested breaking change Doing this would break either source or binary compatibility with existing versions. labels Jun 19, 2019
airbreather added a commit that referenced this issue Jul 28, 2019
- All CoordinateSystems types are gone (resolves #7)
- (I)AttributesTable is gone, in favor of bare Dictionary<string, object> (resolves #6) (resolves #3)
- FeatureCollection now inherits from Collection<Feature>
- [Serializable] is now implemented via ISerializable
@FObermaier
Copy link
Member

IAttributesTable has 7 function and a property to implement, IDictionary<string, object> has lots more.

For specific classes implementing IFeature I think the current approach is better.

@airbreather
Copy link
Member Author

airbreather commented Aug 1, 2019

Can IAttributesTable / AttributesTable be replaced with just IDictionary<string, object> directly on the Feature class?

IAttributesTable has 7 function and a property to implement, IDictionary<string, object> has lots more.

For better or for worse, IDictionary<string, object> is the standard interface for the kind of object that we need, and IMO that outweighs the concern of wanting to keep things simpler for implementers.

Agreed, though, custom implementations of IDictionary<TKey, TValue> aren't easy. If it's important to support custom implementations, then perhaps we could help solve that by offering an abstract base class that implements the interface?

1 hour or so (completely untested) got me this, which only requires implementing 5 members:

public abstract class EasierDictionary<TKey, TValue> : IReadOnlyDictionary<TKey, TValue>, IDictionary<TKey, TValue>
{
    protected EasierDictionary()
    {
        Keys = new KeyCollection(this);
        Values = new ValueCollection(this);
    }

    public abstract int Count { get; }

    public abstract IEnumerator<KeyValuePair<TKey, TValue>> GetEnumerator();

    public abstract void Clear();

    protected abstract bool GetOrRemoveValue(TKey key, out TValue value, bool remove);

    protected abstract bool SetValue(TKey key, TValue value, bool onlyIfMissing);

    public KeyCollection Keys { get; }
    ICollection<TKey> IDictionary<TKey, TValue>.Keys => Keys;
    IEnumerable<TKey> IReadOnlyDictionary<TKey, TValue>.Keys => Keys;

    public ValueCollection Values { get; }
    ICollection<TValue> IDictionary<TKey, TValue>.Values => Values;
    IEnumerable<TValue> IReadOnlyDictionary<TKey, TValue>.Values => Values;

    public IEqualityComparer<TKey> KeyComparer { get; set; }

    public IEqualityComparer<TValue> ValueComparer { get; set; }

    public TValue this[TKey key]
    {
        get => GetOrRemoveValue(key, out var value, false) ? value : throw new KeyNotFoundException();
        set => SetValue(key, value, false);
    }

    public bool ContainsKey(TKey key) => GetOrRemoveValue(key, out _, false);

    public void CopyTo(KeyValuePair<TKey, TValue>[] array, int arrayIndex)
    {
        foreach (var kvp in this)
        {
            array[arrayIndex++] = kvp;
        }
    }

    public void Add(TKey key, TValue value)
    {
        if (SetValue(key, value, true))
        {
            throw new ArgumentException("", nameof(key));
        }
    }

    public bool Remove(TKey key) => GetOrRemoveValue(key, out _, true);

    public bool TryGetValue(TKey key, out TValue value) => GetOrRemoveValue(key, out value, false);

    void ICollection<KeyValuePair<TKey, TValue>>.Add(KeyValuePair<TKey, TValue> kvp) => Add(kvp.Key, kvp.Value);

    bool ICollection<KeyValuePair<TKey, TValue>>.Contains(KeyValuePair<TKey, TValue> item)
    {
        if (!GetOrRemoveValue(item.Key, out var value, false))
        {
            return false;
        }

        var valueComparer = ValueComparer;
        if (default(TValue) == null)
        {
            // https://github.com/dotnet/coreclr/issues/17273
            valueComparer = valueComparer ?? EqualityComparer<TValue>.Default;
        }

        return valueComparer is null
            ? EqualityComparer<TValue>.Default.Equals(item.Value, value)
            : valueComparer.Equals(item.Value, value);
    }

    bool ICollection<KeyValuePair<TKey, TValue>>.Remove(KeyValuePair<TKey, TValue> item)
    {
        if (!TryGetValue(item.Key, out var value))
        {
            return false;
        }

        var valueComparer = ValueComparer;
        if (default(TValue) == null)
        {
            // https://github.com/dotnet/coreclr/issues/17273
            valueComparer = valueComparer ?? EqualityComparer<TValue>.Default;
        }

        if (valueComparer is null)
        {
            if (!EqualityComparer<TValue>.Default.Equals(item.Value, value))
            {
                return false;
            }
        }
        else
        {
            if (!valueComparer.Equals(item.Value, value))
            {
                return false;
            }
        }

        return GetOrRemoveValue(item.Key, out _, true);
    }

    bool ICollection<KeyValuePair<TKey, TValue>>.IsReadOnly => false;

    IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();

    public sealed class KeyCollection : ICollection<TKey>
    {
        internal readonly EasierDictionary<TKey, TValue> _dict;

        public KeyCollection(EasierDictionary<TKey, TValue> dict) => _dict = dict;

        public int Count => _dict.Count;

        public void CopyTo(TKey[] array, int arrayIndex)
        {
            foreach (var kvp in _dict)
            {
                array[arrayIndex++] = kvp.Key;
            }
        }

        public Enumerator GetEnumerator() => new Enumerator(_dict);

        bool ICollection<TKey>.IsReadOnly => true;
        IEnumerator<TKey> IEnumerable<TKey>.GetEnumerator() => GetEnumerator();
        IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();
        bool ICollection<TKey>.Contains(TKey item) => _dict.ContainsKey(item);
        void ICollection<TKey>.Add(TKey item) => throw new NotSupportedException("");
        void ICollection<TKey>.Clear() => throw new NotSupportedException("");
        bool ICollection<TKey>.Remove(TKey item) => throw new NotSupportedException("");

        public struct Enumerator : IEnumerator<TKey>
        {
            private readonly IEnumerator<KeyValuePair<TKey, TValue>> _enumerator;

            internal Enumerator(EasierDictionary<TKey, TValue> dict) => _enumerator = dict.GetEnumerator();

            public TKey Current => _enumerator.Current.Key;
            object IEnumerator.Current => Current;

            public void Dispose() => _enumerator.Dispose();
            public bool MoveNext() => _enumerator.MoveNext();
            public void Reset() => _enumerator.Reset();
        }
    }

    public sealed class ValueCollection : ICollection<TValue>
    {
        private readonly EasierDictionary<TKey, TValue> _dict;

        internal ValueCollection(EasierDictionary<TKey, TValue> dict) => _dict = dict;

        public int Count => _dict.Count;

        public void CopyTo(TValue[] array, int arrayIndex)
        {
            foreach (var kvp in _dict)
            {
                array[arrayIndex++] = kvp.Value;
            }
        }

        public Enumerator GetEnumerator() => new Enumerator(_dict);

        bool ICollection<TValue>.IsReadOnly => true;
        IEnumerator<TValue> IEnumerable<TValue>.GetEnumerator() => GetEnumerator();
        IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();

        bool ICollection<TValue>.Contains(TValue item)
        {
            var valueComparer = _dict.ValueComparer;

            if (default(TValue) == null)
            {
                // https://github.com/dotnet/coreclr/issues/17273
                valueComparer = valueComparer ?? EqualityComparer<TValue>.Default;
            }

            if (valueComparer is null)
            {
                foreach (var kvp in _dict)
                {
                    if (EqualityComparer<TValue>.Default.Equals(kvp.Value, item))
                    {
                        return true;
                    }
                }
            }
            else
            {
                foreach (var kvp in _dict)
                {
                    if (valueComparer.Equals(kvp.Value, item))
                    {
                        return true;
                    }
                }
            }

            return false;
        }

        void ICollection<TValue>.Add(TValue item) => throw new NotSupportedException("");
        void ICollection<TValue>.Clear() => throw new NotSupportedException("");
        bool ICollection<TValue>.Remove(TValue item) => throw new NotSupportedException("");

        public struct Enumerator : IEnumerator<TValue>
        {
            private readonly IEnumerator<KeyValuePair<TKey, TValue>> _enumerator;

            internal Enumerator(EasierDictionary<TKey, TValue> dict) => _enumerator = dict.GetEnumerator();

            public TValue Current => _enumerator.Current.Value;
            object IEnumerator.Current => Current;

            public void Dispose() => _enumerator.Dispose();
            public bool MoveNext() => _enumerator.MoveNext();
            public void Reset() => _enumerator.Reset();
        }
    }
}

@airbreather airbreather removed the question Further information is requested label Aug 23, 2019
@airbreather airbreather changed the title Why is AttributesTable even a separate thing? Redesign IFeature / IAttributesTable to better model the different use cases it solves Aug 23, 2019
@shmuelie
Copy link

I spent some time thinking on this, with the hardest part being "what is a feature". After reading too many OGC docs, I found that a "Feature" is defined as part of GML, section 9.

If we go by what GML says then a Feature is simply an object, with some optionally predefined properties and a geometry.

I almost feel like IFeature could just have a geometry and an IReadOnlyDictionary<string, object> for properties. For things that have predefined properties additional interfaces or just implementations would work. so something like:

interface IFeature
{
    Geometry Geometry { get; set; }
    IReadOnlyDictionary<string, object> Attributes { get; }
}

// For GPX for example
interface IWaypointFeature : IFeature
{
    int Elevation { get; set; }
    DateTimeOffset Time { get; set; }
    // ...
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Doing this would break either source or binary compatibility with existing versions.
Projects
None yet
Development

No branches or pull requests

3 participants