From b2a80ceca30df26e628d6f343dbc49d7b6481409 Mon Sep 17 00:00:00 2001 From: Bhavik Patel Date: Mon, 6 Jul 2015 22:13:06 +0530 Subject: [PATCH] Solution to Magic Number is not detected properly #1266 --- .../checks/coding/MagicNumberCheck.java | 74 +++++++++++++++---- .../checks/coding/MagicNumberCheckTest.java | 59 +++++++++++++++ .../tools/checkstyle/InputMagicNumber.java | 19 ++++- src/xdocs/config_coding.xml | 36 ++++++++- 4 files changed, 167 insertions(+), 21 deletions(-) diff --git a/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/MagicNumberCheck.java b/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/MagicNumberCheck.java index 2e185ed2b..54857b178 100644 --- a/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/MagicNumberCheck.java +++ b/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/MagicNumberCheck.java @@ -22,6 +22,7 @@ package com.puppycrawl.tools.checkstyle.checks.coding; import java.util.Arrays; import com.puppycrawl.tools.checkstyle.ScopeUtils; +import com.puppycrawl.tools.checkstyle.Utils; import com.puppycrawl.tools.checkstyle.api.Check; import com.puppycrawl.tools.checkstyle.api.DetailAST; import com.puppycrawl.tools.checkstyle.api.TokenTypes; @@ -35,6 +36,7 @@ import com.puppycrawl.tools.checkstyle.checks.CheckUtils; * By default, -1, 0, 1, and 2 are not considered to be magic numbers. *

*

+ * Constant definition is any variable/field that has 'final' modifier. * It is fine to have one constant defining multiple numeric literals within one expression: * 

  * static final int SECONDS_PER_DAY = 24 * 60 * 60;
@@ -95,12 +97,42 @@ import com.puppycrawl.tools.checkstyle.checks.CheckUtils;
  *
  *       void foo() {
  *          int i = i + 1; // no violation
- *          int j = j + 8; // violation
+ *          int j = j + (int)0.5; // no violation
  *       }
  *   }
  * 
  *
- * + *

+ * Config example of constantWaiverParentToken option: + *

+ * 
+ *   <module name="MagicNumber">
+ *       <property name="constantWaiverParentToken" value="ASSIGN,ARRAY_INIT,EXPR,
+ *       UNARY_PLUS, UNARY_MINUS, TYPECAST, ELIST, DIV, PLUS "/>
+ *   </module>
+ *
+ *

+ * result is following violation: + *

+ * 
+ * 
+ * class TestMethodCall {
+ *     public void method2() {
+ *         final TestMethodCall dummyObject = new TestMethodCall(62);    //violation
+ *         final int a = 3;        // ok as waiver is ASSIGN
+ *         final int [] b = {4, 5} // ok as waiver is ARRAY_INIT
+ *         final int c = -3;       // ok as waiver is UNARY_MINUS
+ *         final int d = +4;       // ok as waiver is UNARY_PLUS
+ *         final int e = method(1, 2) // ELIST is there but violation due to METHOD_CALL
+ *         final int x = 3 * 4;    // violation
+ *         final int y = 3 / 4;    // ok as waiver is DIV
+ *         final int z = 3 + 4;    // ok as waiver is PLUS
+ *         final int w = 3 - 4;    // violation
+ *         final int x = (int)(3.4);    //ok as waiver is TYPECAST
+ *     }
+ * }
+ * 
+ *
* @author Rick Giles * @author Lars Kühne * @author Daniel Solano Gómez @@ -117,7 +149,7 @@ public class MagicNumberCheck extends Check { * The token types that are allowed in the AST path from the * number literal to the enclosing constant definition. */ - private static final int[] ALLOWED_PATH_TOKENTYPES = { + private int[] constantWaiverParentToken = { TokenTypes.ASSIGN, TokenTypes.ARRAY_INIT, TokenTypes.EXPR, @@ -133,10 +165,6 @@ public class MagicNumberCheck extends Check { TokenTypes.MINUS, }; - static { - Arrays.sort(ALLOWED_PATH_TOKENTYPES); - } - /** the numbers to ignore in the check, sorted */ private double[] ignoreNumbers = {-1, 0, 1, 2}; @@ -149,14 +177,18 @@ public class MagicNumberCheck extends Check { /** Whether to ignore magic numbers in field declaration. */ private boolean ignoreFieldDeclaration; + /** + * Constructor for MagicNumber Check. + * Sort the allowedTokensBetweenMagicNumberAndConstDef array for binary search. + */ + public MagicNumberCheck() { + super(); + Arrays.sort(constantWaiverParentToken); + } + @Override public int[] getDefaultTokens() { - return new int[] { - TokenTypes.NUM_DOUBLE, - TokenTypes.NUM_FLOAT, - TokenTypes.NUM_INT, - TokenTypes.NUM_LONG, - }; + return getAcceptableTokens(); } @Override @@ -191,7 +223,6 @@ public class MagicNumberCheck extends Check { final boolean found = isMagicNumberExists(ast, constantDefAST); if (found) { reportMagicNumber(ast); - } } } @@ -202,12 +233,12 @@ public class MagicNumberCheck extends Check { * @param constantDefAST constant ast * @return true if magic number is present */ - private static boolean isMagicNumberExists(DetailAST ast, DetailAST constantDefAST) { + private boolean isMagicNumberExists(DetailAST ast, DetailAST constantDefAST) { boolean found = false; DetailAST astNode = ast.getParent(); while (astNode != constantDefAST) { final int type = astNode.getType(); - if (Arrays.binarySearch(ALLOWED_PATH_TOKENTYPES, type) < 0) { + if (Arrays.binarySearch(constantWaiverParentToken, type) < 0) { found = true; break; } @@ -353,6 +384,17 @@ public class MagicNumberCheck extends Check { == TokenTypes.CLASS_DEF; } + /** + * sets the tokens which are allowed between Magic Number and defined Object. + * @param tokens The string representation of the tokens interested in + */ + public void setConstantWaiverParentToken(String... tokens) { + constantWaiverParentToken = new int[tokens.length]; + for (int i = 0; i < tokens.length; i++) { + constantWaiverParentToken[i] = Utils.getTokenId(tokens[i]); + } + Arrays.sort(constantWaiverParentToken); + } /** * Sets the numbers to ignore in the check. diff --git a/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/MagicNumberCheckTest.java b/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/MagicNumberCheckTest.java index 304ab9231..23d2e4f21 100644 --- a/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/MagicNumberCheckTest.java +++ b/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/MagicNumberCheckTest.java @@ -373,4 +373,63 @@ public class MagicNumberCheckTest }; verify(checkConfig, getPath("InputMagicNumber.java"), expected); } + + @Test + public void testwaiverParentToken() + throws Exception { + final DefaultConfiguration checkConfig = + createCheckConfig(MagicNumberCheck.class); + checkConfig.addAttribute("constantWaiverParentToken", "ASSIGN, ARRAY_INIT," + + " EXPR, UNARY_PLUS, UNARY_MINUS, TYPECAST, ELIST, STAR, DIV, PLUS, MINUS"); + final String[] expected = { + "41:26: " + getCheckMessage(MSG_KEY, "3_000"), + "42:32: " + getCheckMessage(MSG_KEY, "1.5_0"), + "43:27: " + getCheckMessage(MSG_KEY, "3"), + "43:31: " + getCheckMessage(MSG_KEY, "4"), + "45:29: " + getCheckMessage(MSG_KEY, "3"), + "47:23: " + getCheckMessage(MSG_KEY, "3"), + "48:26: " + getCheckMessage(MSG_KEY, "1.5"), + "50:22: " + getCheckMessage(MSG_KEY, "3"), + "50:29: " + getCheckMessage(MSG_KEY, "5"), + "50:37: " + getCheckMessage(MSG_KEY, "3"), + "54:26: " + getCheckMessage(MSG_KEY, "3"), + "55:39: " + getCheckMessage(MSG_KEY, "3"), + "60:25: " + getCheckMessage(MSG_KEY, "010"), + "61:25: " + getCheckMessage(MSG_KEY, "011"), + "63:30: " + getCheckMessage(MSG_KEY, "0_10L"), + "64:30: " + getCheckMessage(MSG_KEY, "011l"), + "68:24: " + getCheckMessage(MSG_KEY, "0x10"), + "69:24: " + getCheckMessage(MSG_KEY, "0X011"), + "71:29: " + getCheckMessage(MSG_KEY, "0x10L"), + "72:29: " + getCheckMessage(MSG_KEY, "0X11l"), + "85:28: " + getCheckMessage(MSG_KEY, "3"), + "92:14: " + getCheckMessage(MSG_KEY, "0xffffffffL"), + "100:30: " + getCheckMessage(MSG_KEY, "+3"), + "101:29: " + getCheckMessage(MSG_KEY, "-2"), + "102:35: " + getCheckMessage(MSG_KEY, "+3.5"), + "103:36: " + getCheckMessage(MSG_KEY, "-2.5"), + "111:35: " + getCheckMessage(MSG_KEY, "0x80000000"), + "112:36: " + getCheckMessage(MSG_KEY, "0x8000000000000000L"), + "115:37: " + getCheckMessage(MSG_KEY, "020000000000"), + "116:38: " + getCheckMessage(MSG_KEY, "01000000000000000000000L"), + "131:20: " + getCheckMessage(MSG_KEY, "378"), + "140:52: " + getCheckMessage(MSG_KEY, "27"), + "143:53: " + getCheckMessage(MSG_KEY, "3"), + "143:56: " + getCheckMessage(MSG_KEY, "3"), + "143:59: " + getCheckMessage(MSG_KEY, "3"), + "143:62: " + getCheckMessage(MSG_KEY, "3"), + "160:16: " + getCheckMessage(MSG_KEY, "31"), + "165:16: " + getCheckMessage(MSG_KEY, "42"), + "170:16: " + getCheckMessage(MSG_KEY, "13"), + "174:15: " + getCheckMessage(MSG_KEY, "21"), + "178:15: " + getCheckMessage(MSG_KEY, "37"), + "182:15: " + getCheckMessage(MSG_KEY, "101"), + "185:26: " + getCheckMessage(MSG_KEY, "42"), + "189:32: " + getCheckMessage(MSG_KEY, "43"), + "193:26: " + getCheckMessage(MSG_KEY, "-44"), + "197:32: " + getCheckMessage(MSG_KEY, "-45"), + "209:63: " + getCheckMessage(MSG_KEY, "62"), + }; + verify(checkConfig, getPath("InputMagicNumber.java"), expected); + } } diff --git a/src/test/resources/com/puppycrawl/tools/checkstyle/InputMagicNumber.java b/src/test/resources/com/puppycrawl/tools/checkstyle/InputMagicNumber.java index d029df35e..5d91067ab 100644 --- a/src/test/resources/com/puppycrawl/tools/checkstyle/InputMagicNumber.java +++ b/src/test/resources/com/puppycrawl/tools/checkstyle/InputMagicNumber.java @@ -136,10 +136,10 @@ class ComplexAndFlagged class ComplexButNotFlagged { // according to user feedback this is typical code that should not be flagged - public static final double SPECIAL_SUM = 2 + 1e10, SPECIAL_DIFFERENCE = 4 - java.lang.Math.PI; - public static final Integer DEFAULT_INT = new Integer(27); - public static final int SECS_PER_DAY = 24 * 60 * 60, SPECIAL_RATIO = 4 / 3; - public static final javax.swing.border.Border STD_BORDER = + public final double SpecialSum = 2 + 1e10, SpecialDifference = 4 - java.lang.Math.PI; + public final Integer DefaultInit = new Integer(27); + public final int SpecsPerDay = 24 * 60 * 60, SpecialRatio = 4 / 3; + public final javax.swing.border.Border StdBorder = javax.swing.BorderFactory.createEmptyBorder(3, 3, 3, 3); } @@ -198,3 +198,14 @@ class TestHashCodeMethod { public void anotherNegative2() { } } + +class TestMethodCall { + + public TestMethodCall(int x){ + + } + + public void method2() { + final TestMethodCall dummyObject = new TestMethodCall(62); + } +} diff --git a/src/xdocs/config_coding.xml b/src/xdocs/config_coding.xml index 23d6bdd94..6bbc788b2 100644 --- a/src/xdocs/config_coding.xml +++ b/src/xdocs/config_coding.xml @@ -991,6 +991,13 @@ static final Integer ANSWER_TO_THE_ULTIMATE_QUESTION_OF_LIFE = new Integer(42); boolean false + + constantWaiverParentToken + Token that are allowed in the AST path from the number literal to the enclosing constant definition. + subset of tokens + ASSIGN, ARRAY_INIT, EXPR, UNARY_PLUS, UNARY_MINUS, TYPECAST, ELIST, STAR, DIV, PLUS, + MINUS, LITERAL_NEW, METHOD_CALL + @@ -1042,8 +1049,35 @@ class MyClass { } } +

+ Config Example for constantWaiverParentToken Option: +

+ + <module name="MagicNumber"> + <property name="constantWaiverParentToken" value="ASSIGN,ARRAY_INIT,EXPR, + UNARY_PLUS, UNARY_MINUS, TYPECAST, ELIST, DIV, PLUS "/> + </module> + +

result is following violation:

+ + class TestMethodCall { + public void method2() { + final TestMethodCall dummyObject = new TestMethodCall(62); //violation + final int a = 3; // ok as waiver is ASSIGN + final int [] b = {4, 5} // ok as waiver is ARRAY_INIT + final int c = -3; // ok as waiver is UNARY_MINUS + final int d = +4; // ok as waiver is UNARY_PLUS + final int e = method(1, 2) // ELIST is there but violation due to METHOD_CALL + final int x = 3 * 4; // violation + final int y = 3 / 4; // ok as waiver is DIV + final int z = 3 + 4; // ok as waiver is PLUS + final int w = 3 - 4; // violation + final int x = (int)(3.4); //ok as waiver is TYPECAST + } + } + - +