From 8ffba84906486d998c32fd00d5e7a4227b775b4f Mon Sep 17 00:00:00 2001 From: rnveach Date: Tue, 9 May 2017 15:05:05 -0400 Subject: [PATCH] Pull #565: fixed violations of MoveVariableInsideIfCheck --- .../coding/ConfusingConditionCheck.java | 2 +- .../coding/CustomDeclarationOrderCheck.java | 21 +++++++++---------- .../MapIterationInForEachLoopCheck.java | 10 ++++----- ...ipleVariableDeclarationsExtendedCheck.java | 6 ++++-- .../SimpleAccessorNameNotationCheck.java | 5 ++--- .../PublicReferenceToPrivateTypeCheck.java | 2 +- .../checks/sizes/LineLengthExtendedCheck.java | 3 ++- .../internal/CheckstyleRegressionTest.java | 6 +++--- .../internal/CommitValidationTest.java | 2 +- 9 files changed, 28 insertions(+), 29 deletions(-) diff --git a/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/ConfusingConditionCheck.java b/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/ConfusingConditionCheck.java index c245a75da4..bdc8618893 100644 --- a/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/ConfusingConditionCheck.java +++ b/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/ConfusingConditionCheck.java @@ -231,9 +231,9 @@ private boolean isRatioBetweenIfAndElseBlockSuitable(DetailAST literalIf) { boolean result = true; final DetailAST lastChildAfterIf = literalIf.getLastChild(); - final int linesOfCodeInIfBlock = getAmounOfCodeRowsInBlock(literalIf); final int linesOfCodeInElseBlock = getAmounOfCodeRowsInBlock(lastChildAfterIf); if (linesOfCodeInElseBlock > 0) { + final int linesOfCodeInIfBlock = getAmounOfCodeRowsInBlock(literalIf); result = linesOfCodeInIfBlock / linesOfCodeInElseBlock < multiplyFactorForElseBlocks; } return result; diff --git a/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/CustomDeclarationOrderCheck.java b/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/CustomDeclarationOrderCheck.java index 6df860e705..886a43caad 100644 --- a/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/CustomDeclarationOrderCheck.java +++ b/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/CustomDeclarationOrderCheck.java @@ -390,6 +390,8 @@ public void visitToken(DetailAST ast) { public void leaveToken(DetailAST ast) { if (ast.getType() == TokenTypes.CLASS_DEF && !isClassDefInMethodDef(ast)) { + // -@cs[MoveVariableInsideIf] assignment value is a modification + // call so it can't be moved final ClassDetail classDetail = classDetails.pop(); if (checkGettersSetters) { @@ -687,9 +689,6 @@ private static boolean isSetterName(String methodName) { private boolean isGetterCorrect(DetailAST methodDef, String methodPrefix) { boolean result = false; - final String methodName = getIdentifier(methodDef); - final String methodNameWithoutPrefix = getNameWithoutPrefix(methodName, methodPrefix); - final DetailAST parameters = methodDef.findFirstToken(TokenTypes.PARAMETERS); // no parameters @@ -703,6 +702,9 @@ private boolean isGetterCorrect(DetailAST methodDef, String methodPrefix) { if (returnStatementAst != null) { final DetailAST exprAst = returnStatementAst.getFirstChild(); final String returnedFieldName = getNameOfGetterField(exprAst); + final String methodName = getIdentifier(methodDef); + final String methodNameWithoutPrefix = getNameWithoutPrefix(methodName, + methodPrefix); if (returnedFieldName != null && !localVariableHidesField(statementsAst, returnedFieldName) && verifyFieldAndMethodName(returnedFieldName, @@ -748,15 +750,14 @@ private static boolean localVariableHidesField(DetailAST slist, private boolean isSetterCorrect(DetailAST methodDefAst, String methodPrefix) { boolean result = false; - final String methodName = getIdentifier(methodDefAst); - final String setterFieldName = fieldPrefix - + getNameWithoutPrefix(methodName, methodPrefix); - final DetailAST methodTypeAst = methodDefAst.findFirstToken(TokenTypes.TYPE); if (methodTypeAst.branchContains(TokenTypes.LITERAL_VOID)) { final DetailAST statementsAst = methodDefAst.findFirstToken(TokenTypes.SLIST); + final String methodName = getIdentifier(methodDefAst); + final String setterFieldName = fieldPrefix + + getNameWithoutPrefix(methodName, methodPrefix); result = statementsAst != null && !localVariableHidesField(statementsAst, setterFieldName) @@ -1066,9 +1067,8 @@ private static boolean isMainMethodModifiers(final DetailAST methodAST) { */ private static boolean isVoidType(final DetailAST methodAST) { boolean result = true; - DetailAST methodTypeAST = null; if (hasChildToken(methodAST, TokenTypes.TYPE)) { - methodTypeAST = methodAST.findFirstToken(TokenTypes.TYPE); + final DetailAST methodTypeAST = methodAST.findFirstToken(TokenTypes.TYPE); result = hasChildToken(methodTypeAST, TokenTypes.LITERAL_VOID); } return result; @@ -1343,9 +1343,8 @@ else if (isBooleanGetterName(getterName)) { // method is NOT getter final DetailAST setterAst = allGettersSetters.get(j); final String setterName = getIdentifier(setterAst); - String setterField = null; if (isSetterName(setterName)) { - setterField = getNameWithoutPrefix( + final String setterField = getNameWithoutPrefix( getIdentifier(setterAst), SETTER_PREFIX); // if fields are same and setter is sibling with getter diff --git a/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/MapIterationInForEachLoopCheck.java b/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/MapIterationInForEachLoopCheck.java index cddf7d38f5..9e7c096c94 100644 --- a/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/MapIterationInForEachLoopCheck.java +++ b/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/MapIterationInForEachLoopCheck.java @@ -330,14 +330,10 @@ private String validate(DetailAST forLiteralNode) { final DetailAST forEachNode = forLiteralNode.findFirstToken(TokenTypes.FOR_EACH_CLAUSE); final DetailAST keySetOrEntrySetNode = getKeySetOrEntrySetNode(forEachNode); - boolean isMapClassField = false; // Search for keySet or entrySet if (keySetOrEntrySetNode != null) { - if (keySetOrEntrySetNode.getPreviousSibling().getChildCount() != 0) { - isMapClassField = true; - } - final DetailAST variableDefNode = forEachNode.getFirstChild(); - final String keyOrEntryVariableName = variableDefNode.getLastChild().getText(); + final boolean isMapClassField = keySetOrEntrySetNode.getPreviousSibling() + .getChildCount() != 0; final String currentMapVariableName; @@ -351,6 +347,8 @@ private String validate(DetailAST forLiteralNode) { final DetailAST forEachOpeningBrace = forLiteralNode.getLastChild(); if (!isMapPassedIntoAnyMethod(forEachOpeningBrace)) { + final DetailAST variableDefNode = forEachNode.getFirstChild(); + final String keyOrEntryVariableName = variableDefNode.getLastChild().getText(); if (proposeKeySetUsage && KEY_SET_METHOD_NAME.equals( diff --git a/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/MultipleVariableDeclarationsExtendedCheck.java b/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/MultipleVariableDeclarationsExtendedCheck.java index f15daee212..ba1995b267 100644 --- a/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/MultipleVariableDeclarationsExtendedCheck.java +++ b/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/MultipleVariableDeclarationsExtendedCheck.java @@ -98,10 +98,12 @@ public int[] getDefaultTokens() { public void work(DetailAST ast) { DetailAST nextNode = ast.getNextSibling(); - final boolean isCommaSeparated = (nextNode != null) - && (nextNode.getType() == TokenTypes.COMMA); if (nextNode != null) { + // -@cs[MoveVariableInsideIf] assignment value is modified later so + // it can't be moved + final boolean isCommaSeparated = nextNode.getType() == TokenTypes.COMMA; + if ((nextNode.getType() == TokenTypes.COMMA) || (nextNode.getType() == TokenTypes.SEMI)) { nextNode = nextNode.getNextSibling(); diff --git a/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/SimpleAccessorNameNotationCheck.java b/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/SimpleAccessorNameNotationCheck.java index 2ebd9db589..00ffae7386 100644 --- a/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/SimpleAccessorNameNotationCheck.java +++ b/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/coding/SimpleAccessorNameNotationCheck.java @@ -93,8 +93,8 @@ public int[] getDefaultTokens() { @Override public void visitToken(DetailAST methodDef) { - final String methodName = methodDef.findFirstToken(TokenTypes.IDENT).getText(); if (hasBody(methodDef) && !isMethodAtAnonymousClass(methodDef)) { + final String methodName = methodDef.findFirstToken(TokenTypes.IDENT).getText(); if (methodName.startsWith(BOOLEAN_GETTER_PREFIX)) { if (!isGetterCorrect(methodDef, methodName.substring(BOOLEAN_GETTER_PREFIX.length()))) { @@ -224,10 +224,9 @@ private static String getNameOfSettingField(DetailAST assign, DetailAST parameters) { String nameOfSettingField = null; - final DetailAST assigningFirstChild = assign.getFirstChild(); - if (assign.getChildCount() == 2 && assign.getLastChild().getType() == TokenTypes.IDENT) { + final DetailAST assigningFirstChild = assign.getFirstChild(); if (assigningFirstChild.getType() == TokenTypes.IDENT) { diff --git a/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/design/PublicReferenceToPrivateTypeCheck.java b/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/design/PublicReferenceToPrivateTypeCheck.java index a8d13ff430..3ecf6ca03d 100644 --- a/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/design/PublicReferenceToPrivateTypeCheck.java +++ b/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/design/PublicReferenceToPrivateTypeCheck.java @@ -214,11 +214,11 @@ private void addExternallyAccessibleFieldTypes(DetailAST fieldDefAst) { */ private static List getMethodParameterTypes(DetailAST parametersDefAst) { - DetailAST parameterType = null; final List parameterTypes = new ArrayList<>(); if (parametersDefAst.getFirstChild() != null) { DetailAST currentNode = parametersDefAst; + DetailAST parameterType = null; while (currentNode != null) { if (currentNode.getType() == TokenTypes.PARAMETER_DEF) { diff --git a/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/sizes/LineLengthExtendedCheck.java b/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/sizes/LineLengthExtendedCheck.java index 404b4909a7..602627382d 100644 --- a/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/sizes/LineLengthExtendedCheck.java +++ b/sevntu-checks/src/main/java/com/github/sevntu/checkstyle/checks/sizes/LineLengthExtendedCheck.java @@ -194,11 +194,12 @@ public int[] getDefaultTokens() { @Override public void visitToken(DetailAST ast) { - final DetailAST endOfIgnoreLine = ast.findFirstToken(TokenTypes.SLIST); if (ast.getParent() != null && ast.getParent().getType() == TokenTypes.OBJBLOCK || ast.getType() == TokenTypes.CLASS_DEF) { final int mNumberOfLine = ast.getLineNo(); + final DetailAST endOfIgnoreLine = ast.findFirstToken(TokenTypes.SLIST); + if (endOfIgnoreLine == null) { lines[mNumberOfLine - 1] = null; } diff --git a/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/internal/CheckstyleRegressionTest.java b/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/internal/CheckstyleRegressionTest.java index 234335dcb0..9249db4c96 100644 --- a/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/internal/CheckstyleRegressionTest.java +++ b/sevntu-checks/src/test/java/com/github/sevntu/checkstyle/internal/CheckstyleRegressionTest.java @@ -88,9 +88,6 @@ private static void work(File config, File suppression) throws Exception { trimSevntuChecksNotReferenced(configChecks, sevntuChecks); - String configContents = new String(Files.readAllBytes(config.toPath()), UTF_8); - String suppressionContents = new String(Files.readAllBytes(suppression.toPath()), UTF_8); - if (sevntuChecks.size() > 0) { System.out.println("Adding " + sevntuChecks.size() + " missing check(s)"); @@ -113,6 +110,8 @@ private static void work(File config, File suppression) throws Exception { } } + String configContents = new String(Files.readAllBytes(config.toPath()), UTF_8); + int treeWalkerPosition = configContents.lastIndexOf(""); treeWalkerPosition = configContents.indexOf('\n', treeWalkerPosition) + 1; @@ -122,6 +121,7 @@ private static void work(File config, File suppression) throws Exception { Files.write(config.toPath(), configContents.getBytes(UTF_8), StandardOpenOption.CREATE); if (!suppressionAdditions.isEmpty()) { + String suppressionContents = new String(Files.readAllBytes(suppression.toPath()), UTF_8); final int position = suppressionContents.lastIndexOf("