“Experience is the name everyone gives to their mistakes.”
Oscar Wilde
Im vorherigen Beitrag zur statischen Code Analyse mit Error Prone wurde die Möglichkeit erwähnt, eigene Bug Pattern prüfen zu lassen. In diesem Beitrag wird nun ein BugChecker implementiert, der ein störendes Bug Pattern erkennt und patchen kann.
if (operation != null) {
log.info("operation");
// a lot of code
} else {
throw new IllegalStateException();
}
Der oben dargestellte Code ist zwar valide, kann unübersichtlich werden, wenn der Then-Zweig der If-Anweisung etwas länger wird. Die einzelne Throw-Anweisung im Else-Zweig kann leicht übersehen werden, sollte aber eigentlich prominent dargestellt werden. Im Beitrag Refactoring mit Guard Clauses wurde dieses Pattern Exceptions als Überraschung genannt.
if (operation == null) {
throw new IllegalStateException();
}
log.info("operation");
// a lot of code
Wird die Bedingung der If-Anweisung negiert, dann kann der bisherige Else-Zweig in den Then-Zweig wandern. Weil nun dort die IllegalStateException geworfen wird, kann das else entfernt werden und der restliche Code im äußeren Block stehen.
Um dieses Bug Pattern zu erkennen, wird zunächst eine eigene Implementierung von BugChecker benötigt.
@AutoService(BugChecker.class)
@BugPattern(name = "ExceptionAsSurprise", severity = SeverityLevel.WARNING
summary = "Throwing an Exception in the else statement is much harder to read."
)
public class ExceptionAsSurprise extends BugChecker implements IfTreeMatcher {
@Override
public Description matchIf(IfTree tree, VisitorState state) {
StatementTree elseStatement = tree.getElseStatement();
if (elseStatement == null) {
return Description.NO_MATCH;
}
if (elseStatement.getKind() == Kind.BLOCK) {
List<? extends StatementTree> statements = ((BlockTree) elseStatement).getStatements();
if (statements.size() == 1 && statements.get(0).getKind() == Kind.THROW) {
return switchThenAndElse(tree, (ThrowTree)statements.get(0), state);
}
} else if (elseStatement.getKind() == Kind.THROW) {
return switchThenAndElse(tree, (ThrowTree)elseStatement, state);
}
return Description.NO_MATCH;
}
}
Die Klasse ExceptionAsSurprise erweitert die Klasse BugChecker und implementiert das Interface IfTreeMatcher. Dadurch erkennt Error Prone, dass sich dieser BugChecker für If-Anweisungen im Source Code interessiert.
In der Methode matchIf erhält der BugChecker die aktuelle If-Anweisung als IfTree Instanz übergeben. Enthält die If-Anweisung keinen Else-Zweig, dann ist die Bearbeitung schon beendet. Im anderen Fall wird geprüft, ob der Else-Zweig eine einzelne Throw-Anweisung ist, oder ein Block mit einer einzelnen Throw-Anweisung. In beiden Fällen wird dann die Methode switchThenAndElse aufgerufen um eine Beschreibung mit einem passenden Fix zu erzeugen.
private Description switchThenAndElse(IfTree tree, ThrowTree throwTree, VisitorState state) {
Description.Builder description = buildDescription(tree);
description.setLinkUrl("https://schegge.de/2018/04/refactoring-mit-guard-clauses/");
description.addFix(SuggestedFix.replace(tree,
"if (" + invert(tree.getCondition()) + ") {\n" + state.getSourceForNode(throwTree) + "\n}\n" +
withoutOuterBlock(tree.getThenStatement(), state)));
return description.build();
}
In der Methode wird mit Hilfe einer Builder Instanz eine Description erzeugt, die als Link zum Bug Pattern, den Link zum Beitrag Refactoring mit Guard Clauses erhält und als Fix eine Ersetzung der bisherigen If-Anweisung durch neuen Code. Der neue Code besteht aus einer If-Anweisung mit invertierter Bedingung, der Throw-Anweisung im Then-Zweig und den Inhalt des bisherigen Then-Zweigs als Anweisungen hinter der If-Anweisung.
Der bisherige Then-Zweig wird in Source Code umgewandelt, in dem ein möglicherweise vorhandener umschließender Block mit der Methode withoutOuterBlock entfernt wird.
private String withoutOuterBlock(StatementTree tree, VisitorState state) {
if (tree.getKind() == Kind.EMPTY_STATEMENT) {
return "";
}
if (tree.getKind() != Kind.BLOCK) {
return state.getSourceForNode(tree);
}
StringBuilder builder = new StringBuilder();
for (StatementTree statement : ((BlockTree) tree).getStatements()) {
builder.append(state.getSourceForNode(statement)).append('\n');
}
return builder.toString();
}
Handelt es sich um eine leere Anweisung, dann wird ein leerer String zurückgegeben. In diesem Fall muss nichts in den Source Code eingebaut werden. Bei einem Block werden die enthaltenen Anweisungen konkateniert. Auf diese Weise geht leider die Einrückung der Anweisungen verloren. Eine schönere Lösung bedeutet aber einen noch tieferen Abstieg in die Details des Java Compilers.
Zur Invertierung der Bedingung der If-Anweisung reicht die Negation mit "!(" + tree.getCondition() + ")". Damit erzeugt der Fix aber eine ganze Reihe hässlicher Bedingungen, daher ist die invert Methode etwas intelligenter implementiert.
private String invert(ExpressionTree ifCondition) {
ExpressionTree condition = removeParenthesis(ifCondition);
if (condition.getKind() == Kind.LOGICAL_COMPLEMENT) {
return ((UnaryTree) condition).getExpression().toString();
}
if (condition.getKind() == Kind.BOOLEAN_LITERAL) {
return String.valueOf(!(Boolean) ((LiteralTree) condition).getValue());
}
String operator = REVERSE.get(condition.getKind());
if (operator != null) {
BinaryTree binaryTree = (BinaryTree) condition;
return binaryTree.getLeftOperand() + operator + binaryTree.getRightOperand();
}
operator = DE_MORGAN.get(condition.getKind());
if (operator != null) {
BinaryTree binaryTree = (BinaryTree) condition;
return addComplement(binaryTree.getLeftOperand()) + operator + addComplement(binaryTree.getRightOperand());
}
return addComplement(condition);
}
Im ersten Schritt werden alle Klammern entfernt, in der die Bedingung eingepackt ist. Leider werden beim Umbau von Ausdrücken immer wieder Klammern hinzugefügt aber viel seltener unnötige Klammern gelöscht.
Handelt es sich bei der Bedingung um eine Negation, dann wird der enthaltene Ausdruck als neue Bedingung verwendet. Handelt es sich um einen boolschen Literal, dann wird dieser invertiert. Diese Behandlung ist nur zur Vollständigkeit enthalten, kein If-Anweisungen sollte einen boolschen Literal als Bedingung enthalten.
Handelt es sich bei der Bedingung um einen Vergleich, dann wird in der Map REVERSE nach dem entsprechenden invertierten Vergleich gesucht.
private static final Map<Kind, String> REVERSE = Map.of( Kind.LESS_THAN, " >= ", Kind.LESS_THAN_EQUAL, " > ", Kind.GREATER_THAN, " <= ", Kind.GREATER_THAN_EQUAL, " < ", Kind.EQUAL_TO, " != ", Kind.NOT_EQUAL_TO, " == " );
Auf diese Weise wird ein Ausdruck count > 0 in count <= 0, oder ein Ausdruck map != null in map == null umgewandelt.
Handelt es sich nicht um einen Vergleich, sondern um eine boolsche Verknüpfung mit && oder ||, dann werden die De Morganschen Regeln angewendet. Aus einer Und-Verknüpfung wird eine Oder-Verknüpfung mit negierten Operanden und umgekehrt. Negiert werden die Operanden in der addComplement Methode.
private String addComplement(ExpressionTree condition) {
if (condition.getKind() == Kind.PARENTHESIZED || condition.getKind() == Kind.BOOLEAN_LITERAL || condition.getKind() == Kind.IDENTIFIER) {
return "!" + condition;
}
return "!(" + condition + ")";
}
Handelt es sich um einen geklammerten Ausdruck, ein boolsches Literal oder eine Variable, dann kann der Negations-Operator direkt davor gesetzt werden. In allen anderen Fällen muss der Ausdruck geklammert werden.
Damit der eigene BugChecker verwendet werden kann, muss er für den Java Service Provider Interface Mechanismus sichtbar sein. Dazu wird die Datei META-INF/services/com.google.errorprone.bugpatterns.BugChecker in der eigenen Bibliothek benötigt, die den vollständigen Namen der eigenen Implementierung enthält. Bei einem einzelnen BugChecker kann diese Datei manuell erstellt werden. Bei vielen Implementierungen oder Namensänderungen ist diese Vorgehensweise eher lästig.
Dafür stellt Google das Projekt AutoService mit der gleichnamigen Annotation bereit. Mit @AutoService(BugChecker.class) wird der annotierte Typ in die entsprechende Service Datei eingefügt.
Wird der am Anfang des Beitrags vorgestellte unschöne Code erneut durch den Compiler geschickt, erzeugt dieser nun folgende Meldung:
[WARNING] /rest-markdown/src/main/java/de/schegge/rest/markdown/EndPointUpdater.java:[53,5] [ExceptionAsSurprise] Throwing an Exception in the else statement is much harder to read.
(see https://schegge.de/2018/04/refactoring-mit-guard-clauses/)
Did you mean 'if (operation == null) {'?
Und der folgende Patch wird erstellt.
--- de\schegge\rest\markdown\EndPointUpdater.java
+++ de\schegge\rest\markdown\EndPointUpdater.java
@@ -51,28 +51,27 @@
@Override
public Void visit(Endpoint endpoint, String operation, Void input) {
- if (operation != null) {
- log.info("operation");
- // a lot of code
- } else {
- throw new IllegalStateException();
- }
+ if (operation == null) {
+throw new IllegalStateException();
+}
+log.info("operation");
Damit ist die Implementierung eines BugCheckers für ein eigenes Bug Pattern auch schon beendet. Eigene Bug Pattern helfen dem Team Verstöße gegen die Code Conventionen leichter zu entdecken und sogar automatisiert im Build Prozess zu korrigieren.