From 0f22ed337c8b206ececb7a37d495e6824b7289a8 Mon Sep 17 00:00:00 2001 From: Philippe Marschall Date: Sat, 11 Feb 2017 15:15:18 +0100 Subject: [PATCH] Issue #49: Reduce rule violations Fix various rule violations reported by Teamcity. --- .../checkstyle/CheckstyleProfileImporter.java | 6 +-- ...le.checks.coding.EqualsAvoidNullCheck.html | 7 +--- ...checkstyle.checks.coding.NoCloneCheck.html | 14 +++---- ....checkstyle.checks.header.HeaderCheck.html | 10 ++--- ...style.checks.header.RegexpHeaderCheck.html | 42 +++++++++---------- ...yle.checks.imports.ImportControlCheck.html | 4 +- ...ecks.whitespace.FileTabCharacterCheck.html | 3 +- .../CheckstyleAuditListenerTest.java | 12 +++--- .../CheckstyleConfigurationTest.java | 4 +- .../checkstyle/CheckstyleExecutorTest.java | 2 +- .../checkstyle/CheckstyleSensorTest.java | 8 ++-- .../checkstyle/CheckstyleTestUtils.java | 6 +-- .../checkstyle/internal/CheckUtil.java | 5 +-- .../checkstyle/internal/ChecksTest.java | 5 +-- 14 files changed, 59 insertions(+), 69 deletions(-) diff --git a/checkstyle-sonar-plugin/src/main/java/org/sonar/plugins/checkstyle/CheckstyleProfileImporter.java b/checkstyle-sonar-plugin/src/main/java/org/sonar/plugins/checkstyle/CheckstyleProfileImporter.java index 894ad43d..cf7e1a4a 100644 --- a/checkstyle-sonar-plugin/src/main/java/org/sonar/plugins/checkstyle/CheckstyleProfileImporter.java +++ b/checkstyle-sonar-plugin/src/main/java/org/sonar/plugins/checkstyle/CheckstyleProfileImporter.java @@ -50,7 +50,7 @@ public class CheckstyleProfileImporter extends ProfileImporter { private static final String CHECKER_MODULE = "Checker"; private static final String TREEWALKER_MODULE = "TreeWalker"; private static final String MODULE_NODE = "module"; - private static final String[] FILTERS = new String[] { + private static final String[] FILTERS = { "SeverityMatchFilter", "SuppressionFilter", "SuppressWarningsFilter", @@ -61,8 +61,8 @@ public class CheckstyleProfileImporter extends ProfileImporter { private static class Module { private String name; - private Map properties = Maps.newHashMap(); - private List modules = Lists.newArrayList(); + private final Map properties = Maps.newHashMap(); + private final List modules = Lists.newArrayList(); } public CheckstyleProfileImporter(RuleFinder ruleFinder) { diff --git a/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.coding.EqualsAvoidNullCheck.html b/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.coding.EqualsAvoidNullCheck.html index f61d77f5..5d18fe90 100644 --- a/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.coding.EqualsAvoidNullCheck.html +++ b/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.coding.EqualsAvoidNullCheck.html @@ -1,19 +1,16 @@

Checks that any combination of String literals with optional assignment is on the left side of an equals() comparison.

Rationale: Calling the equals() method on String literals will avoid a potential NullPointerException. Also, it is pretty common to see null check right before equals comparisons which is not necessary in the below example.

-

For example: +

For example:

   String nullString = null;
   nullString.equals("My_Sweet_String");
-

-

-should be refactored to: +

should be refactored to:

   String nullString = null;
   "My_Sweet_String".equals(nullString);
-

Limitations: If the equals method is overridden or a covariant equals method is defined and the implementation is incorrect (where s.equals(t) does not return the same result as t.equals(s)) then rearranging the called on object and parameter may have unexpected results.

Java's Autoboxing feature has an affect on how this check is implemented. Pre Java 5 all IDENT + IDENT object concatenations would not cause a NullPointerException even if null. Those situations could have been included in this check. They would simply act as if they surrounded by String.valueof() which would concatenate the String null.

The following example will cause a NullPointerException as a result of what autoboxing does.

diff --git a/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.coding.NoCloneCheck.html b/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.coding.NoCloneCheck.html index 906be8cf..7390a0cc 100644 --- a/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.coding.NoCloneCheck.html +++ b/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.coding.NoCloneCheck.html @@ -1,6 +1,6 @@

Checks that the clone method is not overridden from the Object class.

-

Rationale: The clone method relies on strange/hard to follow rules that do not work it all situations. Consequently, it is difficult to override correctly. Below are some of the rules/reasons why the clone method should be avoided. +

Rationale: The clone method relies on strange/hard to follow rules that do not work it all situations. Consequently, it is difficult to override correctly. Below are some of the rules/reasons why the clone method should be avoided.

  • Classes supporting the clone method should implement the Cloneable interface but the Cloneable interface does not include the clone method. As a result, it doesn't enforce the method override.
  • The Cloneable interface forces the Object's clone method to work correctly. Without implementing it, the Object's clone method will throw a CloneNotSupportedException.
    • @@ -10,27 +10,25 @@
  • If a class has references to mutable objects then those object references must be replaced with copies in the clone method after calling super.clone().
  • The clone method does not work correctly with final mutable object references because final references cannot be reassigned.
  • If a super class overrides the clone method then all subclasses must provide a correct clone implementation.
    • -

+

Two alternatives to the clone method, in some cases, is a copy constructor or a static factory method to return copies of an object. Both of these approaches are simpler and do not conflict with final fields. The do not force the calling client to handle a CloneNotSuportException. They also are typed therefore no casting is necessary. Finally, they are more flexible since they can take interface types rather than concrete classes.

Sometimes a copy constructor or static factory is not an acceptable alternative to the clone method. The example below highlights the limitation of a copy constructor (or static factory). Assume Square is a subclass for Shape.

-

   Shape s1 = new Square();
   System.out.println(s1 instanceof Square); //true
-

+

...assume at this point the code knows nothing of s1 being a Square that's the beauty of polymorphism but the code wants to copy the Square which is declared as a Shape, its super type...

-

   Shape s2 = new Shape(s1); //using the copy constructor
   System.out.println(s2 instanceof Square); //false
-

+ -

The working solution (without knowing about all subclasses and doing many casts) is to do the following (assuming correct clone implementation).
+

The working solution (without knowing about all subclasses and doing many casts) is to do the following (assuming correct clone implementation).

   Shape s2 = s1.clone();
   System.out.println(s2 instanceof Square); //true
-

+

Just keep in mind if this type of polymorphic cloning is required then a properly implemented clone method may be the best choice.

diff --git a/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.header.HeaderCheck.html b/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.header.HeaderCheck.html index f2dc2f11..6a0d331f 100644 --- a/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.header.HeaderCheck.html +++ b/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.header.HeaderCheck.html @@ -1,10 +1,10 @@

Checks that a source file begins with a specified header. Property headerFile specifies a file that contains the required header. Alternatively, the header specification can be set directly in the header property without the need for an external file.

Property ignoreLines specifies the line numbers to ignore when matching lines in a header file. This property is very useful for supporting headers that contain copyright dates. For example, consider the following header:

-	line 1: ////////////////////////////////////////////////////////////////////
-	line 2: // checkstyle:
-	line 3: // Checks Java source code for adherence to a set of rules.
-	line 4: // Copyright (C) 2002  Oliver Burn
-	line 5: ////////////////////////////////////////////////////////////////////
+    line 1: ////////////////////////////////////////////////////////////////////
+    line 2: // checkstyle:
+    line 3: // Checks Java source code for adherence to a set of rules.
+    line 4: // Copyright (C) 2002  Oliver Burn
+    line 5: ////////////////////////////////////////////////////////////////////

Since the year information will change over time, you can tell Checkstyle to ignore line 4 by setting property ignoreLines to 4.

\ No newline at end of file diff --git a/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.header.RegexpHeaderCheck.html b/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.header.RegexpHeaderCheck.html index e725361a..d97b42db 100644 --- a/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.header.RegexpHeaderCheck.html +++ b/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.header.RegexpHeaderCheck.html @@ -1,30 +1,30 @@

Checks the header of a source file against a header that contains a regular expression for each line of the source header.

-

Rationale: In some projects checking against a fixed header is not sufficient, e.g. the header might require a copyright line where the year information is not static. For example, consider the following header:

+

Rationale: In some projects checking against a fixed header is not sufficient, e.g. the header might require a copyright line where the year information is not static. For example, consider the following header:

-	line  1: ^/{71}$
-	line  2: ^// checkstyle:$
-	line  3: ^// Checks Java source code for adherence to a set of rules\.$
-	line  4: ^// Copyright \(C\) \d\d\d\d  Oliver Burn$
-	line  5: ^// Last modification by \$Author.*\$$
-	line  6: ^/{71}$
-	line  7:
-	line  8: ^package
-	line  9:
-	line 10: ^import
-	line 11:
-	line 12: ^/\*\*
-	line 13: ^ \*([^/]|$)
-	line 14: ^ \*/
+    line  1: ^/{71}$
+    line  2: ^// checkstyle:$
+    line  3: ^// Checks Java source code for adherence to a set of rules\.$
+    line  4: ^// Copyright \(C\) \d\d\d\d  Oliver Burn$
+    line  5: ^// Last modification by \$Author.*\$$
+    line  6: ^/{71}$
+    line  7:
+    line  8: ^package
+    line  9:
+    line 10: ^import
+    line 11:
+    line 12: ^/\*\*
+    line 13: ^ \*([^/]|$)
+    line 14: ^ \*/

Lines 1 and 6 demonstrate a more compact notation for 71 '/' characters. Line 4 enforces that the copyright notice includes a four digit year. Line 5 is an example how to enforce revision control keywords in a file header. Lines 12-14 is a template for javadoc (line 13 is so complicated to remove conflict with and of javadoc comment).

Different programming languages have different comment syntax rules, but all of them start a comment with a non-word character. Hence you can often use the non-word character class to abstract away the concrete comment syntax and allow checking the header for different languages with a single header definition. For example, consider the following header specification (note that this is not the full Apache license header):

-	line 1: ^#!
-	line 2: ^<\?xml.*>$
-	line 3: ^\W*$
-	line 4: ^\W*Copyright 2006 The Apache Software Foundation or its licensors, as applicable\.$
-	line 5: ^\W*Licensed under the Apache License, Version 2\.0 \(the "License"\);$
-	line 6: ^\W*$
+    line 1: ^#!
+    line 2: ^<\?xml.*>$
+    line 3: ^\W*$
+    line 4: ^\W*Copyright 2006 The Apache Software Foundation or its licensors, as applicable\.$
+    line 5: ^\W*Licensed under the Apache License, Version 2\.0 \(the "License"\);$
+    line 6: ^\W*$

Lines 1 and 2 leave room for technical header lines, e.g. the "#!/bin/sh" line in Unix shell scripts, or the xml file header of XML files. Set the multiline property to "1, 2" so these lines can be ignored for file types where they do no apply. Lines 3 through 6 define the actual header content. Note how lines 2, 4 and 5 use escapes for characters that have special regexp semantics.

Note: ignoreLines property has been removed from this check to simplify it. To make some line optional use "^.*$" regexp for this line.

diff --git a/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.imports.ImportControlCheck.html b/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.imports.ImportControlCheck.html index e0a75357..00d2d1db 100644 --- a/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.imports.ImportControlCheck.html +++ b/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.imports.ImportControlCheck.html @@ -1,6 +1,6 @@
-

ImportControl

+

ImportControl

Description

@@ -190,7 +190,7 @@

Examples

-
+

Notes on regular expressions

diff --git a/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.whitespace.FileTabCharacterCheck.html b/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.whitespace.FileTabCharacterCheck.html index 91498462..80068566 100644 --- a/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.whitespace.FileTabCharacterCheck.html +++ b/checkstyle-sonar-plugin/src/main/resources/org/sonar/l10n/checkstyle/rules/checkstyle/com.puppycrawl.tools.checkstyle.checks.whitespace.FileTabCharacterCheck.html @@ -1,7 +1,6 @@ -

Checks that there are no tab characters ('\t') in the source code. Rationale: +

Checks that there are no tab characters ('\t') in the source code. Rationale:

  • Developers should not need to configure the tab width of their text editors in order to be able to read source code.
  • From the Apache jakarta coding standards: In a distributed development environment, when the commit messages get sent to a mailing list, they are almost impossible to read if you use tabs.
-

diff --git a/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleAuditListenerTest.java b/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleAuditListenerTest.java index 5e5e4bda..59b8b2b5 100644 --- a/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleAuditListenerTest.java +++ b/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleAuditListenerTest.java @@ -44,15 +44,15 @@ public class CheckstyleAuditListenerTest { - private File file = new File("file1"); - private AuditEvent event = + private final File file = new File("file1"); + private final AuditEvent event = new AuditEvent(this, file.getAbsolutePath(), new LocalizedMessage(42, "", "", null, "", CheckstyleAuditListenerTest.class, "msg")); - private DefaultFileSystem fs = new DefaultFileSystem(new File("")); - private RuleFinder ruleFinder = mock(RuleFinder.class); - private DefaultInputFile inputFile = new DefaultInputFile("", file.getPath()); - private ResourcePerspectives perspectives = mock(ResourcePerspectives.class); + private final DefaultFileSystem fs = new DefaultFileSystem(new File("")); + private final RuleFinder ruleFinder = mock(RuleFinder.class); + private final DefaultInputFile inputFile = new DefaultInputFile("", file.getPath()); + private final ResourcePerspectives perspectives = mock(ResourcePerspectives.class); @Before public void before() { diff --git a/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleConfigurationTest.java b/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleConfigurationTest.java index 57c772a2..cb084950 100644 --- a/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleConfigurationTest.java +++ b/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleConfigurationTest.java @@ -117,9 +117,9 @@ public void getCheckstyleConfiguration() throws IOException, CheckstyleException FileUtils.forceDelete(xmlFile); } - public class FakeExporter extends CheckstyleProfileExporter { + /* default */ static class FakeExporter extends CheckstyleProfileExporter { - public FakeExporter() { + FakeExporter() { super(new Settings()); } diff --git a/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleExecutorTest.java b/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleExecutorTest.java index 9bb7deda..9c4ec99f 100644 --- a/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleExecutorTest.java +++ b/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleExecutorTest.java @@ -55,7 +55,7 @@ public class CheckstyleExecutorTest { @Rule - public ExpectedException thrown = ExpectedException.none(); + public final ExpectedException thrown = ExpectedException.none(); @Test public void execute() throws CheckstyleException { diff --git a/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleSensorTest.java b/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleSensorTest.java index 07c0f536..1d8daa6a 100644 --- a/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleSensorTest.java +++ b/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleSensorTest.java @@ -37,11 +37,11 @@ public class CheckstyleSensorTest { - private RulesProfile profile = mock(RulesProfile.class); - private DefaultFileSystem fileSystem = new DefaultFileSystem(new File("")); - private CheckstyleSensor sensor = new CheckstyleSensor(profile, null, fileSystem); + private final RulesProfile profile = mock(RulesProfile.class); + private final DefaultFileSystem fileSystem = new DefaultFileSystem(new File("")); + private final CheckstyleSensor sensor = new CheckstyleSensor(profile, null, fileSystem); - private Project project = new Project("projectKey"); + private final Project project = new Project("projectKey"); @Test public void shouldExecuteOnProjectWithoutJavaFileAndWithRule() { diff --git a/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleTestUtils.java b/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleTestUtils.java index d272a38a..58cf5593 100644 --- a/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleTestUtils.java +++ b/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/CheckstyleTestUtils.java @@ -51,12 +51,10 @@ public static void assertSimilarXml(String expectedXml, String xml) { Diff diff; try { diff = XMLUnit.compareXML(xml, expectedXml); - } catch (SAXException e) { - throw new IllegalArgumentException("Could not run XML comparison", e); - } catch (IOException e) { + } catch (SAXException | IOException e) { throw new IllegalArgumentException("Could not run XML comparison", e); } - String message = "Diff: " + diff.toString() + CharUtils.LF + "XML: " + xml; + String message = "Diff: " + diff + CharUtils.LF + "XML: " + xml; assertTrue(message, diff.similar()); } diff --git a/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/internal/CheckUtil.java b/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/internal/CheckUtil.java index b2531553..08d04066 100644 --- a/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/internal/CheckUtil.java +++ b/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/internal/CheckUtil.java @@ -126,9 +126,8 @@ public static boolean isFilterModule(Class loadedClass) { * Get's the check's messages. * @param module class to examine. * @return a set of checkstyle's module message fields. - * @throws ClassNotFoundException if the attempt to read a protected class fails. */ - public static Set getCheckMessages(Class module) throws ClassNotFoundException { + public static Set getCheckMessages(Class module) { final Set checkstyleMessages = new HashSet<>(); // get all fields from current class @@ -154,7 +153,7 @@ public static Set getCheckMessages(Class module) throws ClassNotFoundE * Gets the check message 'as is' from appropriate 'messages.properties' * file. * - * @param locale the locale to get the message for. + * @param module the module to get the message form. * @param messageKey the key of message in 'messages*.properties' file. * @param arguments the arguments of message in 'messages*.properties' file. * @return the check's formatted message. diff --git a/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/internal/ChecksTest.java b/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/internal/ChecksTest.java index b83edde8..e94797d8 100644 --- a/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/internal/ChecksTest.java +++ b/checkstyle-sonar-plugin/src/test/java/org/sonar/plugins/checkstyle/internal/ChecksTest.java @@ -73,7 +73,6 @@ public final class ChecksTest { "SuppressionCommentFilter.fileContents" ); - @SuppressWarnings("static-method") @Test public void verifyTestConfigurationFiles() throws Exception { final Set> modules = CheckUtil.getCheckstyleModules(); @@ -305,8 +304,8 @@ else if (AbstractFileSetCheck.class.isAssignableFrom(clss)) { if (AbstractCheck.class.isAssignableFrom(clss)) { final AbstractCheck check; try { - check = (AbstractCheck) clss.newInstance(); - } catch (InstantiationException | IllegalAccessException e) { + check = (AbstractCheck) clss.getConstructor().newInstance(); + } catch (ReflectiveOperationException e) { throw new IllegalStateException(e); }