Made MagicNumber check less agressive, based on feedback on checkstyle-user list
This commit is contained in:
Lars Kühne
parent
bd4ec9a98e
commit
fc17c98323
|
|
@ -127,6 +127,9 @@ checkstyle-user</a>).</li>
|
|||
within the same file.</li>
|
||||
|
||||
<li class="body">Unexpected char (bug 975346).</li>
|
||||
|
||||
<li class="body">MagicNumber check overly aggressive
|
||||
(reported on user mailing list).</li>
|
||||
</ul>
|
||||
|
||||
<p class="body">
|
||||
|
|
|
|||
|
|
@ -41,10 +41,29 @@ import java.util.Arrays;
|
|||
* </module>
|
||||
* </pre>
|
||||
* @author Rick Giles
|
||||
* @author Lars Kühne
|
||||
*/
|
||||
public class MagicNumberCheck extends Check
|
||||
{
|
||||
/** the numbers to ignore in the check, sorted */
|
||||
private static final int[] ALLOWED_PATH_TOKENTYPES =
|
||||
{
|
||||
TokenTypes.ASSIGN,
|
||||
TokenTypes.ARRAY_INIT,
|
||||
TokenTypes.EXPR,
|
||||
TokenTypes.UNARY_PLUS,
|
||||
TokenTypes.UNARY_MINUS,
|
||||
TokenTypes.TYPECAST,
|
||||
TokenTypes.ELIST,
|
||||
TokenTypes.LITERAL_NEW,
|
||||
TokenTypes.METHOD_CALL,
|
||||
TokenTypes.STAR,
|
||||
};
|
||||
|
||||
static {
|
||||
Arrays.sort(ALLOWED_PATH_TOKENTYPES);
|
||||
}
|
||||
|
||||
/** the numbers to ignore in the check, sorted */
|
||||
private double[] mIgnoreNumbers = {-1, 0, 1, 2};
|
||||
|
||||
/** @see com.puppycrawl.tools.checkstyle.api.Check */
|
||||
|
|
@ -61,26 +80,87 @@ public class MagicNumberCheck extends Check
|
|||
/** @see com.puppycrawl.tools.checkstyle.api.Check */
|
||||
public void visitToken(DetailAST aAST)
|
||||
{
|
||||
if (!inIgnoreList(aAST) && !isConstantDefinition(aAST)) {
|
||||
String text = aAST.getText();
|
||||
final DetailAST parent = aAST.getParent();
|
||||
DetailAST reportAST = aAST;
|
||||
if (parent.getType() == TokenTypes.UNARY_MINUS) {
|
||||
reportAST = parent;
|
||||
text = "-" + text;
|
||||
}
|
||||
else if (parent.getType() == TokenTypes.UNARY_PLUS) {
|
||||
reportAST = parent;
|
||||
text = "+" + text;
|
||||
}
|
||||
log(reportAST.getLineNo(),
|
||||
reportAST.getColumnNo(),
|
||||
"magic.number",
|
||||
text);
|
||||
if (inIgnoreList(aAST)) {
|
||||
return;
|
||||
}
|
||||
|
||||
DetailAST constantDefAST = findContainingConstantDef(aAST);
|
||||
|
||||
if (constantDefAST == null) {
|
||||
reportMagicNumber(aAST);
|
||||
}
|
||||
else {
|
||||
DetailAST ast = aAST.getParent();
|
||||
while (ast != constantDefAST) {
|
||||
int type = ast.getType();
|
||||
if (Arrays.binarySearch(ALLOWED_PATH_TOKENTYPES, type) < 0) {
|
||||
reportMagicNumber(aAST);
|
||||
break;
|
||||
}
|
||||
|
||||
ast = ast.getParent();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Finds the constant definition that contains aAST.
|
||||
* @param aAST the AST
|
||||
* @return the constant def or null if aAST is not
|
||||
* contained in a constant definition
|
||||
*/
|
||||
private DetailAST findContainingConstantDef(DetailAST aAST) {
|
||||
DetailAST varDefAST = aAST;
|
||||
while (varDefAST != null
|
||||
&& varDefAST.getType() != TokenTypes.VARIABLE_DEF)
|
||||
{
|
||||
varDefAST = varDefAST.getParent();
|
||||
}
|
||||
|
||||
// no containing variable definition?
|
||||
if (varDefAST == null) {
|
||||
return null;
|
||||
}
|
||||
|
||||
// implicit constant?
|
||||
if (ScopeUtils.inInterfaceBlock(varDefAST)) {
|
||||
return varDefAST;
|
||||
}
|
||||
|
||||
// explicit constant
|
||||
final DetailAST modifiersAST =
|
||||
varDefAST.findFirstToken(TokenTypes.MODIFIERS);
|
||||
if (modifiersAST.branchContains(TokenTypes.FINAL)) {
|
||||
return varDefAST;
|
||||
}
|
||||
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Reports aAST as a magic number, includes unary operators as needed.
|
||||
* @param aAST the AST node that contains the number to report
|
||||
*/
|
||||
private void reportMagicNumber(DetailAST aAST)
|
||||
{
|
||||
String text = aAST.getText();
|
||||
final DetailAST parent = aAST.getParent();
|
||||
DetailAST reportAST = aAST;
|
||||
if (parent.getType() == TokenTypes.UNARY_MINUS) {
|
||||
reportAST = parent;
|
||||
text = "-" + text;
|
||||
}
|
||||
else if (parent.getType() == TokenTypes.UNARY_PLUS) {
|
||||
reportAST = parent;
|
||||
text = "+" + text;
|
||||
}
|
||||
log(reportAST.getLineNo(),
|
||||
reportAST.getColumnNo(),
|
||||
"magic.number",
|
||||
text);
|
||||
}
|
||||
|
||||
/**
|
||||
* Decides whether the number of an AST is in the ignore list of this
|
||||
* check.
|
||||
* @param aAST the AST to check
|
||||
|
|
@ -97,59 +177,6 @@ public class MagicNumberCheck extends Check
|
|||
return (Arrays.binarySearch(mIgnoreNumbers, value) >= 0);
|
||||
}
|
||||
|
||||
/**
|
||||
* Decides whether the number of an AST is the RHS of a constant
|
||||
* definition.
|
||||
* @param aAST the AST to check.
|
||||
* @return true if the number of aAST is the RHS of a constant definition.
|
||||
*/
|
||||
private boolean isConstantDefinition(DetailAST aAST)
|
||||
{
|
||||
if (ScopeUtils.inInterfaceBlock(aAST)) {
|
||||
return true;
|
||||
}
|
||||
DetailAST parent = aAST.getParent();
|
||||
|
||||
if (parent == null) {
|
||||
return false;
|
||||
}
|
||||
|
||||
//skip TYPECAST, UNARY_MINUS, UNARY_PLUS
|
||||
while ((parent.getType() == TokenTypes.UNARY_MINUS)
|
||||
|| (parent.getType() == TokenTypes.UNARY_PLUS)
|
||||
|| (parent.getType() == TokenTypes.TYPECAST))
|
||||
{
|
||||
parent = parent.getParent();
|
||||
}
|
||||
|
||||
//expression?
|
||||
if ((parent == null) || (parent.getType() != TokenTypes.EXPR)) {
|
||||
return false;
|
||||
}
|
||||
|
||||
//array init?
|
||||
parent = parent.getParent();
|
||||
if ((parent != null) && (parent.getType() == TokenTypes.ARRAY_INIT)) {
|
||||
parent = parent.getParent();
|
||||
}
|
||||
|
||||
//assignment?
|
||||
if ((parent == null) || (parent.getType() != TokenTypes.ASSIGN)) {
|
||||
return false;
|
||||
}
|
||||
|
||||
//variable definition?
|
||||
parent = parent.getParent();
|
||||
if ((parent == null) || (parent.getType() != TokenTypes.VARIABLE_DEF)) {
|
||||
return false;
|
||||
}
|
||||
|
||||
//final?
|
||||
final DetailAST modifiersAST =
|
||||
parent.findFirstToken(TokenTypes.MODIFIERS);
|
||||
return modifiersAST.branchContains(TokenTypes.FINAL);
|
||||
}
|
||||
|
||||
/**
|
||||
* Sets the numbers to ignore in the check.
|
||||
* BeanUtils converts numeric token list to double array automatically.
|
||||
|
|
|
|||
|
|
@ -83,6 +83,7 @@ class ArrayMagicTest
|
|||
{
|
||||
private static final int[] NONMAGIC = {3};
|
||||
private int[] magic = {3};
|
||||
private static final int[][] NONMAGIC2 = {{1}, {2}, {3}};
|
||||
}
|
||||
|
||||
/** test long hex */
|
||||
|
|
@ -119,3 +120,25 @@ class Cast
|
|||
{
|
||||
public static final int TESTINTVAL = (byte) 0x80;
|
||||
}
|
||||
|
||||
class ComplexAndFlagged
|
||||
{
|
||||
public static final java.util.List MYLIST = new java.util.ArrayList()
|
||||
{
|
||||
public int size()
|
||||
{
|
||||
// should be flagged although technically inside const definition
|
||||
return 378;
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
class ComplexButNotFlagged
|
||||
{
|
||||
// according to user feedback this is typical code that should not be flagged
|
||||
// (at least in the default configuration of MagicNumberCheck)
|
||||
public static final Integer DEFAULT_INT = new Integer(27);
|
||||
public static final int SECS_PER_DAY = 24 * 60 * 60;
|
||||
public static final javax.swing.Border STD_BORDER =
|
||||
javax.swing.BorderFactory.createEmptyBorder(3, 3, 3, 3);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -33,15 +33,16 @@ public class MagicNumberCheckTest
|
|||
"71:29: '0x10L' is a magic number.",
|
||||
"72:29: '0X11l' is a magic number.",
|
||||
"85:28: '3' is a magic number.",
|
||||
"91:14: '0xffffffffL' is a magic number.",
|
||||
"99:30: '+3' is a magic number.",
|
||||
"100:29: '-2' is a magic number.",
|
||||
"101:35: '+3.5' is a magic number.",
|
||||
"102:36: '-2.5' is a magic number.",
|
||||
"110:35: '0x80000000' is a magic number.",
|
||||
"111:36: '0x8000000000000000L' is a magic number.",
|
||||
"114:37: '020000000000' is a magic number.",
|
||||
"115:38: '01000000000000000000000L' is a magic number.",
|
||||
"92:14: '0xffffffffL' is a magic number.",
|
||||
"100:30: '+3' is a magic number.",
|
||||
"101:29: '-2' is a magic number.",
|
||||
"102:35: '+3.5' is a magic number.",
|
||||
"103:36: '-2.5' is a magic number.",
|
||||
"111:35: '0x80000000' is a magic number.",
|
||||
"112:36: '0x8000000000000000L' is a magic number.",
|
||||
"115:37: '020000000000' is a magic number.",
|
||||
"116:38: '01000000000000000000000L' is a magic number.",
|
||||
"131:32: '378' is a magic number.",
|
||||
};
|
||||
verify(checkConfig, getPath("InputMagicNumber.java"), expected);
|
||||
}
|
||||
|
|
@ -68,18 +69,19 @@ public class MagicNumberCheckTest
|
|||
"64:30: '011l' is a magic number.",
|
||||
"69:24: '0X011' is a magic number.",
|
||||
"72:29: '0X11l' is a magic number.",
|
||||
"91:14: '0xffffffffL' is a magic number.",
|
||||
"100:29: '-2' is a magic number.",
|
||||
"101:35: '+3.5' is a magic number.",
|
||||
"102:36: '-2.5' is a magic number.",
|
||||
"108:34: '0xffffffff' is a magic number.",
|
||||
"109:36: '0xffffffffffffffffL' is a magic number.",
|
||||
"110:35: '0x80000000' is a magic number.",
|
||||
"111:36: '0x8000000000000000L' is a magic number.",
|
||||
"112:36: '037777777777' is a magic number.",
|
||||
"113:38: '01777777777777777777777L' is a magic number.",
|
||||
"114:37: '020000000000' is a magic number.",
|
||||
"115:38: '01000000000000000000000L' is a magic number.",
|
||||
"92:14: '0xffffffffL' is a magic number.",
|
||||
"101:29: '-2' is a magic number.",
|
||||
"102:35: '+3.5' is a magic number.",
|
||||
"103:36: '-2.5' is a magic number.",
|
||||
"109:34: '0xffffffff' is a magic number.",
|
||||
"110:36: '0xffffffffffffffffL' is a magic number.",
|
||||
"111:35: '0x80000000' is a magic number.",
|
||||
"112:36: '0x8000000000000000L' is a magic number.",
|
||||
"113:36: '037777777777' is a magic number.",
|
||||
"114:38: '01777777777777777777777L' is a magic number.",
|
||||
"115:37: '020000000000' is a magic number.",
|
||||
"116:38: '01000000000000000000000L' is a magic number.",
|
||||
"131:32: '378' is a magic number.",
|
||||
};
|
||||
verify(checkConfig, getPath("InputMagicNumber.java"), expected);
|
||||
}
|
||||
|
|
@ -133,19 +135,20 @@ public class MagicNumberCheckTest
|
|||
"71:29: '0x10L' is a magic number.",
|
||||
"72:29: '0X11l' is a magic number.",
|
||||
"85:28: '3' is a magic number.",
|
||||
"91:14: '0xffffffffL' is a magic number.",
|
||||
"99:30: '+3' is a magic number.",
|
||||
"100:29: '-2' is a magic number.",
|
||||
"101:35: '+3.5' is a magic number.",
|
||||
"102:36: '-2.5' is a magic number.",
|
||||
"108:34: '0xffffffff' is a magic number.",
|
||||
"109:36: '0xffffffffffffffffL' is a magic number.",
|
||||
"110:35: '0x80000000' is a magic number.",
|
||||
"111:36: '0x8000000000000000L' is a magic number.",
|
||||
"112:36: '037777777777' is a magic number.",
|
||||
"113:38: '01777777777777777777777L' is a magic number.",
|
||||
"114:37: '020000000000' is a magic number.",
|
||||
"115:38: '01000000000000000000000L' is a magic number.",
|
||||
"92:14: '0xffffffffL' is a magic number.",
|
||||
"100:30: '+3' is a magic number.",
|
||||
"101:29: '-2' is a magic number.",
|
||||
"102:35: '+3.5' is a magic number.",
|
||||
"103:36: '-2.5' is a magic number.",
|
||||
"109:34: '0xffffffff' is a magic number.",
|
||||
"110:36: '0xffffffffffffffffL' is a magic number.",
|
||||
"111:35: '0x80000000' is a magic number.",
|
||||
"112:36: '0x8000000000000000L' is a magic number.",
|
||||
"113:36: '037777777777' is a magic number.",
|
||||
"114:38: '01777777777777777777777L' is a magic number.",
|
||||
"115:37: '020000000000' is a magic number.",
|
||||
"116:38: '01000000000000000000000L' is a magic number.",
|
||||
"131:32: '378' is a magic number.",
|
||||
};
|
||||
verify(checkConfig, getPath("InputMagicNumber.java"), expected);
|
||||
}
|
||||
|
|
@ -176,13 +179,14 @@ public class MagicNumberCheckTest
|
|||
"71:29: '0x10L' is a magic number.",
|
||||
"72:29: '0X11l' is a magic number.",
|
||||
"85:28: '3' is a magic number.",
|
||||
"91:14: '0xffffffffL' is a magic number.",
|
||||
"99:30: '+3' is a magic number.",
|
||||
"100:29: '-2' is a magic number.",
|
||||
"110:35: '0x80000000' is a magic number.",
|
||||
"111:36: '0x8000000000000000L' is a magic number.",
|
||||
"114:37: '020000000000' is a magic number.",
|
||||
"115:38: '01000000000000000000000L' is a magic number.",
|
||||
"92:14: '0xffffffffL' is a magic number.",
|
||||
"100:30: '+3' is a magic number.",
|
||||
"101:29: '-2' is a magic number.",
|
||||
"111:35: '0x80000000' is a magic number.",
|
||||
"112:36: '0x8000000000000000L' is a magic number.",
|
||||
"115:37: '020000000000' is a magic number.",
|
||||
"116:38: '01000000000000000000000L' is a magic number.",
|
||||
"131:32: '378' is a magic number.",
|
||||
};
|
||||
verify(checkConfig, getPath("InputMagicNumber.java"), expected);
|
||||
}
|
||||
|
|
@ -214,9 +218,10 @@ public class MagicNumberCheckTest
|
|||
"71:29: '0x10L' is a magic number.",
|
||||
"72:29: '0X11l' is a magic number.",
|
||||
"85:28: '3' is a magic number.",
|
||||
"91:14: '0xffffffffL' is a magic number.",
|
||||
"99:30: '+3' is a magic number.",
|
||||
"100:29: '-2' is a magic number.",
|
||||
"92:14: '0xffffffffL' is a magic number.",
|
||||
"100:30: '+3' is a magic number.",
|
||||
"101:29: '-2' is a magic number.",
|
||||
"131:32: '378' is a magic number.",
|
||||
};
|
||||
verify(checkConfig, getPath("InputMagicNumber.java"), expected);
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue