GeometryCollection.Empty.Count() == 1

[verdie-g]

GeometryCollection.Empty.Count() == 1

According to the documentation it is expected. I would be interested to know the rational, it seems counter-intuitive. https://github.com/NetTopologySuite/NetTopologySuite/blob/b58b842ad2506e97188fee804a3135935ceae7b8/src/NetTopologySuite/Geometries/GeometryCollectionEnumerator.cs#L6-L13

This behavior makes the below code, using NUnit, result in a stack overflow because it tries to format the element of the collection recursively.

Assert.IsNull(GeometryCollection.Empty);

[FObermaier]

This does not throw any exception:

[Test]
public void TestGeometryCollectionEmptyAccess()
{
    Geometry gcEmpty = null;
    Assert.That(()=> gcEmpty = GeometryCollection.Empty, Throws.Nothing);
    Assert.That(gcEmpty, Is.Not.Null);
}

[airbreather]

I think I first ran into the issue with a test like this:

var g1 = new WKTReader().Read("MULTIPOINT ((0 0))");
var g2 = new WKTReader().Read("MULTIPOINT ((0 0))");
Assert.That(g1, Is.EqualTo(g2));

[airbreather]

FWIW, there's a workaround, which we're already using in our own tests: https://github.com/NetTopologySuite/NetTopologySuite/blob/64a149b0ddeb525d90511d33959150c54e777d40/test/NetTopologySuite.Tests.NUnit/IO/GML3/GML3WriterTest.cs#L297-L307

[FObermaier]

Well, I don't know the reasons that led to this design decision in JTS.
Maybe @dr-jts can explain.

[dr-jts]

Well, I don't know the reasons that led to this design decision in JTS. Maybe @dr-jts can explain.

The only reason I can think of for this design decision is that it provides a certain uniformity, in that every Geometry object in the tree is returned by the iterator. So if you wanted to check that every object meets some condition (say, has a given SRID) the iterator lets you do that in a simple way.

Also, I think the intended use was really to access atomic elements in the geometry (which may be buried inside sub-collections). This makes the presence of the original geometry collection less of an issue, since the caller code ignores non-atomic geometries returned from the iterator.

Perhaps this wasn't a great decision. It seems more reasonable and less error-prone if the iterator did NOT return the original geometry, only the constituent elements.

There's no code in JTS that relies on this behaviour (and it is coded around in a couple of places). Perhaps it's possible to change this without too much impact? Or, users just need to be aware of this behaviour, and code around it appropriately.

[airbreather]

This seems like a less serious issue in JTS than in NTS. In JTS, you have to opt into using GeometryCollectionIterator directly, whereas in NTS, it happens automatically whenever you use GeometryCollection's implementation of IEnumerable<Geometry>.

[dr-jts]

Agreed. Perhaps NTS should use more appropriate behaviour in this case?

[FObermaier]

We could leave GeometryCollectionIterator as is and create a new enumerator that plainly returns the elements of the _geometries array. This is not breaking binary compatibility but the behaviour.
At least that way NumGeometries and Enumerable.Count() would be equal.

[airbreather]

@FObermaier I noticed your branch https://github.com/NetTopologySuite/NetTopologySuite/tree/gh-508. Much simpler would be to just change the implementation of GetEnumerator() to:

/// <inheritdoc />
public IEnumerator<Geometry> GetEnumerator()
{
    return ((IEnumerable<Geometry>)Geometries).GetEnumerator();
}