Android Recycle Rule. Closes #212#225
Conversation
|
|
||
| @Override | ||
| public boolean visit(Assignment node) { | ||
| if (ASTHelper.isSameLocalVariable(node.getLeftHandSide(), cursorSimpleName)) { |
There was a problem hiding this comment.
Please change all uses of ASTHelper methods and fields to use static imports.
|
|
||
| private static boolean isMethodIgnoringParameters(MethodInvocation node, String typeQualifiedName, | ||
| String[] methodNames) { | ||
| boolean isSameMethod; |
There was a problem hiding this comment.
Please move declaration to line 88.
| } | ||
|
|
||
| private static boolean isMethodIgnoringParameters(MethodInvocation node, String typeQualifiedName, | ||
| String[] methodNames) { |
| if (isMethodIgnoringParameters( | ||
| node, | ||
| "android.database.sqlite.SQLiteDatabase", | ||
| new String[]{"query", "rawQuery", "queryWithFactory", "rawQueryWithFactory"}) |
There was a problem hiding this comment.
After moving to variable arguments, I think you can get rid of this new String[] { ?
There was a problem hiding this comment.
And the other ones in this method down below.
| String recycleMethodName = methodNameToCleanupResource(node); | ||
| if (recycleMethodName != null) { | ||
| SimpleName cursorExpression = null; | ||
| ASTNode variableAssignmentNode = null; |
There was a problem hiding this comment.
Please do not assign null:
SimpleName cursorExpression;
ASTNode variableAssignmentNode;| if (!closePresenceChecker.isClosePresent()) { | ||
| final ASTBuilder b = this.ctx.getASTBuilder(); | ||
| MethodInvocation closeInvocation = b.invoke(b.copy(cursorExpression), recycleMethodName); | ||
| ExpressionStatement expressionStatement = b.getAST().newExpressionStatement(closeInvocation); |
There was a problem hiding this comment.
Move these 3 lines inside the then block of the next if statement.
| * @param recycleMethodName Recycle method name | ||
| */ | ||
| public ClosePresenceChecker(SimpleName cursorSimpleName, String recycleMethodName) { | ||
| super(); |
| */ | ||
| public ClosePresenceChecker(SimpleName cursorSimpleName, String recycleMethodName) { | ||
| super(); | ||
| this.closePresent = false; |
There was a problem hiding this comment.
Default value. Please remove.
| public ClosePresenceChecker(SimpleName cursorSimpleName, String recycleMethodName) { | ||
| super(); | ||
| this.closePresent = false; | ||
| this.lastCursorUse = null; |
There was a problem hiding this comment.
Default value. Please remove.
|
|
||
| /** | ||
| * @return the closePresent | ||
| */ |
| @@ -0,0 +1,124 @@ | |||
| package org.autorefactor.refactoring.rules.samples_in; | |||
There was a problem hiding this comment.
Please add correct copyright.
| @@ -0,0 +1,141 @@ | |||
| package org.autorefactor.refactoring.rules.samples_out; | |||
There was a problem hiding this comment.
Please add correct copyright.
| import org.eclipse.jdt.core.dom.Statement; | ||
| import org.eclipse.jdt.core.dom.VariableDeclarationFragment; | ||
| import static org.autorefactor.refactoring.ASTHelper.*; | ||
| import static org.eclipse.jdt.core.dom.ASTNode.*; |
| public String getDescription() { | ||
| return "Many resources, such as TypedArrays, VelocityTrackers, etc., should be " | ||
| + "recycled (with a recycle()/close() call) after use. " + "Inspired from " | ||
| + "https://android.googlesource.com/platform/tools/base/+/master/lint/libs/lint-checks/src/main/" |
There was a problem hiding this comment.
Why is this displayed to the user?
| * TODO (low priority) check whether resources are being used after release. | ||
| * TODO add support for FragmentTransaction.beginTransaction(). It can use method | ||
| * chaining (which means local variable might not be present) and it can be released | ||
| * by two methods: commit() and commitAllowingStateLoss(). |
There was a problem hiding this comment.
There is a bit of work left :)
|
|
||
| @Override | ||
| public String getName() { | ||
| return "RecycleRefactoring"; |
There was a problem hiding this comment.
Again this is displayed to the user "Android recycle" would be better.
| final ASTBuilder b = this.ctx.getASTBuilder(); | ||
| final Refactorings r = this.ctx.getRefactorings(); | ||
| MethodInvocation closeInvocation = b.invoke(b.copy(cursorExpression), recycleMethodName); | ||
| ExpressionStatement expressionStatement = b.getAST().newExpressionStatement(closeInvocation); |
| return "Many resources, such as TypedArrays, VelocityTrackers, etc., should be " | ||
| + "recycled (with a recycle()/close() call) after use. " + "Inspired from " | ||
| + "https://android.googlesource.com/platform/tools/base/+/master/lint/libs/lint-checks/src/main/" | ||
| + "java/com/android/tools/lint/checks/CleanupDetector.java"; |
There was a problem hiding this comment.
Code in this class is a lot nicer than CleanupDetector.java.
| "ROUTE_ID=?", | ||
| new String[]{Long.toString(route_id)}, | ||
| null, null, null); | ||
| cursor.close(); |
There was a problem hiding this comment.
I am very surprised this is not closed in a try / finally block.
Any reason for that?
Same comment for all others down below.
There was a problem hiding this comment.
I don't know the reason but it is not common in Android db queries:
https://www.codota.com/android/methods/android.database.sqlite.SQLiteDatabase/query
There was a problem hiding this comment.
Apparently they are all bad practice:
- http://stackoverflow.com/questions/4144328/idiom-to-close-a-cursor
- http://stackoverflow.com/questions/12950725/how-to-properly-close-a-cursor-in-android
Phew! I was worried for a second.
I think AutoRefactor should do the right thing here, i.e. use a try / finally block.
3e0fe43 to
90aac00
Compare
e9111cd to
d11ddab
Compare
a1b70c6 to
cc358af
Compare
b87707a to
74160ad
Compare
2f35a94 to
e5cce10
Compare
4f9c5c6 to
86bde66
Compare
No description provided.