Skip to content

Commit

Permalink
new Check UselessSuperCtorCallCheck implemeted. Fixes sevntu-checksty…
Browse files Browse the repository at this point in the history
  • Loading branch information
alex-zuy committed Jan 2, 2015
1 parent 0f081de commit a58590f
Show file tree
Hide file tree
Showing 12 changed files with 362 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,10 @@ UnnecessaryParenthesesExtended.ignoreCalculationOfBooleanVariablesWithAssert = C
UselessSingleCatchCheck.desc = <p>Checks for the presence of useless single catch blocks.</p><p>Catch block can be considered useless if it is the only catch block for try,contains only one statement which rethrows catched exception. Fox example:</p> <code> try {<br> ...<br> }<br> catch(Exception e) {<br> throw e;<br> }<br> </code>
UselessSingleCatchCheck.name = Useless single catch check
UselessSuperCtorCallCheck.name = Useless super constructor call
UselessSuperCtorCallCheck.desc = <p>Checks for useless "super()" calls in c-tors.</p><p>"super()" call could be considered by Check as "useless" in two cases:</p><p>Case 1. Example:<pre>class Dummy {<br> Dummy() {<br> super();<br> }<br>}<br></pre>"super()" call is redundant. This class does not extendanything.</p><p>Case 2. Example:<pre>class Derived extends Base {<br> Derived() {<br> super();<br> }<br>}<br></pre>"super()" is called without parameters. Java compiler automatically insertsa call to the no-argument constructor of the superclass. Check has an option"allowCallToNoArgsSuperCtorFromDerivedClass" to skip this case.</p><p>Xml checkstyle configuration example with "allowCallToNoArgsSuperCtorFromDerivedClass" option enabled:<pre> &ltmodule name="UselessSuperCtorCallCheck"&gt<br> &ltproperty name="severity" value="warning"/&gt<br> &ltproperty name="allowCallToNoArgsSuperCtorFromDerivedClass" value="true"/&gt<br> &lt/module&gt<br></pre></p>@author <a href="mailto:zuy_alexey@mail.ru">Zuy Alexey</a>
UselessSuperCtorCallCheck.allowCallToNoArgsSuperCtorFromDerivedClass = Allow calls to no-arguments super constructor from derived class.
EitherLogOrThrowCheck.desc = <p>Either log the exception, or throw it, but never do both. Logging and throwing results in multiple log messages for a single problem in the code, and makes problems for the support engineer who is trying to dig through the logs. This is one of the most annoying error-handling antipatterns. All of these examples are equally wrong.</p><p><b>Examples:</b><pre>catch (NoSuchMethodException e) {\t\nLOG.error("Message", e);\t\nthrow e;\n}</pre><b>or</b><pre>catch (NoSuchMethodException e) {\t\nLOG.error("Message", e);\t\nthrow new MyServiceException("AnotherMessage", e);\n}</pre><b>or</b><pre>catch (NoSuchMethodException e) {\t\ne.printStackTrace();\t\nthrow new MyServiceException("Message", e);\n}</pre></p><p><b>What check can detect:</b> <br><b>Loggers</b><ul><li>logger is declared as class field</li><li>logger is declared as method's local variable</li><li>logger is declared as local variable in <code>catch</code> block</li><li>logger is passed through method's parameters</li></ul><b>Exceptions</b><ul><li>logger logs <code>catch</code> parameter exception or it's message</li><li>throw <code>catch</code> parameter exception</li><li>throw another exception which is based on <code>catch</code> parameter exception</li><li>printStackTrace was called on <code>catch</code> parameter exception</li></ul></p><b>What check can not detect:</b> <br><ul><li>loggers that is used like method's return value. Example:<pre>getLogger().error(&quot;message&quot;, e)</pre></li> <li>loggers that is used like static fields from another classes:<pre>MyAnotherClass.LOGGER.error("message", e);<pre></li></ul></p><p>Default parameters are:<ul><li><b>loggerFullyQualifiedClassName</b> - fully qualified class name of logger type. Default value is <i>"org.slf4j.Logger"</i>.</li><li><b>loggingMethodNames</b> - comma separated names of logging methods. Default value is <i>"error, warn, info, debug"</i>.</li></ul></p><p>Note that check works with only one logger type. If you have multiple different loggers, then create another instance of this check.</p>
EitherLogOrThrowCheck.name = Either log exception or throw exception.
EitherLogOrThrowCheck.loggerFullyQualifiedClassName = Logger fully qualified class name. Example: "org.slf4j.Logger".
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -323,12 +323,22 @@
<message-key key="unnecessary.paren.return" />
<message-key key="unnecessary.paren.string" />
</rule-metadata>

<rule-metadata name="%UselessSingleCatchCheck.name" internal-name="UselessSingleCatchCheck" parent="TreeWalker">
<alternative-name internal-name="com.github.sevntu.checkstyle.checks.coding.UselessSingleCatchCheck"/>
<description>%UselessSingleCatchCheck.desc</description>
</rule-metadata>


<rule-metadata name="%UselessSuperCtorCallCheck.name" internal-name="UselessSuperCtorCallCheck" parent="TreeWalker">
<alternative-name internal-name="com.github.sevntu.checkstyle.checks.coding.UselessSuperCtorCallCheck"/>
<description>%UselessSuperCtorCallCheck.desc</description>
<property-metadata name="allowCallToNoArgsSuperCtorFromDerivedClass" datatype="Boolean" default-value="false">
<description>%UselessSuperCtorCallCheck.allowCallToNoArgsSuperCtorFromDerivedClass</description>
</property-metadata>
<message-key key="useless.super.ctor.call.in.not.derived.class" />
<message-key key="useless.super.ctor.call.without.args" />
</rule-metadata>

<rule-metadata name="%EitherLogOrThrowCheck.name" internal-name="EitherLogOrThrowCheck" parent="TreeWalker">
<alternative-name internal-name="com.github.sevntu.checkstyle.checks.coding.EitherLogOrThrowCheck" />
<description>%EitherLogOrThrowCheck.desc</description>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
////////////////////////////////////////////////////////////////////////////////
// checkstyle: Checks Java source code for adherence to a set of rules.
// Copyright (C) 2001-2014 Oliver Burn
//
// This library is free software; you can redistribute it and/or
// modify it under the terms of the GNU Lesser General Public
// License as published by the Free Software Foundation; either
// version 2.1 of the License, or (at your option) any later version.
//
// This library is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
// Lesser General Public License for more details.
//
// You should have received a copy of the GNU Lesser General Public
// License along with this library; if not, write to the Free Software
// Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
////////////////////////////////////////////////////////////////////////////////

package com.github.sevntu.checkstyle.checks.coding;

import com.puppycrawl.tools.checkstyle.api.Check;
import com.puppycrawl.tools.checkstyle.api.DetailAST;
import com.puppycrawl.tools.checkstyle.api.TokenTypes;

/**
* <p>
* Checks for useless "super()" calls in c-tors.
* </p>
* <p>
* "super()" call could be considered by Check as "useless" in two cases:
* </p>
* <p>
* Case 1. Example:
* <pre>
* class Dummy {
* Dummy() {
* super();
* }
* }
* </pre>
* "super()" call is redundant. This class does not extend
* anything.
* </p>
*
* <p>
* Case 2. Example:
* <pre>
* class Derived extends Base {
* Derived() {
* super();
* }
* }
* </pre>
* "super()" is called without parameters. Java compiler automatically inserts
* a call to the no-argument constructor of the superclass. Check has an option
* "allowCallToNoArgsSuperCtorFromDerivedClass" to skip this case.
* </p>
* <p>
* Xml checkstyle configuration example with "allowCallToNoArgsSuperCtorFromDerivedClass"
* option enabled:
* <pre>
* {@code
* <module name="UselessSuperCtorCallCheck">
* <property name="severity" value="warning"/>
* <property name="allowCallToNoArgsSuperCtorFromDerivedClass" value="true"/>
* </module>
* }
* </pre>
* </p>
*
* @author <a href="mailto:zuy_alexey@mail.ru">Zuy Alexey</a>
*/
public class UselessSuperCtorCallCheck extends Check
{
/**
* Violation message key.
*/
public static final String MSG_IN_NOT_DERIVED_CLASS =
"useless.super.ctor.call.in.not.derived.class";

/**
* Violation message key.
*/
public static final String MSG_WITHOUT_ARGS =
"useless.super.ctor.call.without.args";

/**
* Used to allow calls to no-arguments super constructor from derived class.
* By default check will log this case.
*/
private boolean mAllowCallToNoArgsSuperCtorFromDerivedClass = false;

/**
* Sets flag to allowCallToNoArgsSuperCtor.
* @param aAllowCallToNoArgsSuperCtor
* if true, check will allow super() calls without arguments
*/
public void setAllowCallToNoArgsSuperCtorFromDerivedClass(boolean aAllowCallToNoArgsSuperCtor)
{
mAllowCallToNoArgsSuperCtorFromDerivedClass = aAllowCallToNoArgsSuperCtor;
}

@Override
public int[] getDefaultTokens()
{
return new int[] { TokenTypes.SUPER_CTOR_CALL };
}

@Override
public void visitToken(DetailAST aSuperCallNode)
{
if (getMethodCallArgsCount(aSuperCallNode) == 0)
{
DetailAST classDefNode = getClassDefinitionNode(aSuperCallNode);
String className = classDefNode.findFirstToken(TokenTypes.IDENT).getText();

if (isClassDerived(classDefNode))
{
if (!mAllowCallToNoArgsSuperCtorFromDerivedClass)
{
log(aSuperCallNode.getLineNo(), MSG_WITHOUT_ARGS, className);
}
}
else
{
log(aSuperCallNode.getLineNo(), MSG_IN_NOT_DERIVED_CLASS, className);
}
}
}

/**
* Returns arguments count for method call.
* @param aMethodCallNode
* node of type TokenTypes.METHOD_CALL
* @return arguments count for method call
*/
private static int getMethodCallArgsCount(DetailAST aMethodCallNode)
{
DetailAST argsListNode = aMethodCallNode.findFirstToken(TokenTypes.ELIST);

return argsListNode.getChildCount();
}

/**
* Returns class definition node for class, which contains given AST node.
* @param aNode
* AST node inside class
* @return class definition node
*/
private static DetailAST getClassDefinitionNode(DetailAST aNode)
{
DetailAST result = aNode;

while (result.getType() != TokenTypes.CLASS_DEF)
{
result = result.getParent();
}

return result;
}

/**
* Checks whether this class is derived from other class.
* @param aClassDefNode
* class definition node
* @return true, if this class extends anything
*/
private static boolean isClassDerived(DetailAST aClassDefNode)
{
return aClassDefNode.findFirstToken(TokenTypes.EXTENDS_CLAUSE) != null;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -100,5 +100,7 @@ unnecessary.paren.literal=Unnecessary parentheses around literal ''{0}''.
unnecessary.paren.return=Unnecessary parentheses around return value.
unnecessary.paren.string=Unnecessary parentheses around string {0}.
useless.single.catch.check=Useless catch block. No logging, wrapping or handling code here.
useless.super.ctor.call.in.not.derived.class= Super call could be removed: Class ''{0}'' does not extend anything.
useless.super.ctor.call.without.args= Redundant super constructor call could be removed. Class ''{0}'' has superclass. Java compiler automatically inserts a call to the no-argument constructor of the superclass.
return.null.Boolean=Method declares to return Boolean and returns null.
return.boolean.ternary=Returning explicit boolean from ternary operator.
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
package com.github.sevntu.checkstyle.checks.coding;

import static com.github.sevntu.checkstyle.checks.coding.UselessSuperCtorCallCheck.*;

import org.junit.Test;

import com.github.sevntu.checkstyle.BaseCheckTestSupport;
import com.puppycrawl.tools.checkstyle.DefaultConfiguration;

public class UselessSuperCtorCallCheckTest extends BaseCheckTestSupport
{
private final DefaultConfiguration mDefaultConfig = createCheckConfig(UselessSuperCtorCallCheck.class);

@Test
public void testSingleCtorWithSuperWithinNotDerivedClass()
throws Exception
{
final String expected[] = {
"7: " + getCheckMessage(MSG_IN_NOT_DERIVED_CLASS,
"InputUselessSuperCtorCall1"),
};

verify(mDefaultConfig, getPath("InputUselessSuperCtorCall1.java"), expected);
}

@Test
public void testSingleCtorWithSuperWithinDerivedClass()
throws Exception
{
final String expected[] = {
"7: " + getCheckMessage(MSG_WITHOUT_ARGS,
"InputUselessSuperCtorCall2"),
};

verify(mDefaultConfig, getPath("InputUselessSuperCtorCall2.java"), expected);
}

@Test
public void testMultipleCtorsWithSuperWithinNonsubclass()
throws Exception
{
final String expected[] = {
"7: " + getCheckMessage(MSG_IN_NOT_DERIVED_CLASS,
"InputUselessSuperCtorCall3"),
"12: " + getCheckMessage(MSG_IN_NOT_DERIVED_CLASS,
"InputUselessSuperCtorCall3"),
};

verify(mDefaultConfig, getPath("InputUselessSuperCtorCall3.java"), expected);
}

@Test
public void testInnerClassWithCtorWithSuper()
throws Exception
{
final String expected[] = {
"9: " + getCheckMessage(MSG_IN_NOT_DERIVED_CLASS,
"Inner"),
};

verify(mDefaultConfig, getPath("InputUselessSuperCtorCall4.java"), expected);
}

@Test
public void testClassWithSuperCtorWithArgs()
throws Exception
{
final String expected[] = {};

verify(mDefaultConfig, getPath("InputUselessSuperCtorCall5.java"), expected);
}

@Test
public void testClassWithSuperCtorWithoutArgsAndIgnoreSuperCtorCallOptionSetToTrue()
throws Exception
{
final DefaultConfiguration checkConfig = createCheckConfig(UselessSuperCtorCallCheck.class);
checkConfig.addAttribute("allowCallToNoArgsSuperCtorFromDerivedClass", "true");

final String expected[] = {};

verify(checkConfig, getPath("InputUselessSuperCtorCall6.java"), expected);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package com.github.sevntu.checkstyle.checks.coding;

public class InputUselessSuperCtorCall1
{
public InputUselessSuperCtorCall1()
{
super();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package com.github.sevntu.checkstyle.checks.coding;

public class InputUselessSuperCtorCall2 extends java.lang.Object
{
public InputUselessSuperCtorCall2()
{
super();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package com.github.sevntu.checkstyle.checks.coding;

public class InputUselessSuperCtorCall3
{
public InputUselessSuperCtorCall3()
{
super();
}

public InputUselessSuperCtorCall3(int i)
{
super();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.github.sevntu.checkstyle.checks.coding;

public class InputUselessSuperCtorCall4 extends java.lang.Object
{
public class Inner
{
public Inner()
{
super();
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package com.github.sevntu.checkstyle.checks.coding;

import java.io.File;
import java.io.PrintWriter;

public class InputUselessSuperCtorCall5
{
private static class Base
{
public Base(int i)
{

}
}

private static class Derived extends Base
{
public Derived()
{
super(2);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package com.github.sevntu.checkstyle.checks.coding;

public class InputUselessSuperCtorCall6 extends java.lang.Object
{
public InputUselessSuperCtorCall6() {
super();
}
}
Loading

0 comments on commit a58590f

Please sign in to comment.