Skip to content

Commit

Permalink
Merge pull request #653 from Luro02/main
Browse files Browse the repository at this point in the history 
Implement some things
  • Loading branch information
@Luro02
Luro02 authored Dec 10, 2024
2 parents 1c76978 + d0067bc commit d58af9b
Show file tree
Hide file tree
Showing 28 changed files with 688 additions and 625 deletions.
5 changes: 4 additions & 1 deletion autograder-core/src/main/java/de/firemage/autograder/core/check/api/UseEntrySet.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@

@ExecutableCheck(reportedProblems = {ProblemType.USE_ENTRY_SET})
public class UseEntrySet extends IntegratedCheck {
// If a map is iterated over and the key is used to get the value more than MINIMUM_GET_CALLS times, it suggests using entrySet
private static final int MINIMUM_GET_CALLS = 3;

private static boolean hasInvokedKeySet(CtInvocation<?> ctInvocation) {
return ctInvocation.getTarget() != null
&& ctInvocation.getExecutable() != null
Expand Down Expand Up @@ -66,7 +69,7 @@ public void process(CtForEach ctForEach) {
&& ctVariableAccess.getVariable().equals(loopVariable.getReference()))
.toList();

if (!invocations.isEmpty()) {
if (invocations.size() >= MINIMUM_GET_CALLS) {
addLocalProblem(
ctForEach.getExpression(),
new LocalizedMessage(
Expand Down
49 changes: 44 additions & 5 deletions ...rader-core/src/main/java/de/firemage/autograder/core/check/complexity/ChainedIfCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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(
Expand All @@ -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;
Expand All @@ -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
);
Expand Down
2 changes: 1 addition & 1 deletion ...ore/src/main/java/de/firemage/autograder/core/check/complexity/RedundantBooleanEqual.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ private void reportProblem(CtBinaryOperator<?> ctBinaryOperator, boolean literal
addLocalProblem(
ctBinaryOperator,
new LocalizedMessage(
"redundant-boolean-equal",
"common-reimplementation",
Map.of(
"suggestion", suggestion
)
Expand Down
22 changes: 11 additions & 11 deletions ...er-core/src/main/java/de/firemage/autograder/core/check/complexity/RedundantVariable.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ private boolean isComplexExpression(CtExpression<?> ctExpression) {
return ctExpression instanceof CtSwitchExpression<?,?> || ctExpression.toString().length() > MAX_EXPRESSION_SIZE;
}

private void checkVariableRead(CtStatement ctStatement, CtVariableRead<?> ctVariableRead, CodeModel model) {
private void checkVariableRead(CtStatement ctStatement, CtVariableRead<?> ctVariableRead) {
if (// the variable must be a local variable
!(ctVariableRead.getVariable().getDeclaration() instanceof CtLocalVariable<?> ctLocalVariable)
// it should not have any annotations (e.g. @SuppressWarnings("unchecked"))
Expand Down Expand Up @@ -82,19 +82,19 @@ private void checkVariableRead(CtStatement ctStatement, CtVariableRead<?> ctVari
protected void check(StaticAnalysis staticAnalysis) {
staticAnalysis.getModel().getRootPackage().accept(new CtScanner() {
@Override
public <T> void visitCtInvocation(CtInvocation<T> ctInvocation) {
if (!ctInvocation.getPosition().isValidPosition()
|| ctInvocation.isImplicit()
// only check invocations with a single variable
|| ctInvocation.getArguments().size() != 1
|| !(ctInvocation.getArguments().get(0) instanceof CtVariableRead<?> ctVariableRead)) {
super.visitCtInvocation(ctInvocation);
public <T> void visitCtLocalVariable(CtLocalVariable<T> ctLocalVariable) {
if (!ctLocalVariable.getPosition().isValidPosition()
|| ctLocalVariable.isImplicit()
// only check local variables with a default expression
|| ctLocalVariable.getDefaultExpression() == null
|| !(ctLocalVariable.getDefaultExpression() instanceof CtVariableRead<?> ctVariableRead)) {
super.visitCtLocalVariable(ctLocalVariable);
return;
}

checkVariableRead(ctInvocation, ctVariableRead, staticAnalysis.getCodeModel());
checkVariableRead(ctLocalVariable, ctVariableRead);

super.visitCtInvocation(ctInvocation);
super.visitCtLocalVariable(ctLocalVariable);
}

@Override
Expand All @@ -107,7 +107,7 @@ public <T> void visitCtReturn(CtReturn<T> ctReturn) {
return;
}

checkVariableRead(ctReturn, ctVariableRead, staticAnalysis.getCodeModel());
checkVariableRead(ctReturn, ctVariableRead);

super.visitCtReturn(ctReturn);
}
Expand Down
8 changes: 4 additions & 4 deletions ...ore/src/main/java/de/firemage/autograder/core/check/complexity/UseOperatorAssignment.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import de.firemage.autograder.core.check.ExecutableCheck;
import de.firemage.autograder.core.integrated.IntegratedCheck;
import de.firemage.autograder.core.integrated.StaticAnalysis;
import de.firemage.autograder.core.integrated.TypeUtil;
import de.firemage.autograder.core.integrated.evaluator.OperatorHelper;
import spoon.processing.AbstractProcessor;
import spoon.reflect.code.BinaryOperatorKind;
Expand Down Expand Up @@ -39,8 +40,7 @@ public class UseOperatorAssignment extends IntegratedCheck {
private boolean isCommutativeType(CtTypedElement<?> ctTypedElement) {
return ctTypedElement.getType() == null
|| NON_COMMUTATIVE_TYPES.stream()
.map(ty -> ctTypedElement.getFactory().Type().createReference(ty))
.noneMatch(ty -> ty.equals(ctTypedElement.getType()));
.noneMatch(ty -> TypeUtil.isTypeEqualTo(ctTypedElement.getType(), ty));
}

private boolean isCommutative(BinaryOperatorKind binaryOperatorKind) {
Expand Down Expand Up @@ -106,8 +106,8 @@ public void process(CtAssignment<?, ?> assignment) {
addLocalProblem(
assignment,
new LocalizedMessage(
"use-operator-assignment-exp",
Map.of("simplified", simplifiedExpr)
"common-reimplementation",
Map.of("suggestion", simplifiedExpr)
),
ProblemType.USE_OPERATOR_ASSIGNMENT
);
Expand Down
2 changes: 1 addition & 1 deletion autograder-core/src/main/java/de/firemage/autograder/core/check/debug/AssertCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ protected void check(StaticAnalysis staticAnalysis) {
staticAnalysis.processWith(new AbstractProcessor<CtAssert<?>>() {
@Override
public void process(CtAssert<?> element) {
addLocalProblem(element, new LocalizedMessage("assert-used-exp"), ProblemType.ASSERT);
addLocalProblem(element, new LocalizedMessage("assert-used"), ProblemType.ASSERT);
}
});
}
Expand Down
8 changes: 7 additions & 1 deletion ...grader-core/src/main/java/de/firemage/autograder/core/check/general/DoNotUseRawTypes.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import de.firemage.autograder.core.integrated.IntegratedCheck;
import de.firemage.autograder.core.integrated.StaticAnalysis;
import spoon.processing.AbstractProcessor;
import spoon.reflect.code.CtExpression;
import spoon.reflect.declaration.CtType;
import spoon.reflect.path.CtRole;
import spoon.reflect.reference.CtTypeReference;
Expand All @@ -27,8 +28,13 @@ private boolean isRawType(CtTypeReference<?> ctTypeReference) {
return false;
}

// ignore types in expressions like 'new ArrayList()'
if (ctTypeReference.getParent(CtExpression.class) != null) {
return false;
}

return declaration.getFormalCtTypeParameters().size() != ctTypeReference.getActualTypeArguments().size();
}
} //

@Override
protected void check(StaticAnalysis staticAnalysis) {
Expand Down
7 changes: 5 additions & 2 deletions ...ader-core/src/main/java/de/firemage/autograder/core/check/general/StringCompareCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,11 @@ public void process(CtBinaryOperator<?> operator) {

if (isStringComparison(lhs, rhs)) {
addLocalProblem(operator, new LocalizedMessage(
"string-cmp-exp",
Map.of("lhs", lhs, "rhs", rhs)
"suggest-replacement",
Map.of(
"original", operator.toString(),
"replacement", "%s.equals(%s)".formatted(lhs, rhs)
)
),
ProblemType.STRING_COMPARE_BY_REFERENCE
);
Expand Down
111 changes: 0 additions & 111 deletions autograder-core/src/main/java/de/firemage/autograder/core/check/general/UseGuardClauses.java
View file

This file was deleted.

2 changes: 2 additions & 0 deletions ...ain/java/de/firemage/autograder/core/check/naming/ConstantsHaveDescriptiveNamesCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@ private static List<String> listCharOptions(char c) {
case '_' -> List.of("underscore", "dash", "line");
case '/', '\\' -> List.of("slash", "backslash");
case '[', ']' -> List.of("bracket");
case '*' -> List.of("star", "asterisk");
case '+' -> List.of("plus");
default -> Character.isAlphabetic(c) ? List.of(String.valueOf(Character.toLowerCase(c))) : null;
};
}
Expand Down
Loading

0 comments on commit d58af9b

Please sign in to comment.