Android Recycle Rule. Closes #212 #225

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 3e0fe43 to 90aac00 Compare April 28, 2020 18:28

Fabrice-TIERCELIN force-pushed the master branch 2 times, most recently from e9111cd to d11ddab Compare June 14, 2020 07:44

Fabrice-TIERCELIN force-pushed the master branch 8 times, most recently from a1b70c6 to cc358af Compare October 10, 2020 18:24

Fabrice-TIERCELIN force-pushed the master branch 5 times, most recently from b87707a to 74160ad Compare January 3, 2021 05:34

Fabrice-TIERCELIN force-pushed the master branch 3 times, most recently from 2f35a94 to e5cce10 Compare May 2, 2021 13:19

Fabrice-TIERCELIN force-pushed the master branch 6 times, most recently from 4f9c5c6 to 86bde66 Compare May 28, 2021 06:11

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

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

Android Recycle Rule. Closes #212 #225

Are you sure you want to change the base?

Android Recycle Rule. Closes #212 #225

Uh oh!

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