Hi Gabriel, I would love to see having a limit on the method length... but remember when running these QAs you're pretty much alone (as I've been for most of the QA work, even when other people participated, I ended up doing well over 90% of the job).
I would suggest you start with a very high limit, get it fixed, and then you have a limit that PMD can apply, even if stupidly high. Then it can go down little by little, until you exhaust the time you want to dedicate on it. These days I won't be able to do much, with full lockdown in place and kids at home, it's a lot if I can run some cleanups where IntelliJ does most of the work for me :-D (which is, what I've been doing for the past month or two). Cheers Andrea On Wed, Mar 10, 2021 at 3:13 PM 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 > -- Regards, Andrea Aime == GeoServer Professional Services from the experts! Visit http://goo.gl/it488V for more information. == Ing. Andrea Aime @geowolf Technical Lead GeoSolutions S.A.S. Via di Montramito 3/A 55054 Massarosa (LU) phone: +39 0584 962313 fax: +39 0584 1660272 mob: +39 339 8844549 http://www.geo-solutions.it http://twitter.com/geosolutions_it ------------------------------------------------------- *Con riferimento alla normativa sul trattamento dei dati personali (Reg. UE 2016/679 - Regolamento generale sulla protezione dei dati “GDPR”), si precisa che ogni circostanza inerente alla presente email (il suo contenuto, gli eventuali allegati, etc.) è un dato la cui conoscenza è riservata al/i solo/i destinatario/i indicati dallo scrivente. Se il messaggio Le è giunto per errore, è tenuta/o a cancellarlo, ogni altra operazione è illecita. Le sarei comunque grato se potesse darmene notizia. This email is intended only for the person or entity to which it is addressed and may contain information that is privileged, confidential or otherwise protected from disclosure. We remind that - as provided by European Regulation 2016/679 “GDPR” - copying, dissemination or use of this e-mail or the information herein by anyone other than the intended recipient is prohibited. If you have received this email by mistake, please notify us immediately by telephone or e-mail.*
_______________________________________________ GeoTools-Devel mailing list GeoTools-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geotools-devel
