RefactoringBenchmarksForPullUpMethod

Last edit December 10, 2001

Few more benchmarks I used when debugging Xrefactory.

Those benchmarks consider simplified version of pull-up with just one method to be moved into superclass (opposed to pulling up of two methods introduced in Martin's book).

In general pulling up a method looks like a simple copy and paste of code, but take for example the case:

  class Super {
    int x = 0;
  }

  class Infer {
    int x = 1;
    void toto() {System.out.println("x==" + x);}
  }

You can't simply move it to the superclass because it would change its behavior. One would expect that refactoring browser will issue an error message. It can also to move the method as follows:

  class Super {
    int x = 0;
    void toto() {System.out.println("x==" + ((Infer)this).x);} 
  }

  class Infer {
    int x = 1;
  }

However, this makes too much automatic modifications of code and users don't like big changes in their code. Isn't it?

So, I would vote for an error message in this benchmark (Marian).

On the other side, automatic correction is surely expected in the following case:

  class Super {
    int x = 0;
  }

  class Infer {
    int x = 1;
    void toto() {System.out.println("x==" + super.x);}
  }

should give:

  class Super {
    int x = 0;
    void toto() {System.out.println("x==" + this.x);}  // or simply x ???
  }

  class Infer {
    int x = 1;
  }

Also following benchmark is disputable:

  class Super {
    void f(Super s) {System.out.println("f(Super)");}
    void f(Infer i) {System.out.println("f(Infer)");}
  }

  class Infer {
    void callf() { f(this); }
  }

after pulling-up of callf, it should be:

  class Super {
    void f(Super s) {System.out.println("f(Super)");}
    void f(Infer i) {System.out.println("f(Infer)");}
    void callf() { f((Infer)this); }
  }

  class Infer {
  }

And no cast is necessary in the following (similar) benchmark:

  class Super {
    void f(Super s) {System.out.println("f(Super)");}
  }

  class Infer {
    void callf() { f(this); }
  }

so, pulling up can simply move the method:

  class Super {
    void f(Super s) {System.out.println("f(Super)");}
    void callf() { f(this); }
  }

  class Infer {
  }

I don't understand this. Is the problem that a subclass can define a new variable with the same name as a variable in the superclass? Moving a method will cause it to refer to the superclass variable instead of the subclass variable. So, is the problem that when you move a function to a different scope, you have to make sure that it refers to the same variables that it did in the old scope?

IMO, defining a new variable with the same name as a variable in a superclass is a code smell. It makes the code harder to understand because you might miss the local variable definition and think it refers to the one in the superclass. If there is only one possibility, you can't choose the wrong one. We need freedom from choice.

Those benchmarks are intended to check capabilities of refactoring browsers. That is why they present small non-trivial cases with some problem. They do not demonstrate nothing in their own, they are just examples of initial situation and the result which should be obtained after application of the refactoring. A browser passing more benchmarks is better (in some sense) than another passing less benchmarks.

But when looking on benchmarks, actually it seems that those problematic examples are due to bad smells in code. In fact, anything making future refactoring difficult is a bad smell.