Android Recycle Rule. Closes #212 by luiscruz · Pull Request #225 · JnRouvignac/AutoRefactor

luiscruz · 2016-11-01T12:43:07Z

No description provided.

JnRouvignac · 2016-11-02T15:05:05Z

plugin/src/main/java/org/autorefactor/refactoring/rules/AndroidRecycleRefactoring.java

+
+        @Override
+        public boolean visit(Assignment node) {
+            if (ASTHelper.isSameLocalVariable(node.getLeftHandSide(), cursorSimpleName)) {


Please change all uses of ASTHelper methods and fields to use static imports.

JnRouvignac · 2016-11-02T23:51:52Z

plugin/src/main/java/org/autorefactor/refactoring/rules/AndroidRecycleRefactoring.java

+
+    private static boolean isMethodIgnoringParameters(MethodInvocation node, String typeQualifiedName,
+            String[] methodNames) {
+        boolean isSameMethod;


Please move declaration to line 88.

JnRouvignac · 2016-11-02T23:52:23Z

plugin/src/main/java/org/autorefactor/refactoring/rules/AndroidRecycleRefactoring.java

+    }
+
+    private static boolean isMethodIgnoringParameters(MethodInvocation node, String typeQualifiedName,
+            String[] methodNames) {


String[] => String...

JnRouvignac · 2016-11-02T23:52:52Z

plugin/src/main/java/org/autorefactor/refactoring/rules/AndroidRecycleRefactoring.java

+        if (isMethodIgnoringParameters(
+                node,
+                "android.database.sqlite.SQLiteDatabase",
+                new String[]{"query", "rawQuery", "queryWithFactory", "rawQueryWithFactory"})


After moving to variable arguments, I think you can get rid of this new String[] { ?

And the other ones in this method down below.

JnRouvignac · 2016-11-02T23:55:28Z

plugin/src/main/java/org/autorefactor/refactoring/rules/AndroidRecycleRefactoring.java

+        String recycleMethodName = methodNameToCleanupResource(node);
+        if (recycleMethodName != null) {
+            SimpleName cursorExpression = null;
+            ASTNode variableAssignmentNode = null;


Please do not assign null:

SimpleName cursorExpression; ASTNode variableAssignmentNode;

JnRouvignac · 2016-11-02T23:56:35Z

plugin/src/main/java/org/autorefactor/refactoring/rules/AndroidRecycleRefactoring.java

+            if (!closePresenceChecker.isClosePresent()) {
+                final ASTBuilder b = this.ctx.getASTBuilder();
+                MethodInvocation closeInvocation = b.invoke(b.copy(cursorExpression), recycleMethodName);
+                ExpressionStatement expressionStatement = b.getAST().newExpressionStatement(closeInvocation);


Move these 3 lines inside the then block of the next if statement.

JnRouvignac · 2016-11-02T23:56:56Z

plugin/src/main/java/org/autorefactor/refactoring/rules/AndroidRecycleRefactoring.java

+         * @param recycleMethodName Recycle method name
+         */
+        public ClosePresenceChecker(SimpleName cursorSimpleName, String recycleMethodName) {
+            super();


Please remove.

JnRouvignac · 2016-11-02T23:57:16Z

plugin/src/main/java/org/autorefactor/refactoring/rules/AndroidRecycleRefactoring.java

+         */
+        public ClosePresenceChecker(SimpleName cursorSimpleName, String recycleMethodName) {
+            super();
+            this.closePresent = false;


Default value. Please remove.

JnRouvignac · 2016-11-02T23:57:21Z

plugin/src/main/java/org/autorefactor/refactoring/rules/AndroidRecycleRefactoring.java

+        public ClosePresenceChecker(SimpleName cursorSimpleName, String recycleMethodName) {
+            super();
+            this.closePresent = false;
+            this.lastCursorUse = null;


Default value. Please remove.

JnRouvignac · 2016-11-02T23:57:31Z

plugin/src/main/java/org/autorefactor/refactoring/rules/AndroidRecycleRefactoring.java

+
+        /**
+         * @return the closePresent
+         */


Please remove empty javadoc.

JnRouvignac · 2016-11-04T16:05:01Z

samples/src/test/java/org/autorefactor/refactoring/rules/samples_in/AndroidRecycleSample.java

@@ -0,0 +1,124 @@
+package org.autorefactor.refactoring.rules.samples_in;


Please add correct copyright.

JnRouvignac · 2016-11-04T16:05:08Z

samples/src/test/java/org/autorefactor/refactoring/rules/samples_out/AndroidRecycleSample.java

@@ -0,0 +1,141 @@
+package org.autorefactor.refactoring.rules.samples_out;


Please add correct copyright.

JnRouvignac · 2016-11-15T22:44:55Z

plugin/src/main/java/org/autorefactor/refactoring/rules/AndroidRecycleRefactoring.java

+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.*;


Unused import :)

JnRouvignac · 2016-11-15T22:46:55Z

plugin/src/main/java/org/autorefactor/refactoring/rules/AndroidRecycleRefactoring.java

+    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/"


Why is this displayed to the user?

JnRouvignac · 2016-11-15T22:48:16Z

plugin/src/main/java/org/autorefactor/refactoring/rules/AndroidRecycleRefactoring.java

+ * 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 is a bit of work left :)

JnRouvignac · 2016-11-15T22:50:05Z

plugin/src/main/java/org/autorefactor/refactoring/rules/AndroidRecycleRefactoring.java

+
+    @Override
+    public String getName() {
+        return "RecycleRefactoring";


Again this is displayed to the user "Android recycle" would be better.

JnRouvignac · 2016-11-15T23:20:37Z

plugin/src/main/java/org/autorefactor/refactoring/rules/AndroidRecycleRefactoring.java

+                    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);


b.toStmt()

JnRouvignac · 2016-11-15T23:23:13Z

plugin/src/main/java/org/autorefactor/refactoring/rules/AndroidRecycleRefactoring.java

+        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";


Code in this class is a lot nicer than CleanupDetector.java.

JnRouvignac · 2016-11-15T23:28:33Z

samples/src/test/java/org/autorefactor/refactoring/rules/samples_out/AndroidRecycleSample.java

+                "ROUTE_ID=?",
+                new String[]{Long.toString(route_id)},
+                null, null, null);
+        cursor.close();


I am very surprised this is not closed in a try / finally block.
Any reason for that?

Same comment for all others down below.

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

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.

JnRouvignac reviewed Nov 2, 2016

View reviewed changes

luiscruz force-pushed the recycle branch from 7f6cfc3 to 041693d Compare November 2, 2016 15:35

JnRouvignac reviewed Nov 2, 2016

View reviewed changes

luiscruz force-pushed the recycle branch from 041693d to f92cea6 Compare November 4, 2016 11:27

JnRouvignac reviewed Nov 4, 2016

View reviewed changes

JnRouvignac reviewed Nov 15, 2016

View reviewed changes

Android Recycle Rule. Closes JnRouvignac#212

2bb8198

luiscruz force-pushed the recycle branch from ff1e80e to 2bb8198 Compare November 30, 2016 19:01

luiscruz and others added 7 commits December 19, 2016 15:19

android-recycle: test for try-with-resources

7abd438

support for try with resources

cd5effb

recycle: surround cleanup with null validation

aceba59

close resources when they are using in return statement

0a79399

solved case for resources being close in the finally block

fe45531

recycle fixed bug with resource scope

f0405c7

added recycle to early returns

f999342

Fabrice-TIERCELIN force-pushed the master branch 6 times, most recently from aebd2b8 to 0d63a43 Compare September 14, 2019 17:41

Fabrice-TIERCELIN force-pushed the master branch 6 times, most recently from 87b5052 to 67259b6 Compare October 3, 2019 17:57

Fabrice-TIERCELIN force-pushed the master branch 18 times, most recently from a7cdf4a to e446e46 Compare October 7, 2019 16:31

		@@ -0,0 +1,124 @@
		package org.autorefactor.refactoring.rules.samples_in;

		@@ -0,0 +1,141 @@
		package org.autorefactor.refactoring.rules.samples_out;

Conversation

luiscruz commented Nov 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants