Thanks for the review/feedback Andrea!
I am having a hard time clicking on your links and finding which line you are referring to - the page is too big (14 MB) and my browser gives up waiting. Saving the file locally may work ...
> took some time to go over the pull request at
> https://github.com/geotools/geotools/pull/44/files (it's the right
> one, yes?)
>
>
Yes, very much so.
> - Like that we're dropping the FeatureCollections "factory" methods
> - The example here is scary:
> https://github.com/geotools/geotools/pull/44/files#L2R66
> Basically I have to make a check with an if, and what if the
> returned value for some reason tomorrow
> is not a java.util.Collection? This is looking for trouble (who
> says the returned collection is immutable
> only because it does not implement java.util.Collection, it could
> have its own implementation specific
> modifiers methods, and btw, what would be the point of creating an
> immutable empty collection?
>
>
Micheal Bedward also commented that he was uncomfortable with the DefaultFeatureCollection being used directly in tutorials. We have both arrived at the same conclusion, our initial example collects information into a List<SimpleFeature> and then uses ListFeatureCollection.
> Imho the message should be straight and clear: collections are
> generally speaking read only
> access delegates to some storage subsystem, some implementations
> actually store data in memory
> and have mutator methods, if this is your use case please use
> either ListFeatureCollection (if you want
> it efficient and easy) or DefaultFeatureCollection (if you want
> guarantees that the feature ids are unique)
>
>
Bingo!
> - this one is a fair and square regression coding wise, besides for
> people using JDK7 that can use
> try with resources:
> https://github.com/geotools/geotools/pull/44/files#L2R127
> Pretty please, add a DataUtilities.closeQuietly(iterator)
> (commons-io like) so that the code in question does not have to
> be written over and over, and also advise people to use use
> FeatureIterator instead.
>
>
Yeah I like that a lot, also gives me a chance to make it null safe.
> - imho this form of commenting is not helping, people will probably
> won't notice it unless they are
> doing a line by line review of your patch like I'm doing:
> https://github.com/geotools/geotools/pull/44/files#L3R139
> Why not open a jira instead and assign it to someone in the app-schema team?
>
>
It was generally a response of horror :(
Here is an issue: https://jira.codehaus.org/browse/GEOT-4303
> - it seems that app-schema tests have a knack for repeating the same
> manual feature
> counting code, three times in three different tests (I guess it's
> done to compare the manual
> count with featureCollection.size(), but still... 3 times? No wait,
> I think I see a 4th as well...).
> Not Jody's fault anyways.
> - Unrelated to this work, but the code base has code around like this
> one that extracts
> the first feature from a collection, often to examine its contents,
> taking it as an exemplar:
> https://github.com/geotools/geotools/pull/44/files#L7R315
> I guess it would be nice to have a
> DataUtilities.firstFeature(FeatureCollection) method
> avoid us to repeat the job and having to handle try/finally all the time.
> I see this could also simplify testing code. A interesting companion could be
> List<Feature> DataUtilities.dump(FeatureCollection, maxFeatures)
>
>
We have DataUtilities.collection( … ) methods so perhaps DataUtilities.list( featureCollection, limit ) would be best.
> - leftover commented out code here:
> https://github.com/geotools/geotools/pull/44/files#L25R1133
> - I see bridge iterator is copied in two modules, may it be something
> that needs to
> be pushed back in main, maybe provided by a DataUtilities methods?
> https://github.com/geotools/geotools/pull/44/files#L31R151
> - I see sprinkled in the code a pattern like this:
> if(collection instanceof java.util.Collection) {
> // do something
> } else {
> thrown new Exception("Bwaa, how did you dare giving me something
> immutable!");
> }
>
> Wouldn't it be nicer and more consistent to have a
> DataUtilities.castToCollection(FeatureCollection)
> that does the test, throws the exception in case it's necessary, and
> returns a java.util.Collection?
> Less code, more consistency, and lower cyclomatic complexity
> (If one really wants, the method could be DataUtilties.toCollection
> with a flag stating wheter we
> want a mere cast or if a full copy into a memory collection is also
> an acceptable behavior)
> - why switch from Decorating to Base here? The two things have
> different meaning, a decorating
> base class helps deal with a delegate, a base class gives you only
> the few template
> methods needed without consideration for a possible delegate being around?
> Large change, not sure I understand it, hopefully Justin will chime in
>
>
When I looked at the class it was just setting up an iterator() for reuse, I found I was failing tests converting it features() while still using the same base class.
I created BaseFeatureCollection (provide a features() method) as an alternative to AbstractFeatureCollection (provide an iterator() method) so it seemed appropriate.
> - Err... uh?
> https://github.com/geotools/geotools/pull/44/files#L40R55
>
>
Yes see my email on this topic, it was also mentioned as an outstanding problem on the pull request.
I *did* catch up with Justin and he was not familiar with this code.
The original was incredibly messed up, returning Iterator<Attribute> (note this is not an Iterator<Feature>) and
was being incredibly luck to make it through the encoding test at all.
I could not see how to salvage it as written, thus the substantial rewrite to return a Iterator<Feature> consisting of a single Attribute.
> - Btw, I see you coupled in this one also a large DefaultQuery ->
> Query refactor,
> unrelated but good nonetheless, since we tend to also point people to our test
> cases as a source of inspiration
>
>
Yeah more of that is needed I thought the better of doing too much work as I went.
> - this is the winner for the most misleading name:
> https://github.com/geotools/geotools/pull/44/files#L65R96
> Don't know where it's using, but can't we just call it
> featureIdCount(Filter) ?
>
>
Sure, it is used to provide the expected number of features for that filter to return :-)
assertEquals( expected( filter ), featureSource.getFeatures( filter ).size() );
> Or at least "www", just as unrelated by at least Justin will like it
> better :-p
>
>
I don't get the joke :P
> - commented out code left in the code base:
> https://github.com/geotools/geotools/pull/44/files#L69R191
> - unrelated to this work, but what is a test base class doing inside src/main?
> FeatureCollectionTest, in the main module. Afaik only test classes in the
> main module are using it
> - commented out leftover:
> https://github.com/geotools/geotools/pull/44/files#L72R175
> - shouldn't this class be marked as deprecated just like RetypingIterator?
> https://github.com/geotools/geotools/pull/44/files#L85R44
>
>
I think it may still be used by AbstractFeatureCollection - which is iterator based().
> Err... at this point in the review my brain was half toast and fever started
> to catch up on me again, I haven't notice anything major left in main,
> but the changes in streamingrenderer do look a bit scary, just can't
> focus enough to review them now, will try later, here are some more things I could
> still glance:
You are now making me worried, what are you doing with a computer on and a fever?
> - https://github.com/geotools/geotools/pull/44/files#L140L111
> Why are these fields being removed?
>
>
They were not used. The question is why there were there at all…
> - changes in the versioning module... can't we just kill the module
> and be done with it?
>
>
Yes if I was smart I would of done that first, consider it the next pull request.
And how about shapefile-renderer?
> - again, please open a jira for this one (don't understand what you
> mean there, either):
> https://github.com/geotools/geotools/pull/44/files#L203R181
> - shapefile-renderer: another module that it's big time to kill
> (unless someone wants
> to step up and maintain it)
>
> Phew, that's all for now, some more sleeping for me. Bye
Um thanks - get sleep/better !
Jody
|