[
https://issues.apache.org/jira/browse/OFBIZ-11167?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17561935#comment-17561935
]
Gil Portenseigne commented on OFBIZ-11167:
------------------------------------------
I wrote a little script to extract the rules in order of occurrence. I will
launch an email to the dev list to discuss about the rules to be respected, and
the implied changes.
There are the results :
||Number of occurence||Rule name and details||
|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.|
|190|DuplicateNumberLiteral - Code containing number String literals can
usually be improved by declaring the number as a constant field. The
ignoreNumbers property (0,1) can optionally specify a comma-separated list of
numbers to ignore.|
|190|NoWildcardImports - Wildcard imports, static or otherwise, should not be
used.|
|173|TrailingComma - Check whether list and map literals contain optional
trailing comma.|
|167|InvertedCondition - An inverted condition is one where a constant
expression is used on the left hand side of the equals comparision. Such
conditions can be confusing especially when used in assertions where the
expected value is by convention placed on the right hand side of the
comparision.|
|167|JavadocEmptyReturnTag - Checks for empty @return tags within javadoc.|
|164|UnnecessarySetter - Checks for explicit calls to setter methods which can,
for the most part, be replaced by assignment to property. A setter is defined
as a method call that matches set[A-Z] but not set[A-Z][A-Z] such as setURL().
Setters take one method argument.|
|146|CouldBeElvis - Catch an if block that could be written as an elvis
expression.|
|142|UnusedImport - Imports for a class that is never referenced within the
source file is unnecessary.|
|141|AbcMetric - Checks the ABC size metric for methods/classes. A method (or
"closure field") with an ABC score greater than the maxMethodAbcScore property
(60) causes a violation. Likewise, a class that has an (average method) ABC
score greater than the maxClassAverageMethodAbcScore property (60) causes a
violation.|
|117|ConsecutiveBlankLines - Makes sure there are no consecutive lines that are
either blank or whitespace only.|
|104|SpaceAfterComma - Checks that there is at least one space or whitespace
following each comma. That includes checks for method and closure declaration
parameter lists, method call parameter lists, Map literals and List literals.|
|103|FactoryMethodName - A factory method is a method that creates objects, and
they are typically named either buildFoo(), makeFoo(), or createFoo(). This
rule enforces that only one naming convention is used. It defaults to
makeFoo(), but that can be changed using the property 'regex'.|
|102|ImplicitClosureParameter - Checks that the implicit it closure parameter
is not used and that parameters are declared explicitly instead.|
|91|VariableTypeRequired - Checks that variable types are explicitly specified
in declarations (and not using def)|
|85|InvertedIfElse - An inverted if-else statement is one in which there is a
single if statement with a single else branch and the boolean test of the if is
negated. For instance if (!x) false else true. It is usually clearer to write
this as if (x) true else false.|
|74|UnnecessarySemicolon - Semicolons as line terminators are not required in
Groovy: remove them. Do not use a semicolon as a replacement for empty braces
on for and while loops; this is a confusing practice.|
|69|SpaceBeforeOpeningBrace - Check that there is at least one space (blank) or
whitespace before each opening brace ("{") for method/class/interface
declarations, closure expressions and block statements.|
|68|SpaceAfterIf - Check that there is exactly one space (blank) after the if
keyword and before the opening parenthesis.|
|51|UnnecessaryGroovyImport - A Groovy file does not need to include an import
for classes from java.lang, java.util, java.io, java.net, groovy.lang and
groovy.util, as well as the classes java.math.BigDecimal and
java.math.BigInteger.|
|50|CyclomaticComplexity - Checks the cyclomatic complexity for
methods/classes.A method (or "closure field") with a cyclomatic complexity
value greater than the maxMethodComplexity property (20) causes a violation.
Likewise, a class that has an (average method) cyclomatic complexityvalue
greater than the maxClassAverageMethodComplexity property (20) causes a
violation.|
|46|BlockStartsWithBlankLine - Checks that code blocks such as method bodies,
closures and control structure bodies do not start with an empty line.|
|45|CatchException - Catching Exception is often too broad or general. It
should usually be restricted to framework or infrastructure code, rather than
application code.|
|44|FileEndsWithoutNewline - Makes sure the source code file ends with a
newline character.|
|41|ExplicitCallToCompareToMethod - This rule detects when the
compareTo(Object) method is called directly in code instead of using the <=>,
>, >=, <, and <= operators. A groovier way to express this: a.compareTo(b) is
this: a <=> b, or using the other operators.|
|41|ParameterReassignment - Checks for a method or closure parameter being
reassigned to a new value within the body of the method/closure, which is a
confusing and questionable practice. Use a temporary variable instead.|
|34|ExplicitCallToMultiplyMethod - This rule detects when the minus(Object)
method is called directly in code instead of using the * operator. A groovier
way to express this: a.multiply(b) is this: a * b|
|34|UnusedVariable - Checks for variables that are never referenced. The
ignoreVariableNames property (null) specifies one or more variable names that
should be ignored, optionally containing wildcard characters ('*' or '?').|
|33|DuplicateMapLiteral - Code containing duplicate Map literals can usually be
improved by declaring the Map as a constant field.|
|30|UnnecessaryPackageReference - Checks for explicit package reference for
classes that Groovy imports by default, such as java.lang.String, java.util.Map
and groovy.lang.Closure.|
|28|JavaIoPackageAccess - This rule reports violations of the Enterprise
JavaBeans specification by using the java.io package to access files or the
file system.|
|27|BlockEndsWithBlankLine - Checks that code blocks such as method bodies,
closures and control structure bodies do not end with an empty line.|
|27|NestedForLoop - Reports classes with nested for loops.|
|25|MethodParameterTypeRequired - Checks that method parameters are not
dynamically typed, that is they are explicitly stated and different than def.|
|24|UnnecessaryElseStatement - When an if statement block ends with a return
statement the else is unnecessary. The logic in the else branch can be run
without being in a new scope.|
|23|UnnecessaryParenthesesForMethodCallWithClosure - If a method is called and
the only parameter to that method is an inline closure then the parentheses of
the method call can be omitted.|
|21|Instanceof - Checks for use of the instanceof operator. Use the
ignoreTypeNames property to configure ignored type names.|
|18|MethodSize - Checks if the size of a method exceeds the number of lines
specified by the maxLines property (100).|
|16|NoJavaUtilDate - Do not use java.util.Date. Prefer the classes in the
java.time.* packages.|
|14|ConfusingTernary - In a ternary expression avoid negation in the test. For
example, rephrase: "(x != y) ? diff : same" as: "(x == y) ? same : diff".
Consistent use of this rule makes the code easier to read. Also, this resolves
trivial ordering problems, such as "does the error case go first?" or "does the
common case go first?".|
|13|JavadocMissingParamDescription - Checks for missing description within
@param javadoc tags.|
|11|UnnecessaryCast - Checks for unnecessary cast operations|
|10|LongLiteralWithLowerCaseL - In Java and Groovy, you can specify long
literals with the L or l character, for instance 55L or 24l. It is best
practice to always use an uppercase L and never a lowercase l. This is because
11l rendered in some fonts may look like 111 instead of 11L.|
|9|DuplicateListLiteral - Code containing duplicate List literals can usually
be improved by declaring the List as a constant field.|
|9|ExplicitArrayListInstantiation - This rule checks for the explicit
instantiation of an ArrayList using the no-arg constructor. In Groovy, it is
best to write new ArrayList() as [], which creates the same object.|
|9|NoDouble - Checks for use of the double or Double types, in fields,
variables, method parameters and method return types.|
|8|AddEmptyString - Finds empty string literals which are being added. This is
an inefficient way to convert any type to a String.|
|8|SpaceAfterOpeningBrace - Check that there is at least one space (blank) or
whitespace after each opening brace (" \{") for method/class/interface
declarations, closure expressions and block statements. \| \| 7 \|
CouldBeSwitchStatement - Checks for multiple if statements that could be
converted to a switch \| \| 7 \| UnnecessaryBooleanInstantiation - Use
Boolean.valueOf() for variable values or Boolean.TRUE and Boolean.FALSE for
constant values instead of calling the Boolean() constructor directly or
calling Boolean.valueOf(true) or Boolean.valueOf(false). \| \| 6 \|
MissingBlankLineAfterImports - Makes sure there is a blank line after the
imports of a source code file. \| \| 6 \| ParameterCount - Checks if the number
of parameters in method/constructor exceeds the number of parameters specified
by the maxParameters property. \| \| 6 \| ReturnNullFromCatchBlock - Returning
null from a catch block often masks errors and requires the client to handle
error codes. In some coding styles this is discouraged. \| \| 6 \|
SpaceAfterCatch - Check that there is exactly one space (blank) after the catch
keyword and before the opening parenthesis. \| \| 6 \| SpaceAfterClosingBrace -
Check that there is at least one space (blank) or whitespace after each closing
brace ("}") for method/class/interface declarations, closure expressions and
block statements.|
|5|ExplicitHashMapInstantiation - This rule checks for the explicit
instantiation of a HashMap using the no-arg constructor. In Groovy, it is best
to write new HashMap() as [:], which creates the same object.|
|5|PublicMethodsBeforeNonPublicMethods - Enforce that all public methods are
above protected and private methods.|
|5|SpaceAfterFor - Check that there is exactly one space (blank) after the for
keyword and before the opening parenthesis.|
|4|NoTabCharacter - Checks that all source files do not contain the tab
character|
|4|SimpleDateFormatMissingLocale - Be sure to specify a Locale when creating a
new instance of SimpleDateFormat; the class is locale-sensitive. If you
instantiate SimpleDateFormat without a Locale parameter, it will format the
date and time according to the default Locale. Both the pattern and the Locale
determine the format. For the same pattern, SimpleDateFormat may format a date
and time differently if the Locale varies.|
|4|SpaceBeforeClosingBrace - Check that there is at least one space (blank) or
whitespace before each closing brace ("}") for method/class/interface
declarations, closure expressions and block statements.|
|3|DuplicateImport - Duplicate import statements are unnecessary.|
|3|GStringAsMapKey - A GString should not be used as a map key since its
hashcode is not guaranteed to be stable. Consider calling key.toString().|
|3|SpaceAroundClosureArrow - Checks that there is whitespace around the closure
arrow (->) symbol|
|3|TernaryCouldBeElvis - Checks for ternary expressions where the boolean and
true expressions are the same. These can be simplified to an Elvis expression.|
|3|UnnecessaryPublicModifier - The 'public' modifier is not required on methods
or classes.|
|2|ExplicitHashSetInstantiation - This rule checks for the explicit
instantiation of a HashSet using the no-arg constructor. In Groovy, it is best
to write new HashSet() as [] as Set, which creates the same object.|
|2|ExplicitLinkedListInstantiation - This rule checks for the explicit
instantiation of a LinkedList using the no-arg constructor. In Groovy, it is
best to write new LinkedList() as [] as Queue, which creates the same object.|
|2|ExplicitTreeSetInstantiation - This rule checks for the explicit
instantiation of a TreeSet using the no-arg constructor. In Groovy, it is best
to write new TreeSet() as [] as SortedSet, which creates the same object.|
|2|IfStatementCouldBeTernary - Checks for if statements where both the if and
else blocks contain only a single return statement with a constant or literal
value|
|2|JavadocEmptyFirstLine - Check for javadoc comments with an empty top line.|
|2|SpaceAfterWhile - Check that there is exactly one space (blank) after the
while keyword and before the opening parenthesis.|
|2|UnnecessaryCallForLastElement - This rule checks for excessively verbose
methods of accessing the last element of an array or list. For instance, it is
possible to access the last element of an array by performing
array[array.length - 1], in Groovy it is simpler to either call array.last() or
array[-1]. The same is true for lists. This violation is triggered whenever a
get, getAt, or array-style access is used with an object size check.|
|2|UnnecessaryDotClass - To make a reference to a class, it is unnecessary to
specify the '.class' identifier. For instance String.class can be shortened to
String.|
|2|VariableName - Verifies that the name of each method matches a regular
expression. By default it checks that non-'final' variable names start with a
lowercase letter and contains only letters or numbers, and 'final' variable
names start with an uppercase letter and contain only uppercase letters,
numbers and underscores. The regex property specifies the default regular
expression used to validate a non-'final' variable name. The finalRegex
property specifies the regular expression used to validate 'final' variable
names. The ignoreVariableNames property (null) can specify variable names that
should be ignored, optionally containing wildcard characters ('*' or '?').|
|1|BracesForIfElse - Checks the location of the opening brace ({) for if
statements. By default, requires them on the same line, but the sameLine
property can be set to false to override this.|
|1|CatchNullPointerException - Catching NullPointerException is never
appropriate. It should be avoided in the first place with proper null checking,
and it can mask underlying errors.|
|1|ClassEndsWithBlankLine - Check whether the class ends with a blank line.By
default, it enforces that there must be a blank line before the closing class
brace, except if the class is empty and is written in a single line. A blank
line is defined as any line that does not contain any visible characters.|
|1|ClassName - Verifies that the name of a class matches a regular expression.
By default it checks that the class name starts with an uppercase letter and is
followed by zero or more word characters (letters, numbers or underscores). The
regex property specifies the regular expression used to validate the class
name.|
|1|ClassNameSameAsFilename - Reports files containing only one top level class
/ enum / interface which is named differently than the file.|
|1|ClassStartsWithBlankLine - Check whether the class starts with a blank line
By default, it enforces that there must be a blank line after the opening class
brace, except if the class is empty and is written in a single line. A blank
line is defined as any line that does not contain any visible characters.|
|1|ExplicitCallToPlusMethod - This rule detects when the plus(Object) method is
called directly in code instead of using the + operator. A groovier way to
express this: a.plus(b) is this: a + b|
|1|GStringExpressionWithinString - Check for regular (single quote) strings
containing a GString-type expression (${...}).|
|1|ImplementationAsType - Checks for use of a predefined set of concrete
classes (e.g. ArrayList, Hashtable, ConcurrentHashMap) when specifying the type
of a method parameter, closure parameter, constructor parameter, method return
type or field type. The associated interfaces should be used to specify the
type instead.|
|1|InsecureRandom - Reports usages of java.util.Random, which can produce very
predictable results. If two instances of Random are created with the same seed
and sequence of method calls, they will generate the exact same results. Use
java.security.SecureRandom instead, which provides a cryptographically strong
random number generator. SecureRandom uses PRNG, which means they are using a
deterministic algorithm to produce a pseudo-random number from a true random
seed. SecureRandom produces non-deterministic output.|
|1|JavadocEmptyLastLine - Check for javadoc comments with an empty line at the
bottom.|
|1|JdbcResultSetReference - Check for direct use of java.sql.ResultSet, which
is not necessary if using the Groovy Sql facility or an ORM framework such as
Hibernate.|
|1|MethodName - Verifies that the name of each method matches a regular
expression. By default it checks that the method name starts with a lowercase
letter. The regex property specifies the regular expression to check the method
name against. The ignoreMethodNames property (null) can specify method names
that should be ignored, optionally containing wildcard characters ('*' or '?').|
|1|MissingBlankLineAfterPackage - Makes sure there is a blank line after the
package statement of a source code file.|
|1|NoFloat - Checks for use of the float or Float types, in fields, variables,
method parameters and method return types.|
|1|ParameterName - Verifies that the name of each parameter matches a regular
expression. This rule applies to method parameters, constructor parameters and
closure parameters. By default it checks that parameter names start with a
lowercase letter and contains only letters or numbers. The regex property
specifies the default regular expression used to validate the parameter name.
The ignoreParameterNames property (null) can specify parameter names that
should be ignored, optionally containing wildcard characters ('*' or '?').|
|1|ReturnsNullInsteadOfEmptyCollection - Consider returning a zero length
collection rather than null. It is often a better design to return a length
zero collection rather than a null reference to indicate that there are no
results (i.e., an empty list of results). This way, no explicit check for null
is needed by clients of the method.|
|1|UnnecessaryBigDecimalInstantiation - It is unnecessary to instantiate
BigDecimal objects. Instead just use the decimal literal or the 'G' identifier
to force the type, such as 123.45 or 123.45G.|
|1|UnnecessaryBooleanExpression - Checks for unnecessary boolean expressions,
including ANDing (&&) or ORing (\|\|) with true, false, null, or a
Map/List/String/Number literal. Also checks for negation (!) of true, false,
null, or a Map/List/String/Number literal.|
|1|UnnecessaryCollectCall - Some method calls to Object.collect(Closure) can be
replaced with the spread operator. For instance, list.collect \{ it.multiply(2)
}can be replaced by list*.multiply(2). Warning: if a collection is null,
collect will return an empty list, while *. will return null.|
|1|UnnecessaryDefInMethodDeclaration - If a method has a visibility modifier or
a type declaration, then the def keyword is unneeded. For instance 'def private
method() {}' is redundant and can be simplified to 'private method() {}'.|
|1|UnnecessaryLongInstantiation - It is unnecessary to instantiate Long
objects. Instead just use the literal with the 'L' identifier to force the
type, such as 8L or 42L.|
|1|UnnecessaryToString - Checks for unnecessary calls to toString().|
> Use Codenarc to test Groovy code
> --------------------------------
>
> Key: OFBIZ-11167
> URL: https://issues.apache.org/jira/browse/OFBIZ-11167
> Project: OFBiz
> Issue Type: New Feature
> Components: framework
> Reporter: Jacques Le Roux
> Assignee: Gil Portenseigne
> Priority: Minor
> Attachments: OFBIZ-11167.patch, main.html, test.html
>
>
> Now that we use Groovy more and more, I think we should really have a look a
> Codenarc
> https://docs.gradle.org/current/userguide/codenarc_plugin.html
> We already discussed it at https://markmail.org/message/uigcpnxqgizhd2oi and
> https://markmail.org/message/rp6njoiohkkiodbe
> We know it's a crucial task but not an easy but rather a long term one
> Here are some interesting links (before I delete my FF tabs group about it)
> http://codenarc.sourceforge.net/codenarc-other-tools-frameworks.html
> http://codenarc.sourceforge.net/codenarc-creating-ruleset.html
> https://github.com/gradle/gradle/tree/master/config
> https://stackoverflow.com/questions/14358471/how-to-generate-codenarc-report-for-main-and-test-classes-using-different-rule-s
> https://mrhaki.blogspot.com/2011/01/gradle-goodness-use-groovy-ruleset-file.html
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
