+1 This can be a great addition to the project. Thank you, Gil.
-- Kind Regards, Ashish Vijaywargiya Vice President of Operations *HotWax Systems* *Enterprise open source experts* http://www.hotwaxsystems.com On Mon, Jul 4, 2022 at 8:54 PM 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 >
