+1 in general from me although some of those rules might be challenging to resolve. For example DuplicateStringLiteral could be referring to an entity name being used in delegator queries more than once, I don't think we'd prefer to see that extracted to a constant.
SpaceAroundMapEntryColon I'm not so sure about, my preference for legibility has always been [someKey: someValue]. I find [someKey:someValue] a bit too condensed and harder to differentiate key from value. Regards Scott On Mon, 4 Jul 2022 at 16:25, Gil Portenseigne <gil.portensei...@nereide.fr> wrote: > Hello Devs, > > I would like to continue Codenarc integration in OFBiz (OFBIZ-11167). > > For those who are not aware, Codenarc is an analysis tools for Groovy > for defects, bad practices, inconsistencies, style issues and more. > > For this purpose we need to agree about the ruleset to put in place. > > I took as a basis the ruleset : > https://codenarc.org/StarterRuleSet-AllRulesByCategory.groovy.txt > > And started to fix some in > https://github.com/apache/ofbiz-framework/pull/517 > > Before doing the complete work, a first rule made me wonder if that is a > choice that we will be doing as a community : > > IfStatementBraces - Use braces for if statements, even for a single > statement. > > There are 234 occurrences in the project, and I guess that some opinions > might diverge on this subject. > > Moreover, some rules needs lots of fixes in the project (those with more > than 200 occurrences) : > > > +------------+---------------------------------------------------------------------+ > | Number of | Rule name and details > | > | occurrence | > | > > +============+=====================================================================+ > | 9883 | UnnecessaryGString - String objects should be created with > single | > | | quotes, and GString objects created with double quotes. > Creating | > | | normal String objects with double quotes is confusing to > readers. | > > +------------+---------------------------------------------------------------------+ > | 4569 | DuplicateStringLiteral - Code containing duplicate String > literals | > | | can usually be improved by declaring the String as a > constant | > | | field. The ignoreStrings property () can optionally > specify a | > | | comma-separated list of Strings to ignore. > | > > +------------+---------------------------------------------------------------------+ > | 4209 | SpaceAroundMapEntryColon - Check for configured formatting > of | > | | whitespace around colons for literal Map entries. > | > | | The characterBeforeColonRegex and > characterAfterColonRegex | > | | properties specify a regular expression that must match > the | > | | character before/after the colon. > | > > +------------+---------------------------------------------------------------------+ > | 1448 | LineLength - Checks the maximum length for each line of > | > | | source code. It checks for number of characters, so lines > | > | | that include tabs may appear longer than the allowed > number | > | | when viewing the file. The maximum line length can be > | > | | configured by setting the length property, which defaults > to 120. | > > +------------+---------------------------------------------------------------------+ > | 885 | UnnecessaryGetter - Checks for explicit calls to > getter/accessor | > | | methods which can, for the most part, be replaced by > property | > | | access. A getter is defined as a method call that > matches | > | | get[A-Z] but not getClass() or get[A-Z][A-Z] such as > getURL(). | > | | Getters do not take method arguments. The > ignoreMethodNames | > | | property (null) can specify method names that should be > ignored | > | | , optionally containing wildcard characters ('*' or '?'). > | > > +------------+---------------------------------------------------------------------+ > | 601 | NoDef - def should not be used. You should replace it with > | > | | concrete type. > | > > +------------+---------------------------------------------------------------------+ > | 485 | MethodReturnTypeRequired - Checks that method return > types | > | | are not dynamic, that is they are explicitly stated and > | > | | different than def. > | > > +------------+---------------------------------------------------------------------+ > | 484 | Indentation - Check indentation for class and method > | > | | declarations, and initial statements. > | > > +------------+---------------------------------------------------------------------+ > | 482 | UnnecessaryReturnKeyword - In Groovy, the return keyword > | > | | is often optional. If a statement is the last line in a > | > | | method or closure then you do not need to have the return > keyword. | > > +------------+---------------------------------------------------------------------+ > | 407 | UnnecessaryObjectReferences - Violations are triggered > when | > | | an excessive set of consecutive statements all reference > | > | | the same variable. This can be made more readable by > using | > | | a with or identity block. > | > > +------------+---------------------------------------------------------------------+ > | 345 | CompileStatic - Check that classes are explicitely > annotated | > | | with either @GrailsCompileStatic, @CompileStatic or > @CompileDynamic | > > +------------+---------------------------------------------------------------------+ > | 320 | ExplicitCallToEqualsMethod - This rule detects when the > | > | | equals(Object) method is called directly in code instead > | > | | of using the == or != operator. A groovier way to > | > | | express this: a.equals(b) is this: a == b and a groovier > | > | | way to express : !a.equals(b) is : a != b > | > > +------------+---------------------------------------------------------------------+ > | 235 | IfStatementBraces - Use braces for if statements, > | > | | even for a single statement. > | > > +------------+---------------------------------------------------------------------+ > | 235 | SpaceAroundOperator - Check that there is at least one > space | > | | (blank) or whitespace around each binary operator. > | > > +------------+---------------------------------------------------------------------+ > | 211 | NestedBlockDepth - Checks for blocks or closures nested > more | > | | than maxNestedBlockDepth (5) levels deep. > | > > +------------+---------------------------------------------------------------------+ > | 201 | TrailingWhitespace - Checks that no lines of source code > | > | | end with whitespace characters. > | > > +------------+---------------------------------------------------------------------+ > > I think that if someone want to express an opposition about applying one > of the above rule, i would be great to express it and discuss it here. > > My opinion is that it could be nice to adopt every rules, to respect > standard best practice. Even if it implies lot of code changes. > > Thanks in advance for your feedback. > > Regards, > > Gil >
