Hey all, sorry for cross-posting but this relates to both projects. One of the PMD rules I'd like to set up the most is ExcessiveMEthodLength. It'll just fail on a threshold for lines of code in a single method.
I've made some assessments for 500 and 100 lines of code limits, check this out: GeoTools: https://github.com/groldan/geoserver/wiki/ExcessiveMethodLength-PMD-assessment---GeoTools GeoServer: https://github.com/groldan/geoserver/wiki/ExcessiveMethodLength-PMD-assessment---GeoServer I think avoiding too large methods is important for maintainability and to lower the barrier entry to the project for other people, because trying to keep methods concise forces you to think of the design and to split them into smaller methods. Ideally, a single method would be succinct and easy to read, for anyone to be able to figure out what it does at a given level of abstraction, and having to drill down to a lower level of abstraction method only if necessary. I myself often feel overwhelmed when facing such methods, despite having so many years of experience on the codebase, it's discouraging. So maybe we could do something at least in identifying the most relevant ones and trying to make them cleaner. Of course some of them would not be a problem cause they're just, either generated or not, linear setups of state, like org.geotools.gml3.GMLSchema:GMLSchema(), which is 978 lines long. But mostly there's a lot of code that would benefit from a second round of thinking. As an example, I tried to tidy up org.geotools.coverage.processing.operation.Resampler2D:reproject(...), which is currently 618 lines long, and made it 49. Most important thing I think is it now reads as intended, instead of having to have so many comments explaining what the following hundred lines of code do. This is the original: https://github.com/geotools/geotools/blob/64e0026c62cf37662194b92e8052594d57ef826e/modules/library/coverage/src/main/java/org/geotools/coverage/processing/operation/Resampler2D.java#L245-L863 And this the refactored: https://github.com/groldan/geotools/blob/d25f6424f8f4188824c840f400c3414a9ce12531/modules/library/coverage/src/main/java/org/geotools/coverage/processing/operation/Resampler2D.java#L302-L351 Not saying it's perfect, that shared catch-all state class is debatable, but I can certainly get an idea of what the method does at a glimpse and follow through if needed. I know it's something that'd require quite significant and lacking resources, but also think it's worth pursuing. Probably a first round to set a soft limit of 200 lines long or so, with a longer term goal of 100? Looking forward to your comments, Cheers. -- Gabriel Roldán
_______________________________________________ GeoTools-Devel mailing list GeoTools-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geotools-devel
