SpartanRefactoring / Main

Eclipse plugin that performs automatic refactoring of Java source code, making it shorter, more idiomatic and more readable
https://www.spartan.org.il
100 stars 56 forks source link

Enrich the environment with two methods #106

Open yossigil opened 8 years ago

yossigil commented 8 years ago
yossigil commented 8 years ago

I do not see those commits coming!

yossigil commented 8 years ago

@dngreenst : you should have started with a commit, with unit tests that check that the function is definend and a bunck of @Ignore non-working yet tests.

dngreenst commented 8 years ago

We're using MakeAST.COMPILATION_UNIT to create ASTNodes for the tests (that are not connected to the main AST).

Have we missed a more elegant way of doing this?

sakopzon commented 8 years ago

A few weeks ago we wrote a full bunch of code examples for testing the env... Now while writing the actual tests I thought that we should somehow integrate those code examples instead of writing every single piece of code as a String. Am I right?

yossigil commented 8 years ago

Sure you would. You do the same recursive scanning for implementing these two methods. The whole purpose of these methods is: say your are given a function f with argument X. You want to change X to Y. To be able to do so, you must check that the f does not define Y and that it uses no variable named Y.

For the testing, you would need your @Annotation. For real production, these two function will serve. They are interchangeable.

sakopzon commented 8 years ago

We found a method for traversing ASTNode: for (ICompilationUnit unit : mypackage.getCompilationUnits()) { IType[] types = unit.getTypes(); for (int i = 0; i < types.length; i++) { IType type = types[i]; IMethod[] methods = type.getMethods(); But I think it is not exactly what you meant... in this case we suppose we get a CompilationUnit, which extends ASTNode but then not every ASTNode will be suitable to use. Could you give us some wire end?

yossigil commented 8 years ago

Study the methods in classes wizard, step and hop. I believe you have a solution there. When you find it, document it. If it already documented, document another function

sakopzon commented 8 years ago

We think the return type of the functions should be some sort of ordered collection, and in my understanding Set isn't.. Should we use LinkedHashMap instead? It is an issue when checking the order of the names in the Environment

yossigil commented 8 years ago

There is a LinkdeHashSet

sakopzon commented 8 years ago

One more implementation choice: We need to create new Map.Entry<>() but saw that java isn't supporting that. we found to options:

  1. use ugly variant only for testing(there is no reason I can think of one instantiating Entry out of the test): TreeMap<String,Information> m = new TreeMap<>(); m.put(s, i); testFlatENV.add(m.firstEntry());
  2. You can just implement the Map.Entry<K, V> interface ourselves: `final class MyEntry<K, V> implements Map.Entry<K, V> { private final K key; private V value;
public MyEntry(K key, V value) {
    this.key = key;
    this.value = value;
}

@Override
public K getKey() {
    return key;
}

@Override
public V getValue() {
    return value;
}

@Override
public V setValue(V value) {
    V old = this.value;
    this.value = value;
    return old;
}

}`

We peaked the second

dngreenst commented 8 years ago

There is a problem with unmodifiable for LinkedHashSet - only Collection.unmodifiableSortedSet and Collection.unmodifiableSet exist.

dngreenst commented 8 years ago

@yossigil we have a little problem with Information equality check. For testing purposes, we would like to check equality in a more thorough manner than just checking if they belong to the same instance. I have implemented a equals() function to that end.

However, that means we also need hashCode to be overridden. To do that, I found

http://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/builder/package-summary.html

However, I am uncertain as to how to add that to the project.

EDIT no longer relevant.

dngreenst commented 8 years ago

@yossigil , we were thinking about the design of Environment.uses(ASTNode n) and Environment.declares(ASTNode n). Some thoughts we wanted to run by you:

  1. When we were thinking about the design, renaming was the main use case that came to mind. Are there any other use cases you would like us to consider?
  2. We think that each one of the methods should be split:
    • Environment.declaresUp(ASTNode n) should look for declarations that appear above the current node n. In other words, we should go over n's ancesors, and the statements of these ancestors.
    • Environment.declaresDown(ASTNode n) should look for declarations that appear below the current node n.
    • Environment.UsesUp(ASTNode n) Similar to Environment.declaresUp(ASTNode n), for uses.
    • Environment.UsesDown(ASTNode n) Similar to Environment.declaresDown(ASTNode n), for uses.
yossigil commented 8 years ago

The differences are right: something may be in the scope without being used. But, don't let bother your. Proceed with one definition, but make it right, clear and crisp. Then, when applications come, renaming is the major one, you can fine tune it, but you cannot fine tune a definition which was not clear initially.

yossigil commented 8 years ago

No work for now

yossigil commented 7 years ago

Dor: passing on to you. It is mostly done. If there is nothing more to do, close this

yossigil commented 7 years ago

make the environment more useful in tippers.