diff --git a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/ChainedIfCheck.java b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/ChainedIfCheck.java index 75fdb9b0..919a34dd 100644 --- a/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/ChainedIfCheck.java +++ b/autograder-core/src/main/java/de/firemage/autograder/core/check/complexity/ChainedIfCheck.java @@ -9,6 +9,7 @@ import de.firemage.autograder.core.integrated.StaticAnalysis; import spoon.processing.AbstractProcessor; import spoon.reflect.code.CtBlock; +import spoon.reflect.code.CtExpression; import spoon.reflect.code.CtIf; import spoon.reflect.code.CtStatement; @@ -27,12 +28,48 @@ public void process(CtIf ctIf) { return; } + // We can combine a nested if with the parent if, when the code looks like this: + // if (a) { + // if (b) { + // ... + // } + // } + // + // ... + // + // Because there is + // - no code before/after the if(b) + // - there is no explicit else branch + // + // If there is an else or else if, we cannot merge the if-statements: + // if (a) { + // if (b) { + // ... + // } + // } else { + // ... + // } + // + // Because the code would behave differently if the condition a is true and b is false. + // + // Technically, one could solve this by adjusting the else to have a condition: + // } else if (a && !b || !a) { + // + // but that is not obvious, which is why it is not suggested. + // check if the if-statement has a nested if: List thenStatements = StatementUtil.getEffectiveStatements(ctIf.getThenStatement()); - if (thenStatements.size() == 1 + if (// a nested if is exactly one statement + thenStatements.size() == 1 + // the statement must be an if-statement && thenStatements.get(0) instanceof CtIf nestedIf + // and that nested if must not have an else branch && (nestedIf.getElseStatement() == null - || StatementUtil.getEffectiveStatements(nestedIf.getElseStatement()).isEmpty())) { + // or if it has one, it should be effectively empty + || StatementUtil.getEffectiveStatements(nestedIf.getElseStatement()).isEmpty()) + // and like described above, there must not be an else branch in the parent if + && ctIf.getElseStatement() == null + ) { addLocalProblem( ctIf.getCondition(), new LocalizedMessage( @@ -49,6 +86,7 @@ public void process(CtIf ctIf) { ); } + // Now check if the else branch has a nested if, which could be merged with the parent if: CtStatement elseStatement = ctIf.getElseStatement(); if (!(elseStatement instanceof CtBlock ctBlock) || ctBlock.getStatements().isEmpty()) { return; @@ -60,11 +98,12 @@ public void process(CtIf ctIf) { } if (statements.get(0) instanceof CtIf ctElseIf && !elseStatement.isImplicit()) { + CtExpression condition = ctElseIf.getCondition(); + addLocalProblem( ctElseIf.getCondition(), - new LocalizedMessage("suggest-replacement", Map.of( - "original", "else {\"{\"} if (...) {\"{\"} ... {\"}\"} {\"}\"}", - "suggestion", "else if (...) {\"{\"} ... {\"}\"}" + new LocalizedMessage("common-reimplementation", Map.of( + "suggestion", "} else if (%s) { ... }".formatted(condition) )), ProblemType.UNMERGED_ELSE_IF ); diff --git a/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestChainedIfCheck.java b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestChainedIfCheck.java new file mode 100644 index 00000000..a295b96b --- /dev/null +++ b/autograder-core/src/test/java/de/firemage/autograder/core/check/complexity/TestChainedIfCheck.java @@ -0,0 +1,372 @@ +package de.firemage.autograder.core.check.complexity; + +import de.firemage.autograder.api.JavaVersion; +import de.firemage.autograder.api.LinterException; +import de.firemage.autograder.core.LocalizedMessage; +import de.firemage.autograder.core.Problem; +import de.firemage.autograder.core.ProblemType; +import de.firemage.autograder.core.check.AbstractCheckTest; +import de.firemage.autograder.core.file.StringSourceInfo; +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class TestChainedIfCheck extends AbstractCheckTest { + private static final List PROBLEM_TYPES = List.of(ProblemType.MERGE_NESTED_IF, ProblemType.UNMERGED_ELSE_IF); + + private void assertEqualsNested(Problem problem, String suggestion) { + assertEquals( + this.linter.translateMessage( + new LocalizedMessage( + "merge-nested-if", + Map.of("suggestion", suggestion) + )), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + private void assertEqualsElseIf(Problem problem, String suggestion) { + assertEquals( + this.linter.translateMessage( + new LocalizedMessage("common-reimplementation", Map.of( + "suggestion", "} else if (%s) { ... }".formatted(suggestion) + )) + ), + this.linter.translateMessage(problem.getExplanation()) + ); + } + + @Test + void testNestedIfsWithImplicitElse() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void main(String[] args) { + if (true) { + if (false) { + } + } + } + } + """ + ), PROBLEM_TYPES); + + + assertEqualsNested(problems.next(), "true && false"); + problems.assertExhausted(); + } + + @Test + void testNestedIfsWithOuterElse() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void main(String[] args) { + if (true) { + if (false) { + System.out.println("A"); + } + } else { + System.out.println("B"); + } + } + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testElseIfWithNestedIfNoElse() throws LinterException, IOException { + // TODO: again think of when this will be a problem with control flow? + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int x, int y) { + if (x == 0) { + System.out.println("A"); + } else if (x == 1) { + if (y == 0) { + System.out.println("B"); + } + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertEqualsNested(problems.next(), "x == 1 && y == 0"); + + problems.assertExhausted(); + } + + @Test + void testElseIfWithNestedIfWithElse() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int x, int y) { + if (x == 0) { + System.out.println("A"); + } else if (x == 1) { + if (y == 0) { + System.out.println("B"); + } + } else { + System.out.println("C"); + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testRegularIfElseIfElseChain() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int x, int y) { + if (x == 0) { + System.out.println("A"); + } else if (x == 1) { + System.out.println("B"); + } else { + System.out.println("C"); + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + problems.assertExhausted(); + } + + @Test + void testElseWithNestedIfAndCode() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int x, int y) { + if (x == 0) { + } else if (x == 1) { + } else { + System.out.println("A"); + if (x == 2) { + System.out.println("B"); + } + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testElseWithNestedIf() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int x, int y) { + if (x == 0) { + } else if (x == 1) { + } else { + if (x == 2) { + System.out.println("B"); + } + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertEqualsElseIf(problems.next(), "x == 2"); + + problems.assertExhausted(); + } + + @Test + void testCombineNestedIf() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int x) { + if (x > 0) { + if (x < 10) { + System.out.println("A"); + } + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + assertEqualsNested(problems.next(), "x > 0 && x < 10"); + + problems.assertExhausted(); + } + + @Test + void testNestedIfWithReturn() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int a) { + if (a <= 0) { + if (a == -3) { + System.out.println("a is -3"); + } + return; + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + + @Test + void testComplicatedIfWithNested() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int a, int b) { + if (a == 1) { + if (b == 2) { + System.out.println("a is 1"); + } + } else if (a == 2) { + if (b == 3) { + throw new IllegalStateException("an error occurred"); + } + System.out.println("a is 2"); + } else if (a == 3) { + if (b == 3) { + System.out.println("a is 3 and b is 3"); + } else { + throw new IllegalStateException("should never happen"); + } + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testCombineNestedIfWithCode() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int x) { + if (x > 0) { + System.out.println("A"); + if (x < 10) { + System.out.println("B"); + } + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + @Test + void testCombineNestedIfTrailingCode() throws LinterException, IOException { + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int x) { + if (x > 0) { + if (x < 10) { + System.out.println("B"); + } + System.out.println("A"); + } + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } + + + @Test + void testIfWithoutBlock() throws LinterException, IOException { + // this resulted in a crash in the past + ProblemIterator problems = this.checkIterator(StringSourceInfo.fromSourceString( + JavaVersion.JAVA_17, + "Test", + """ + public class Test { + public static void callable(int x) { + if (x > 0); + } + + public static void main(String[] args) {} + } + """ + ), PROBLEM_TYPES); + + problems.assertExhausted(); + } +} diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ChainedIfCheck/code/Test.java b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ChainedIfCheck/code/Test.java deleted file mode 100644 index b067f80b..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ChainedIfCheck/code/Test.java +++ /dev/null @@ -1,104 +0,0 @@ -public class Test { - public static void main(String[] args) { - if (true) { /*# not ok #*/ - if (false) { - - } - } - - if (true) { /*# not ok #*/ - if (false) { - - } - } else { - - } - - if (true) { - - } else if (true) { /*# not ok #*/ - if (false) { - - } - } - - if (true) { - - } else if (true) { - - } else { - - } - - if (true) { - - } else if (true) { - - } else { - foo(); /*# ok #*/ - } - - if (true) { - - } else if (true) { - - } else { - foo(); /*# ok #*/ - if (true) { - - } - } - - if (true) { - - } else if (true) { - - } else { - if (true) { /*# not ok #*/ - } - } - - if (true) { - - } else if (true) { - - } else { - if (true) { /*# not ok #*/ - } else { - - } - } - } - - private static void foo() { - - } -} - -// Tests from https://github.com/pmd/pmd/blob/e8dbb54cb5ed682d9dfd4f54895f4bbd73be728e/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/design/xml/CollapsibleIfStatements.xml#L4 -class PmdTests { - void callable(boolean x, boolean y) { - if (x) { /*# not ok #*/ - if (y) { - } - } - - if (x) { /*# ok #*/ - int z = 5; - if (y) { - } - } - - if (x) { /*# ok #*/ - if (y) { - } - int z = 5; - } - } -} - -class CrashChainedIf { - void isTrue(boolean a) { - if (a); /*# ok #*/ - } -} diff --git a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ChainedIfCheck/config.txt b/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ChainedIfCheck/config.txt deleted file mode 100644 index e79ff561..00000000 --- a/autograder-core/src/test/resources/de/firemage/autograder/core/check_tests/ChainedIfCheck/config.txt +++ /dev/null @@ -1,8 +0,0 @@ -complexity.ChainedIfCheck -Avoid else { if () { } } -Test.java:3 -Test.java:9 -Test.java:19 -Test.java:57 -Test.java:66 -Test.java:81