Android DrawAllocation. Closes #211 by luiscruz · Pull Request #227 · JnRouvignac/AutoRefactor

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

No description provided.

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

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

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


Please add correct copyright.

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

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


Please add correct copyright.

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

+    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

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

+                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

+                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

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


getAncestor()

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

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

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

+                                    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

+
+        @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

+
+        @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

+@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

+                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

+        @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

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


return DO_NOT_VISIT_SUBTREE;

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

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


return DO_NOT_VISIT_SUBTREE;

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

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

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


This is a type binding :)

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

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


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

+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 5 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 19 times, most recently from e446e46 to 4f0cb47 Compare October 7, 2019 16:43

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

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

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