Hey Andrea, On Wed, 10 Mar 2021 at 12:02, Andrea Aime <andrea.a...@geo-solutions.it> wrote:
> 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). > Understood. No rush though, we can consider most of this 15+ years old technical debt. > > 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. > Yeah, 500 sounded like a stupidly high limit, sounds like a good one to start with, with 4 offenders per project. > > 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). > Don't worry, and thanks for the interest. I know you've worked hard to get us where we're. As said, no rush, and let's see if others feel like contributing. Cheers, Gabriel. > > 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.* > -- Gabriel Roldán
_______________________________________________ GeoTools-Devel mailing list GeoTools-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geotools-devel
