Hey Jody, took some time to go over the pull request at https://github.com/geotools/geotools/pull/44/files (it's the right one, yes?)
- 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? 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) - 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. - 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 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) - 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 - Err... uh? https://github.com/geotools/geotools/pull/44/files#L40R55 - 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 - 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) ? Or at least "www", just as unrelated by at least Justin will like it better :-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 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: - https://github.com/geotools/geotools/pull/44/files#L140L111 Why are these fields being removed? - changes in the versioning module... can't we just kill the module and be done with it? - 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 Cheers Andrea -- == Our support, Your Success! Visit http://opensdi.geo-solutions.it for more information. == Ing. Andrea Aime @geowolf Technical Lead GeoSolutions S.A.S. Via Poggio alle Viti 1187 55054 Massarosa (LU) Italy phone: +39 0584 962313 fax: +39 0584 1660272 mob: +39 339 8844549 http://www.geo-solutions.it http://twitter.com/geosolutions_it ------------------------------------------------------- ------------------------------------------------------------------------------ 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
