I think ExcessiveMethodLength is an important check for the readability and maintainability for code. Is the opposite check also available? Something that would flag too much abstraction and/or too much inheritance or too many interfaces, leading to confusing, unreadable empty methods and classes? In my experience both projects also have a fair amount of that problem.
Joe Miller On Wed, Mar 10, 2021 at 9:12 AM Gabriel Roldan <gabriel.rol...@gmail.com> wrote: > 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 >
_______________________________________________ GeoTools-Devel mailing list GeoTools-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geotools-devel
