Skip to content

Commit

Permalink
Issue checkstyle#49: Reduce rule violations
Browse files Browse the repository at this point in the history
Fix various rule violations reported by Teamcity.
  • Loading branch information
marschall committed Feb 11, 2017
1 parent 8a1d8c9 commit 0f22ed3
Show file tree
Hide file tree
Showing 14 changed files with 59 additions and 69 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -61,8 +61,8 @@ public class CheckstyleProfileImporter extends ProfileImporter {

private static class Module {
private String name;
private Map<String, String> properties = Maps.newHashMap();
private List<Module> modules = Lists.newArrayList();
private final Map<String, String> properties = Maps.newHashMap();
private final List<Module> modules = Lists.newArrayList();
}

public CheckstyleProfileImporter(RuleFinder ruleFinder) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,16 @@
<p>Checks that any combination of String literals with optional assignment is on the left side of an equals() comparison.</p>
<p>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.</p>
<p>For example:
<p>For example:</p>
<pre>
String nullString = null;
nullString.equals("My_Sweet_String");
</pre>
</p>

<p>
should be refactored to:
<p>should be refactored to:</p>
<pre>
String nullString = null;
"My_Sweet_String".equals(nullString);
</pre>
</p>
<p>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.</p>
<p>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.</p>
<p>The following example will cause a NullPointerException as a result of what autoboxing does.</p>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<p> Checks that the clone method is not overridden from the Object class.</p>

<p>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.
<p>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.</p>
<ul>
<li>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.</li>
<li>The Cloneable interface forces the Object's clone method to work correctly. Without implementing it, the Object's clone method will throw a CloneNotSupportedException.</li>
Expand All @@ -10,27 +10,25 @@
<li>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().</li>
<li>The clone method does not work correctly with final mutable object references because final references cannot be reassigned.</li>
<li>If a super class overrides the clone method then all subclasses must provide a correct clone implementation.</li>
</ul></p>
</ul>
<p>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.</p>

<p>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.</p>
<p>
<pre>
Shape s1 = new Square();
System.out.println(s1 instanceof Square); //true
</pre></p>
</pre>
<p>...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...</p>
<p>
<pre>
Shape s2 = new Shape(s1); //using the copy constructor
System.out.println(s2 instanceof Square); //false
</pre></p>
</pre>

<p>The working solution (without knowing about all subclasses and doing many casts) is to do the following (assuming correct clone implementation).<br/>
<p>The working solution (without knowing about all subclasses and doing many casts) is to do the following (assuming correct clone implementation).</p>
<pre>
Shape s2 = s1.clone();
System.out.println(s2 instanceof Square); //true
</pre></p>
</pre>

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

Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
<p>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.</p>
<p>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:</p>
<pre>
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: ////////////////////////////////////////////////////////////////////
</pre>
<p>Since the year information will change over time, you can tell Checkstyle to ignore line 4 by setting property ignoreLines to 4.</p>
Original file line number Diff line number Diff line change
@@ -1,30 +1,30 @@
<p>Checks the header of a source file against a header that contains a regular expression for each line of the source header.</p>
<p>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:</p>
<p>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:</p>
<pre>
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: ^ \*/
</pre>
<p>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).</p>
<p>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):</p>
<pre>
line 1: ^#!
line 2: ^&lt;\?xml.*&gt;$
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: ^&lt;\?xml.*&gt;$
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*$
</pre>
<p>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.</p>
<p>Note: ignoreLines property has been removed from this check to simplify it. To make some line optional use "^.*$" regexp for this line. </p>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<div >
<div class="section">
<h2><a name="ImportControl"></a>ImportControl<div class="anchor"><a href="http://checkstyle.sourceforge.net/config_imports.html#ImportControl"><img src="images/anchor.png"></a></div></h2>
<h2><a name="ImportControl"></a>ImportControl<span class="anchor"><a href="http://checkstyle.sourceforge.net/config_imports.html#ImportControl"><img src="images/anchor.png"></a></span></h2>

<div class="section">
<h3><a name="Description"></a>Description</h3>
Expand Down Expand Up @@ -190,7 +190,7 @@ <h3><a name="Examples"></a>Examples</h3>
</p>


<div class="section" id="regex-notes">
<div class="section">
<h4 id="regex-notes">Notes on regular expressions</h4>

<p>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
<p>Checks that there are no tab characters ('\t') in the source code. Rationale:
<p>Checks that there are no tab characters ('\t') in the source code. Rationale:</p>
<ul>
<li>Developers should not need to configure the tab width of their text editors in order to be able to read source code.</li>
<li>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.</li>
</ul>
</p>

Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<Field> getCheckMessages(Class<?> module) throws ClassNotFoundException {
public static Set<Field> getCheckMessages(Class<?> module) {
final Set<Field> checkstyleMessages = new HashSet<>();

// get all fields from current class
Expand All @@ -154,7 +153,7 @@ public static Set<Field> 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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ public final class ChecksTest {
"SuppressionCommentFilter.fileContents"
);

@SuppressWarnings("static-method")
@Test
public void verifyTestConfigurationFiles() throws Exception {
final Set<Class<?>> modules = CheckUtil.getCheckstyleModules();
Expand Down Expand Up @@ -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);
}

Expand Down

0 comments on commit 0f22ed3

Please sign in to comment.