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
------------------------------------------------------------------------------
LogMeIn Central: Instant, anywhere, Remote PC access and management.
Stay in control, update software, and manage PCs from one command center
Diagnose problems and improve visibility into emerging IT issues
Automate, monitor and manage. Do more in less time with Central
http://p.sf.net/sfu/logmein12331_d2d
_______________________________________________
GeoTools-Devel mailing list
GeoTools-Devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geotools-devel
