Android DrawAllocation. Closes #211 #227

luiscruz · 2016-11-02T16:38:15Z

No description provided.

JnRouvignac · 2016-11-04T16:02:54Z

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

+ * AutoRefactor - Eclipse plugin to automatically refactor Java code bases.
+ *
+ * Copyright (C) 2013-2016 Jean-Noël Rouvignac - initial API and implementation
+ * Copyright (C) 2016 Fabrice Tiercelin - Make sure we do not visit again modified nodes


Neither Fabrice nor I have worked on this yet :)

JnRouvignac · 2016-11-04T16:03:28Z

...src/test/java/org/autorefactor/refactoring/rules/samples_in/AndroidDrawAllocationSample.java

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


Please add correct copyright.

JnRouvignac · 2016-11-04T16:03:37Z

...rc/test/java/org/autorefactor/refactoring/rules/samples_out/AndroidDrawAllocationSample.java

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


Please add correct copyright.

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

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

+    public boolean visit(MethodDeclaration node) {
+        if (isMethod(node, "android.widget.TextView", "onDraw", "android.graphics.Canvas")) {
+            final ASTBuilder b = this.ctx.getASTBuilder();
+            final Refactorings r = this.ctx.getRefactorings();


Please remove unused b and r

JnRouvignac · 2016-11-04T16:06:44Z

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

+
+        public boolean isMethodBindingSubclassOf(ITypeBinding typeBinding, List<String> superClassStrings) {
+            ITypeBinding superClass = typeBinding;
+            while (superClass != null && !superClass.equals(ctx.getAST().resolveWellKnownType("java.lang.Object"))) {


Please extract a variable for this out of the loop: ctx.getAST().resolveWellKnownType("java.lang.Object")
I suspect doing this repeatedly in the loop is not great performance wise.

JnRouvignac · 2016-11-04T16:07:34Z

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

+                if (className.contains("<")) {
+                    className = className.split("<", 2)[0];
+                }
+                if (superClassStrings.contains(className)) {


if (superClassStrings.contains(superClass.getErasure().getName())) {

?

looks better 😅

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

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

+                if (initializer.getNodeType() == ASTNode.CAST_EXPRESSION) {
+                    initializer = ((CastExpression) initializer).getExpression();
+                }
+                if (initializer.getNodeType() == ASTNode.CLASS_INSTANCE_CREATION) {


Please use a switch:

switch (initializer.getNodeType()) { case CAST_EXPRESSION: initializer = ((CastExpression) initializer).getExpression(); break; case CLASS_INSTANCE_CREATION: ...

Switch will not work as expected. Since initializer is changed inside the first case, there are cases in which both blocks are executed.

Indeed, this is not an else if.
Sorry for the noise.

JnRouvignac · 2016-11-04T16:09:27Z

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

+                    InitializerVisitor initializerVisitor = new InitializerVisitor();
+                    initializer.accept(initializerVisitor);
+                    if (initializerVisitor.initializerCanBeExtracted) {
+                        Statement declarationStatement = (Statement) getFirstAncestorOrNull(node,


getAncestor()

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

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

+                                    // allocate object outside onDraw
+                                    r.insertBefore(b.move(declarationStatement), onDrawDeclaration);
+                                    // call collection.clear() in the end of
+                                    // onDraw


move to previous line

JnRouvignac · 2016-11-04T16:10:31Z

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

+                                    // call collection.clear() in the end of
+                                    // onDraw
+                                    ASTNode clearNode = b.getAST()
+                                            .newExpressionStatement(b.invoke(node.getName().getIdentifier(), "clear"));


ASTNode clearNode = b.toStmt(b.invoke(node.getName().getIdentifier(), "clear"));

JnRouvignac · 2016-11-04T16:11:20Z

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

+                                    ASTNode clearNode = b.getAST()
+                                            .newExpressionStatement(b.invoke(node.getName().getIdentifier(), "clear"));
+                                    List bodyStatements = onDrawDeclaration.getBody().statements();
+                                    Statement lastStatement = (Statement) bodyStatements.get(bodyStatements.size() - 1);


List<Statement> bodyStatements = .statements(onDrawDeclaration.getBody()); Statement lastStatement = bodyStatements.get(bodyStatements.size() - 1);

JnRouvignac · 2016-11-04T16:12:15Z

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

+
+        @Override
+        public boolean visit(MethodInvocation node) {
+            initializerCanBeExtracted = false;


It is not checking anything on the MethodInvocation, this seems dubious?

JnRouvignac · 2016-11-04T16:12:23Z

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

+
+        @Override
+        public boolean visit(SimpleName node) {
+            initializerCanBeExtracted = false;


It is not checking anything on the SimpleName, this seems dubious?

So, basically I don't want to refactor expressions like new Foo(bar) or new Foo(bar()). Since I'm moving these expressions to the outside of the method, I will do it only for default constructors or constructors with constants.

This is worth a comment in the code IMO

JnRouvignac · 2016-11-04T16:12:39Z

...src/test/java/org/autorefactor/refactoring/rules/samples_in/AndroidDrawAllocationSample.java

+@SuppressWarnings("unused")
+public class AndroidDrawAllocationSample extends Button {
+
+	public AndroidDrawAllocationSample(Context context, AttributeSet attrs, int defStyle) {


incorrect formatting

I didn't get this one :\

JnRouvignac · 2016-11-15T23:41:51Z

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

+                if (initializer.getNodeType() == ASTNode.CLASS_INSTANCE_CREATION) {
+                    ClassInstanceCreation classInstanceCreation = (ClassInstanceCreation) initializer;
+                    InitializerVisitor initializerVisitor = new InitializerVisitor();
+                    initializer.accept(initializerVisitor);


It would look more logical to me here to do:

classInstanceCreation.accept(initializerVisitor);

JnRouvignac · 2016-11-15T23:42:57Z

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

+        @Override
+        public boolean visit(SimpleType node) {
+            return DO_NOT_VISIT_SUBTREE;
+        }


Why this?

I think you should just remove it, or else handle all the possible types (there are plenty ;) )

JnRouvignac · 2016-11-15T23:43:40Z

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

+                                    }
+                                }
+                            } else {
+                                r.insertBefore(b.move(declarationStatement), onDrawDeclaration);


return DO_NOT_VISIT_SUBTREE;

JnRouvignac · 2016-11-15T23:43:45Z

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

+                                    } else {
+                                        r.insertAfter(clearNode, lastStatement);
+                                    }
+                                }


return DO_NOT_VISIT_SUBTREE;

JnRouvignac · 2016-11-15T23:44:36Z

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

+
+        public boolean isMethodBindingSubclassOf(ITypeBinding typeBinding, List<String> superClassStrings) {
+            ITypeBinding superClass = typeBinding;
+            ITypeBinding objectType = ctx.getAST().resolveWellKnownType("java.lang.Object");


You do not need that.
Just let superClass become null.

JnRouvignac · 2016-11-15T23:44:53Z

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

+            this.onDrawDeclaration = onDrawDeclaration;
+        }
+
+        public boolean isMethodBindingSubclassOf(ITypeBinding typeBinding, List<String> superClassStrings) {


This is a type binding :)

JnRouvignac · 2016-11-15T23:46:40Z

...src/test/java/org/autorefactor/refactoring/rules/samples_in/AndroidDrawAllocationSample.java

+        new String("foo");
+        String s = new String("bar");
+
+        // This one should not be reported:


JnRouvignac · 2016-11-15T23:47:39Z

...src/test/java/org/autorefactor/refactoring/rules/samples_in/AndroidDrawAllocationSample.java

+import android.content.Context;
+import android.graphics.Bitmap;
+import android.graphics.BitmapFactory;
+import android.graphics.Canvas;


These imports are never used:

import android.graphics.Bitmap; import android.graphics.BitmapFactory; import android.graphics.Canvas;

luiscruz force-pushed the drawallocation branch from e3ecac5 to 951e781 Compare November 4, 2016 11:34

JnRouvignac reviewed Nov 4, 2016

View reviewed changes

luiscruz force-pushed the drawallocation branch from d76e438 to 94112d3 Compare November 14, 2016 11:15

JnRouvignac reviewed Nov 15, 2016

View reviewed changes

luiscruz force-pushed the drawallocation branch from 52183d4 to b649b3d Compare November 29, 2016 12:10

Android DrawAllocation. Closes JnRouvignac#211

b25b9db

luiscruz force-pushed the drawallocation branch from b649b3d to b25b9db Compare November 30, 2016 19:01

Fabrice-TIERCELIN force-pushed the master branch from 06597e7 to 36c4878 Compare June 5, 2017 11:57

Fabrice-TIERCELIN added the new cleanup label May 23, 2018

Fabrice-TIERCELIN force-pushed the master branch from a395432 to 7cc3557 Compare April 27, 2019 07:10

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,90 @@
		package org.autorefactor.refactoring.rules.samples_in;

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

Android DrawAllocation. Closes #211 #227

Are you sure you want to change the base?

Android DrawAllocation. Closes #211 #227

Uh oh!

Conversation

luiscruz commented Nov 2, 2016

Uh oh!

JnRouvignac Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

JnRouvignac Nov 4, 2016 •

edited

Loading