fix chained if check, improve tests and improve suggestion Feuermagie…

…r#631 Feuermagier#649
Luro02 · Dec 9, 2024 · 60ff2a0 · 60ff2a0
1 parent 294210a
commit 60ff2a0
Show file tree

Hide file tree

Showing 4 changed files with 416 additions and 117 deletions.
diff --git a/...rader-core/src/main/java/de/firemage/autograder/core/check/complexity/ChainedIfCheck.java b/...rader-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<CtStatement> 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
                     );