Bug 200152 - [refactoring] convert int-constants to Java5-enums
Summary: [refactoring] convert int-constants to Java5-enums
Status: ASSIGNED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: UI (show other bugs)
Version: 3.4   Edit
Hardware: PC Windows XP
: P3 enhancement with 1 vote (vote)
Target Milestone: ---   Edit
Assignee: JDT-UI-Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: helpwanted
Depends on:
Blocks:
 
Reported: 2007-08-16 03:15 EDT by Jan Matèrne CLA
Modified: 2010-12-21 08:30 EST (History)
7 users (show)

See Also:


Attachments
Initial test case. (2.10 KB, application/octet-stream)
2007-10-29 19:53 EDT, Raffi Khatchadourian CLA
no flags Details
Initial plug-in binaries including source. (86.51 KB, application/octet-stream)
2007-11-03 17:19 EDT, Raffi Khatchadourian CLA
no flags Details
mylyn/context/zip (7.35 KB, application/octet-stream)
2007-11-03 17:19 EDT, Raffi Khatchadourian CLA
no flags Details
mylyn/context/zip (14.39 KB, application/octet-stream)
2007-11-11 22:46 EST, Raffi Khatchadourian CLA
no flags Details
1.4 complaint code (83.91 KB, application/octet-stream)
2007-11-11 22:47 EST, Raffi Khatchadourian CLA
no flags Details
Updated 1.4 complaint code (83.94 KB, application/octet-stream)
2008-11-09 17:56 EST, Raffi Khatchadourian CLA
no flags Details
Added a few test cases (83.94 KB, application/octet-stream)
2008-11-28 13:26 EST, Raffi Khatchadourian CLA
no flags Details
Added a few test cases (83.94 KB, application/octet-stream)
2008-11-28 13:28 EST, Raffi Khatchadourian CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Matèrne CLA 2007-08-16 03:15:11 EDT
In pre-Java5 code you can often find enumerations in form of int-constants.

public class Constants {

    public static final int COLOUR_BLACK = 1;
    public static final int COLOUR_WHITE = 2;

    public static void show(int colour) {
        switch (colour) {
        case COLOUR_BLACK:
            System.out.println("black");
            break;
        case COLOUR_WHITE:
            System.out.println("white");
            break;
        default:
            System.out.println("unknown colour");
            break;
        }
    }
}


A refactoring could convert the constants into an enum:

public class ConstantsUsingEnums {

    public enum Colour {
        BLACK, WHITE
    }
    
    public static void show(Colour colour) {
        switch (colour) {
        case BLACK:
            System.out.println("black");
            break;
        case WHITE:
            System.out.println("white");
            break;
        default:
            System.out.println("unknown colour");
            break;
        }
    }
}



The selection which constants should be converted could be
- by manual selection
- by common prefix (COLOUR_*)
For example:
- user positiones the cursor on COLOUR_BLACK
- user starts the "Convert int-constant to enum" refactoring
- separation character seems to be "_"
- multiple used prefix is "COLOUR_"
- display all "public final" constants of the same type (int)
  (here "COLOUR_BLACK" and "COLOUR_WHITE")
- activate all constants with the common prefix
  (so both)
- User could change these settings
Comment 1 Jan Matèrne CLA 2007-08-16 04:00:31 EDT
I googled for some sources and found an article which could be interesting:

ftp://ftp.cse.ohio-state.edu/pub/tech-report/2007/TR26.pdf
"Automated Refactoring of Legacy Java Software to Enumerated Types"

They describe exactly the same problem and an approach for resolving. They implemented an unpublished Eclipse plugin for that refactoring. I dont know what the future of that plugin will be ....
Comment 2 Jerome Lanneluc CLA 2007-08-16 05:02:05 EDT
Moving to JDT/UI
Comment 3 Martin Aeschlimann CLA 2007-08-16 05:54:22 EDT
Would definitely be great to have. Maybe the authors of the paper from comment 1 are interested in contributing this to JDT?
Comment 4 Raffi Khatchadourian CLA 2007-08-16 15:42:01 EDT
We would be extremely interested in contributing our plugin to the JDT. Unfortunately, the plugin is currently in a "research prototype" stage and would need some work prior to source code distribution. I could have this ready by mid-September. As for the prefixes that are mentioned in the bug description, there is some previous work on declaration heuristics to identify enumerated types in C [1]. There is also previous work to identify enumerated types in COBOL through flow-insensitive data-flow analysis [2]. At some point, we would also like to incorporate similar techniques (perhaps even developing new ones) in our plugin. Please let me know if you have any additional questions and I will get to work on molding the source code and I'll post my results hopefully soon!

[1] J. M. Gravley and A. Lakhotia. Identifying enumeration types modeled with symbolic constants. In Working Conference on Reverse Engineering, page 227, Washington, DC, USA, 1996. IEEE Computer Society.

[2] A. V. Deursen and L. Moonen. Type inference for COBOL systems. In Working Conference on Reverse Engineering, page 220, Washington, DC, USA, 1998. IEEE Computer Society.
Comment 5 Martin Aeschlimann CLA 2007-08-17 04:45:13 EDT
Raffi, thanks for the reply and your interest. 
For us to be able to accept a contribution, we must make sure that:
- there are no problems with licenses and copyrights. Best is if all the code has been written and is owned by you
- we can count on you also after committing the code. We're constantly overloaded with work so we rather decline a contribution if we're not sure that it will be in product quality at the next release (~March 2008)
- the code fits in our existing code base, say uses our infrastructure, AST, type constraints. It's not possible for us to maintain two or more of the same infrastructure.
- your code is well tested, contains JUnit tests and test coverage is high.

If you are just lacking a good UI, there we can help.
Comment 6 Raffi Khatchadourian CLA 2007-08-17 10:13:17 EDT
Martin, sure that wouldn't be a problem. I am currently using the Eclipse AST infrastructure (no type constraints as of now). More than likely I will need help on the UI, and yes you can count on me after the initial commit of the code. Thanks again for your interest and I hope to be in touch soon and I am very interested in including the plugin in the next release.
Comment 7 Raffi Khatchadourian CLA 2007-09-12 14:40:23 EDT
Did you guys want this as a standalone plugin or as a patch to the eclipse source, coupled with the other eclipse standard refactoring plugins? Thanks!
Comment 8 Raffi Khatchadourian CLA 2007-09-12 20:57:45 EDT
Also, do you have a desired name for the refactoring? Right now it is "Convert Fields to Enum." Let me know if that is okay. Thanks again!
Comment 9 Jan Matèrne CLA 2007-09-13 02:24:57 EDT
"Convert Fields to Enum." would imply that the type of the field does not matter. "Usually" an int is used, but you also could use a char. Does the refactoring handle this? Then the name is basically ok with me.

Are "fields" or "constants" (= static final) involved?
Comment 10 Martin Aeschlimann CLA 2007-09-13 03:26:34 EDT
If we decide to integrate it into JDT UI, then we would also add it in our source tree and coupled it with the other built-in refactorings. But having it as a separate plug-in might be more comfortable for you to develop and to test. So its your choice. Converting shouldn't be a big problem.
I assume your plug-in will end up with lots of internal references to JDT UI. Don't worry about that at the moment. As mentioned, its important to us to have it well integrated and reusing our infrastructure if possible so we don't end up with duplicate functionality.

'Convert Fields to Enum' sounds good to me!

Let me know if you want me to do a first review of the code .
Comment 11 Martin Aeschlimann CLA 2007-09-13 06:13:24 EDT
'Convert Constants to Enum' might be even better.
Comment 12 Raffi Khatchadourian CLA 2007-09-13 10:31:59 EDT
(In reply to comment #9)
> "Convert Fields to Enum." would imply that the type of the field does not
> matter. 

Yes, that is  a good point. Currently the plugin can refactor fields of all primitive types except boolean.

> "Usually" an int is used, but you also could use a char. Does the
> refactoring handle this?

Yes, it does. However, we found that (unfortunately) a very common type was String. I say unfortunately here because string comparisons, on average, are much more expensive than primitive comparisons. Extending the plugin to deal with String constants is part of our planned future work. In fact, we would like to deal with all kinds of reference types.
 
> Are "fields" or "constants" (= static final) involved?

They are constant fields. I suppose the verbose name would be:
"Convert Primitive Field Constants To Enum" ;) 

Comment 13 Raffi Khatchadourian CLA 2007-09-13 10:36:17 EDT
(In reply to comment #10)
> If we decide to integrate it into JDT UI, then we would also add it in our
> source tree and coupled it with the other built-in refactorings. But having it
> as a separate plug-in might be more comfortable for you to develop and to test.

Yes, I agree.

> So its your choice. Converting shouldn't be a big problem.

Ok, I will write it as a standalone plugin for the moment and leave the conversion for later.

> I assume your plug-in will end up with lots of internal references to JDT UI.

It should. I am currently trying to structure the plugin in the style of this tutorial: http://www.eclipse.org/articles/article.php?file=Article-Unleashing-the-Power-of-Refactoring.

> Let me know if you want me to do a first review of the code.

Great, I'll let you know when I'm ready! Thanks again!
Comment 14 Raffi Khatchadourian CLA 2007-09-13 10:49:18 EDT
Sorry, the above link should read http://www.eclipse.org/articles/article.php?file=Article-Unleashing-the-Power-of-Refactoring/index.html
Comment 15 Jan Matèrne CLA 2007-09-14 02:58:04 EDT
(In reply to comment #12)
> (In reply to comment #9)
> > "Convert Fields to Enum." would imply that the type of the field does not
> > matter. 
> 
> Yes, that is  a good point. Currently the plugin can refactor fields of all
> primitive types except boolean.
> 
> > "Usually" an int is used, but you also could use a char. Does the
> > refactoring handle this?
> 
> Yes, it does. However, we found that (unfortunately) a very common type was
> String. ... part of our planned future work ... we would like to deal with all kinds of reference types.
> 
> > Are "fields" or "constants" (= static final) involved?
> 
> They are constant fields. I suppose the verbose name would be:
> "Convert Primitive Field Constants To Enum" ;) 
> 

If you want to deal with all kinds of references, "Convert Fields to Enum" would be good. If they have to be final, a "Convert Constants to Enum" would be better.

When naming the refactoring see into the future. You dont change the refactoring so dramatically that a new name should exist. And I think the users should have the chance for becoming a friend of that refactoring.
Comment 16 Jan Matèrne CLA 2007-09-14 03:01:58 EDT
(In reply to comment #14)
> Sorry, the above link should read
> http://www.eclipse.org/articles/article.php?file=Article-Unleashing-the-Power-of-Refactoring/index.html
 

Nice article, thanks.
Does someone know if this also the common way for implementing refactoring of Ant buildfiles? 
see https://bugs.eclipse.org/bugs/show_bug.cgi?id=89938
Comment 17 Martin Aeschlimann CLA 2007-09-14 04:14:46 EDT
Yes, the refactoring framework is not Java specific. (its part of the language toolkit ltk).
Look here for a simple example of how to implement a text based refactoring:
http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.ltk.ui.refactoring.tests/src/org/eclipse/ltk/ui/refactoring/examples/
Comment 18 Jan Matèrne CLA 2007-09-14 04:44:49 EDT
(In reply to comment #17)
> Yes, the refactoring framework is not Java specific. (its part of the language
> toolkit ltk).
> Look here for a simple example of how to implement a text based refactoring:
> http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.ltk.ui.refactoring.tests/src/org/eclipse/ltk/ui/refactoring/examples/
> 

Thanks for that. I'll have a look at that and the article.
Comment 19 Raffi Khatchadourian CLA 2007-10-07 09:00:51 EDT
I just wanted to give a brief status report on the current state of the plug-in. Moving the prototype implementation to solely use the Eclipse refactoring framework has taken a little more time than I initially anticipated; however, the development process is steady. I would expect an initial release (ready for code review, etc.) to be ready by the end of this month. Below are further specifics:

Tasks Completed:
- Old pattern (i.e., static final fields) removed from source.

- Declarations of dependent entities (i.e., fields, method return types, formal parameters, local variables) transformed to new enum type (the type name is currently produced via a simulator, for example "NewEnumType0," "NewEnumType1," etc.)

- New enum type programmatically inserted into source via "New Enum Type" wizard.

Tasks in Progress:
- Specifying a package other than the default package for the new enum type to be sent to the "New Enum Type" wizard.

- Utilization of the import rewritter to update imports statements in the client code appropriately.

- Namespace prefix updating (e.g., removal of class name prior to the constant reference in switch case statements and addition of enum type name prefix prior to references in all other expressions if not already present)

That's the state of the union so far. Please let me know if you have any questions and I again apologize for the delay!

Raffi
Comment 20 Raffi Khatchadourian CLA 2007-10-23 22:49:22 EDT
Waiting on Bug 20725 ...
Comment 21 Raffi Khatchadourian CLA 2007-10-23 22:50:23 EDT
Check that, waiting on Bug 207257
Comment 22 Raffi Khatchadourian CLA 2007-10-29 19:53:43 EDT
Created attachment 81525 [details]
Initial test case.

Plug-in source and binaries forthcoming.
Comment 23 Raffi Khatchadourian CLA 2007-11-03 17:19:16 EDT
Created attachment 82035 [details]
Initial plug-in binaries including source.

I apologize for the extremely long delay (life as a grad student isn't all its cracked up to be ;) ). Here is the first attempt at a "commercial" quality version of the "Convert Constants To Enum" Eclipse plug-in. Currently, its functionality is somewhat limited at this point in the following sense:

1. It lacks a UI. Therefore, when you run the plug-in, it attempts to refactor the selected fields (I'll get to that next). If any of the fields meet the initial and final preconditions, then it goes ahead and commits the change (no confirmation, etc.). If a field does not meet the precoditions then there is no effect. If a fatal refactoring status entry is encountered, a runtime exception is thrown with the associated message. Upon any error, the refactoring is aborted and no changes are commit *except* that there may exist empty enum types (I'll also get to this later).

2. There's no undo functionality.

3. There's no way to further union the new enum types (i.e., combine types resulting from transitive dependencies into larger types). Therefore, the *minimal* types are created (may lead to very sparse types in larger programs).

4. There's no way to specify the exact names of the new enum types, this is done via a type name simulation.

5. There's no way to specify the exact package name of the new enum types (also done via a simulation).

6. At this point, the fields to be refactored are inputted by selecting them either in the outline view or in the package view. To select multiple fields to refactor hold down the CTRL key (or command key on Mac). I would prefer to expand this in the following sense: 1) If a class is selected then all primitive (-boolean) static final fields of that class will be input to the plug-in. 2) If a package is selected then for every class in that package, all primitive (-boolean) static final fields of that package are input to the plug-in and so on and so forth. I believe that the "Infer Type Argument" plug-in has a similar functionality. Unfortunaetly, I haven't had the change to implement this but I figured that hand selecting individual fields would okay for now since it will be easier for testing.

7. This is (my) design decision: If an input constant shares transitive dependencies with another constant *that was not input to the plug-in*, the refactoring of the input constant will not proceed. I made this decision because I imagine that if a user did not select a constant to be refactored (e.g., serialVersionUID) then the plug-in should not refactor it.

8. There's no way to create enum types as inner classes of existing types. All new enum types will reside in their own file.

9. The creation of the new enum type is not part of the refactoring. This activity is delegated to the "Create New Enum Type" plug-in. This was not a design decision, I didn't know how else to achieve this functionality. In Eclipse lingo, the insertion of the new enum type is not included in the "change"s that the plug-in "commits." Therefore, abortions of the change commit may not affect the creation of the new enum types.

10. The resulting refactoring status does not always include information about why a field failed precondition checking. It will include some information for simple cases (e.g., the field is a serialVersionUID) but for more complex cases (e.g., rejection by the enumerization computer) no information currently will be available.

11. If creating a new enum type requires moving an annotation from original source code, if there was an import statement for that annotation, that import statement will not be removed and may become unused.

I realize the documentation is a bit slim at this point. The checkFinalPrecondition() and EnumerizationComputer.compute() methods contain the "meat" of the plug-in. The latter method can be treated as a blackbox for now as it is basically the implementation of the ideas from the paper (I placed a link in comments). Also, I have listed several tasks in the source code which most likely correspond to the list of above. The source code is also included with the plug-in.

Please let me know if you have any questions and I look forward to your feedback!
Comment 24 Raffi Khatchadourian CLA 2007-11-03 17:19:21 EDT
Created attachment 82036 [details]
mylyn/context/zip
Comment 25 Martin Aeschlimann CLA 2007-11-07 11:50:31 EST
Hi Raffi,
thanks for the preview. Here are some comments I got from the first look, merely of a stylistic nature. Code looks good in general.
 
1. Let us know if you want to provide you with a UI. But this should not very difficult to do. 

2. In case we plan to integrate this in JDT/UI, you will have convert it to 1.4. Unfortunately we still need to be 1.4 source compatible. From looking at your code I see that you don't have many dependency to internal code, so we could also decide to kept this as a separate plug-in. 

3. don't catch runtime exceptions. In one comment you write that the search engine throws a NullPointerException. Make sure you filed a bug against that.

4. Method bindings already know the type hierarchy they are in. So Util.getTopMostSourceMethod could be avoided, use the binding for that. The rules if methods override is a very tricky business in 1.5, that's can't be done with IJavaElements.
So I would review where 'binding.getJavaElement' is used and I would try to reduce these places.

5. I'm not sure, but do you track the methods you have already searched for? Each method should only searched once per index
foo(constCandidate);
foo(constCandidate);

6. make sure to have test cases for each case covered in code. There are some many details to keep attention to, so tests can help you remembering them all. 

Hope this helps.
Comment 26 Raffi Khatchadourian CLA 2007-11-07 14:14:29 EST
(In reply to comment #25)
Hi Martin,

> thanks for the preview.

Not a problem, thanks for taking the time to review the code.

> Here are some comments I got from the first look,
> merely of a stylistic nature. Code looks good in general.

Thanks!

> 1. Let us know if you want to provide you with a UI. But this should not very
> difficult to do. 

Yes, please. That would be great!

> 2. In case we plan to integrate this in JDT/UI, you will have convert it to
> 1.4.

I can convert the code to 1.4, that shouldn't be a problem.

> 3. don't catch runtime exceptions. In one comment you write that the search
> engine throws a NullPointerException. Make sure you filed a bug against that.

Ok, I will change that, but as far as reporting the bug, its hard to reproduce it. I'll think about that.

> 4. Method bindings already know the type hierarchy they are in. So
> Util.getTopMostSourceMethod could be avoided, use the binding for that.

Ok, I will change that. The reason I needed the top-level method is because the search engine would not retrieve the entire method hierarchy otherwise.

> rules if methods override is a very tricky business in 1.5, that's can't be
> done with IJavaElements.
> So I would review where 'binding.getJavaElement' is used and I would try to
> reduce these places.

I will take a look at that.

> 5. I'm not sure, but do you track the methods you have already searched for?

No, I don't believe I do.

> Each method should only searched once per index
> foo(constCandidate);
> foo(constCandidate);

I see what you mean. The problem is that I split up the problem into two different portions, the analysis and the transformation, which are independent of each other. I suppose that the most a particular program entity can be searched for is twice. Merging these portions may involve some work, however. I'll have to think about that as well.

> 6. make sure to have test cases for each case covered in code. There are some
> many details to keep attention to, so tests can help you remembering them all.

I will do.
 
> Hope this helps.

Very much so! Thanks for your feedback!

Raffi
Comment 27 Raffi Khatchadourian CLA 2007-11-11 22:46:12 EST
Hi Martin,

(In reply to comment #25)
> 2. In case we plan to integrate this in JDT/UI, you will have convert it to 1.4.

The conversion is done, I have attached the new source code.

> 3. don't catch runtime exceptions.

Fixed.

> 4. Method bindings already know the type hierarchy they are in. So
> Util.getTopMostSourceMethod could be avoided, use the binding for that.

I couldn't see how to do this. The Util.getTopMostSourceMethod uses the MethodChecks class, is this what you are talking about?

> 5. I'm not sure, but do you track the methods you have already searched for?

Could I use a SearchMatch caching aspect for this? Am I allowed to turn the project into an AspectJ project?

Thanks! -Raffi
Comment 28 Raffi Khatchadourian CLA 2007-11-11 22:46:15 EST
Created attachment 82637 [details]
mylyn/context/zip
Comment 29 Raffi Khatchadourian CLA 2007-11-11 22:47:25 EST
Created attachment 82638 [details]
1.4 complaint code
Comment 30 Martin Aeschlimann CLA 2007-11-12 06:14:45 EST
4. Instead of
final IMethod meth= (IMethod) mi.resolveMethodBinding().getJavaElement();
final IMethod top= Util.getTopMostSourceMethod(meth, this.monitor);

IMethodBinding binding= mi.resolveMethodBinding();
IMethodBinding top= findDefinition(binding.getDeclaringClass(), binding);


// following code is not tested, but similar as org.eclipse.jdt.internal.corext.dom.Bindings.findOverriddenMethod(...)
public static IMethodBinding findDefinition(ITypeBinding type, IMethodBinding binding) {
  ITypeBinding superClass= type.getSuperclass();
  if (superClass != null) {
    IMethodBinding method= findDefinition(superClass, binding);
    if (method != null)
      return method;			
  }
  ITypeBinding[] interfaces= type.getInterfaces();
  for (int i= 0; i < interfaces.length; i++) {
    method= findOverriddenMethodInHierarchy(interfaces[i], binding);
    if (method != null)
      return method;
    }
    return null;
  }
  IMethodBinding[] methods= type.getDeclaredMethods();
  for (int i= 0; i < methods.length; i++) {
    if (binding.overrides(methods[i])
       return methods[i];
  }
}


5. No, you can't use AspectJ. 
I guess with a good ordering and remembering of results you could maybe reduce the number of searches and ASTs build in the first pass. But I'm just speculating.
In general, you need to be careful with the number of ASTs kept in memory at the same time.



Comment 31 Raffi Khatchadourian CLA 2007-12-22 21:02:11 EST
 (In reply to comment #25) 
> 6. make sure to have test cases for each case covered in code. There are some
> many details to keep attention to, so tests can help you remembering them all.

Are there any examples in the Eclipse library of junit tests (or any test code) for refactoring plug-ins? Thanks!
Comment 32 Benno Baumgartner CLA 2007-12-23 09:27:12 EST
(In reply to comment #31)
> Are there any examples in the Eclipse library of junit tests (or any test code)
> for refactoring plug-ins? Thanks!

See the org.eclipse.jdt.ui.tests.refactoring project. Especially subtypes of org.eclipse.jdt.ui.tests.refactoring.RefactoringTest 
Comment 33 Raffi Khatchadourian CLA 2008-01-26 13:10:19 EST
I'm trying to create a class that extends org.eclipse.jdt.ui.tests.refactoring.RefactoringTest, but I am having trouble getting it to compile due to dependencies. Do I need to checkout certain modules from CVS in order to get it to compile? Thanks!
Comment 34 Raffi Khatchadourian CLA 2008-01-26 13:42:58 EST
 (In reply to comment #25)
> 5. I'm not sure, but do you track the methods you have already searched for?
> Each method should only searched once per index
> foo(constCandidate);
> foo(constCandidate);

What do you mean by "once per index?" Would a simple fix be replacing all calls to SearchEngine.search() with one that caches previous search results? Thanks again for the help!
Comment 35 Martin Aeschlimann CLA 2008-01-28 07:02:37 EST
org.eclipse.jdt.ui.tests.refactoring
requires

org.eclipse.jdt.ui.tests
org.eclipse.test.performance.win32
org.eclipse.test.performance

and plugins from the regular SDK drops


> Would a simple fix be replacing all calls
> to SearchEngine.search() with one that caches previous search results?

Yes, that's what I meant. Searching for references in a larger project can take a long time. Remembering results makes sense.
Comment 36 Raffi Khatchadourian CLA 2008-01-28 07:07:43 EST
 (In reply to comment #35)
> org.eclipse.jdt.ui.tests.refactoring
> requires
> 
> org.eclipse.jdt.ui.tests
> org.eclipse.test.performance.win32
> org.eclipse.test.performance
> 
> and plugins from the regular SDK drops

Ok, I will give it a shot.

> > Would a simple fix be replacing all calls
> > to SearchEngine.search() with one that caches previous search results?
> 
> Yes, that's what I meant. Searching for references in a larger project can take
> a long time. Remembering results makes sense.

This is a separate issue, but do you think this should be the default behavior of the search engine?
Comment 37 Raffi Khatchadourian CLA 2008-02-08 10:27:17 EST
 (In reply to comment #35)
> > Would a simple fix be replacing all calls
> > to SearchEngine.search() with one that caches previous search results?
> 
> Yes, that's what I meant. Searching for references in a larger project can take
> a long time. Remembering results makes sense.

Hi Martin. I have been trying to implement this caching feature that we have been discussing, but honestly I think its getting a little messy, specifically in regards to the separation of concerns. The problem is that the SearchPattern class doesn't override the equals() method, so it is difficult to assess whether or not a certain query has been previously asked, and furthermore, if so, to deliver the correct cached results to the requestor. Another problem that I foresee is that I am unsure as to how much memory space will be required to store the search matches. In other words, I don't know what the trade-off will be between performance and memory in this context, in fact, it reminds me a little of the reasons why we do not cache previously built ASTs. I would imagine that SearchMatches take less space than ASTs, however, but then again this is just speculation. Lastly, as I was looking through the source code of the search engine, I'm not entirely convinced that executing the same query multiple times is that much of a performance hit compared to the memory that must be stored and managed if we were to manually cache the results. That is, it seems that the search engine is using some sort of index, and it may be the case that the index is properly updated once a query has executed thereby allowing repetitive searches to execute much faster. That's just a hunch. Thanks and I look forward to hearing from you! -Raffi
Comment 38 Martin Aeschlimann CLA 2008-02-08 11:14:22 EST
It's your call, of course. Obviously I haven't done any deeper investigations or measurements. After all this is a refactoring and its ok to take a while. 
Comment 39 Raffi Khatchadourian CLA 2008-02-09 17:07:17 EST
(In reply to comment #38)
> It's your call, of course. Obviously I haven't done any deeper investigations
> or measurements. After all this is a refactoring and its ok to take a while. 

Ok, thanks! By the way, when is the due date for the next release? 

Comment 40 Martin Aeschlimann CLA 2008-02-11 08:31:38 EST
To integrate your code to JDT.UI, we have to target for 3.5. We have to go for a separate plug-in for 3.4 first (offered by yourself).

The problem is on our side: We have to get the legal approvement from eclipse.org which can take several weeks.
The 3.4 deadline for legal approvement was January 31. I learned of this deadline until a week ago. Sorry for that.
We also need time for review and we're quite booked in the coming weeks. API and feature freeze is M6 which is March 28.
Comment 41 Raffi Khatchadourian CLA 2008-02-18 06:47:39 EST
 (In reply to comment #40)
> To integrate your code to JDT.UI, we have to target for 3.5. We have to go for a
> separate plug-in for 3.4 first (offered by yourself).

Ok, that's basically its state now (a separate plug-in). I realize, however, that the major task at hand is the creation and execution of test cases.

>API and feature freeze is M6 which is March 28.

I see. Am I to assume that I should have everything (e.g., test cases) completed on my end by that time? 
Comment 42 Martin Aeschlimann CLA 2008-02-18 07:04:36 EST
> I see. Am I to assume that I should have everything (e.g., test cases)
> completed on my end by that time? 
> 
No, with the separate plugin (and separate delivery) there are no time constraints. 
Comment 43 Raffi Khatchadourian CLA 2008-02-18 07:11:07 EST
 (In reply to comment #42)
> No, with the separate plugin (and separate delivery) there are no time
> constraints.

I see. In that case, I will just let you know when I have completed the test cases (I think there were was also a minor implementation issue with the bindings that I will need to fix as well).
Comment 44 Raffi Khatchadourian CLA 2008-02-24 09:23:06 EST
 (In reply to comment #35)
> org.eclipse.jdt.ui.tests.refactoring
> requires
> 
> org.eclipse.jdt.ui.tests

I am having trouble getting the plug-in org.eclipse.jdt.ui.tests to compile properly. I checked it out from CVS and the manifest file says it requires jdt 3.4, but I'm using the standard version of eclipse which is 3.3. Any advise? Should I be developing on 3.4? Thanks!
Comment 45 Martin Aeschlimann CLA 2008-02-25 04:04:43 EST
To work with 3.3, use the branch '3_3_maintenance' for all test projects.
Comment 46 Raffi Khatchadourian CLA 2008-05-11 16:51:03 EDT
 (In reply to comment #32)
> (In reply to comment #31)
> > Are there any examples in the Eclipse library of junit tests (or any test
> code)
> > for refactoring plug-ins? Thanks!
> 
> See the org.eclipse.jdt.ui.tests.refactoring project. Especially subtypes of
> org.eclipse.jdt.ui.tests.refactoring.RefactoringTest

I'm currently examining the org.eclipse.jdt.ui.tests.refactoring.InferTypeArgumentsTest class which is a subclass of org.eclipse.jdt.ui.tests.refactoring.RefactoringTest. I noticed that it defines a method named getRefactoringPath() that returns a *relative* path (i.e., "InferTypeArguments/" in respect to the "resources" folder in the plug-in, which of course contains the test cases. If my plug-in is suppose to be separate from the rest of the refactoring plug-ins, how would I go about providing such a path of my own? Thanks!
Comment 47 Martin Aeschlimann CLA 2008-05-13 07:07:28 EDT
You will have your own class  'RefactoringTest'. Don't hesitate to copy as much test classes as you need ('JavaProjectHelper'...). It's not really important how the tests look like, if they do test something. We have several flavors of test layouts. The one you see in refactoring, but also the ones you see in quick fix (for example 'ModifierCorrectionsQuickFixTest').
Comment 48 Raffi Khatchadourian CLA 2008-08-13 07:10:23 EDT
Hello. I was just wondering if there was any updates on the UI side of this? Thanks!
Comment 49 Dani Megert CLA 2008-08-13 07:14:43 EDT
>Hello. I was just wondering if there was any updates on the UI side of this?
What exactly do you mean?
Comment 50 Raffi Khatchadourian CLA 2008-08-13 07:19:15 EDT
(In reply to comment #49)
> >Hello. I was just wondering if there was any updates on the UI side of this?
> What exactly do you mean?

The tool is lacking a UI. See comment #25 and comment #26. Thanks! 

Comment 51 Dani Megert CLA 2008-08-13 07:38:42 EDT
Ah I see. Sorry but we don't have resources to work on the UI.
Comment 52 Raffi Khatchadourian CLA 2008-08-13 13:44:33 EDT
(In reply to comment #51)
> Ah I see. Sorry but we don't have resources to work on the UI.
> 

Ah, I'm sorry to hear that. Is that a permanent thing?
Comment 53 Dani Megert CLA 2008-08-14 02:09:16 EDT
>Ah, I'm sorry to hear that. Is that a permanent thing?
Most likely, but of course you or someone else can do it. If the contribution looks promising we'll take a look.
Comment 54 Raffi Khatchadourian CLA 2008-08-14 02:18:08 EDT
(In reply to comment #53)
> >Ah, I'm sorry to hear that. Is that a permanent thing?
> Most likely, but of course you or someone else can do it. If the contribution
> looks promising we'll take a look.
> 

Ok. In that case I'll shift my focus from the test cases to the UI.
Comment 55 Benjamin Muskalla CLA 2008-11-04 05:35:04 EST
Just a little note: the code still doesn't compile with 1.4 (usage of StringBuilder, String#contains and Class#getSimpleName).
By the way: Is there any progress on tests / a patch? Just tested the plugin a little bit and there are several big issues (constants not recognized in interfaces, doing the refactoring twice leads to exceptions, etc).
I wonder what's the status on the refactoring.
Comment 56 Raffi Khatchadourian CLA 2008-11-04 14:52:51 EST
(In reply to comment #55)
> Just a little note: the code still doesn't compile with 1.4 (usage of
> StringBuilder, String#contains and Class#getSimpleName).

Really? That's odd. Which version are you speaking of? Do you have the latest one attached above?

> By the way: Is there any progress on tests / a patch?

I was working on test cases, however, I redirected my attention to the UI as there was no progress on that.

> Just tested the plugin a
> little bit and there are several big issues (constants not recognized in
> interfaces, doing the refactoring twice leads to exceptions, etc).

That's very odd. I'm now curious if you are using the latest version. Nevertheless, I'll check in to it.

> I wonder what's the status on the refactoring.

Stagnant at the moment! Hopefully your message will spur things along!

Comment 57 Raffi Khatchadourian CLA 2008-11-09 16:52:49 EST
(In reply to comment #55)
> Just a little note: the code still doesn't compile with 1.4 (usage of
> StringBuilder, String#contains and Class#getSimpleName).

I set the compiler compliance level to 1.4 but unfortunately that doesn't seem to take into account the java libraries from that particular version.
Comment 58 Raffi Khatchadourian CLA 2008-11-09 17:46:08 EST
(In reply to comment #55)
> doing the refactoring twice leads to exceptions

The reason this is happening is due to the lack of a UI. The problem is that there is a default name for the new enum type. Thus, if you perform a refactoring twice it attempts to use the same default name. That would be fixed if/when the UI asks the user for the name of the new enum type.
Comment 59 Raffi Khatchadourian CLA 2008-11-09 17:47:03 EST
One thing I did notice that the plugin needs programmatically (i.e., not having to do necessarily with the UI) is a refactoring history in order to enable the "undo" feature.
Comment 60 Raffi Khatchadourian CLA 2008-11-09 17:51:40 EST
(In reply to comment #55)
> constants not recognized in interfaces

Do you have a specific example of this? It works fine for me.
Comment 61 Raffi Khatchadourian CLA 2008-11-09 17:56:06 EST
Created attachment 117407 [details]
Updated 1.4 complaint code
Comment 62 Raffi Khatchadourian CLA 2008-11-28 13:26:30 EST
Created attachment 119035 [details]
Added a few test cases

Initial test cases added. The code that actually runs the test cases still needs to be completed, however. I am currently taking a look at the MoveMembers refactoring tests to see how this is done.
Comment 63 Raffi Khatchadourian CLA 2008-11-28 13:28:01 EST
Created attachment 119036 [details]
Added a few test cases

Initial test cases added. The code that actually runs the test cases still needs to be completed, however. I am currently taking a look at the MoveMembers refactoring tests to see how this is done.
Comment 64 Raffi Khatchadourian CLA 2009-01-17 08:57:25 EST
Hello. I was just wondering when the deadline for the next milestone release of Eclipse will be. Thanks!
Comment 65 Dani Megert CLA 2009-01-19 03:34:38 EST
M5 is on January 30. Unfortunately the 3.5 deadline for external code which needs legal approval is January 31. Therefore it is very unlikely that this makes it into 3.5.
Comment 66 Raffi Khatchadourian CLA 2009-01-19 13:18:01 EST
(In reply to comment #65)
> M5 is on January 30. Unfortunately the 3.5 deadline for external code which
> needs legal approval is January 31. Therefore it is very unlikely that this
> makes it into 3.5.

Ah, I see, sorry to hear that. Are there any updates as to available resources (i.e., people) to produce a UI? Thanks for all your help and please feel free to answer at your earliest convenience!
Comment 67 Dani Megert CLA 2009-01-20 02:55:07 EST
>Are there any updates as to available resources (i.e., people) to produce a UI?
No.
Comment 68 Jan Matèrne CLA 2009-01-20 05:24:30 EST
(In reply to comment #65)
> M5 is on January 30. Unfortunately the 3.5 deadline for external code which
> needs legal approval is January 31. Therefore it is very unlikely that this
> makes it into 3.5.

For 3.5 (and maybe 3.4/3.3) users - would it be possible to have this feature via an update site as additional plugin? With hint that this should be integrated as base functionality in feature releases?
Comment 69 Dani Megert CLA 2009-01-20 05:30:49 EST
You can always do that but it would be your responsibility to maintain that update site and to make sure it works for 3.3/3.4/3.5 etc.
Comment 70 Raffi Khatchadourian CLA 2009-01-20 12:11:27 EST
(In reply to comment #69)
> You can always do that but it would be your responsibility to maintain that
> update site and to make sure it works for 3.3/3.4/3.5 etc.

That sounds feasible to me. Perhaps it can be hosted on google code or something similar?
Comment 71 Dani Megert CLA 2009-01-20 12:14:32 EST
>That sounds feasible to me. Perhaps it can be hosted on google code or
>something similar?
Whatever you like. Just make sure to check under what license it will be available. Best would of course be EPL so that it can easily be used.
Comment 72 Raffi Khatchadourian CLA 2010-04-05 12:36:47 EDT
FYI: The google code page for the project is located at http://code.google.com/p/constants-to-enum-eclipse-plugin/. There's a bit more work to do on the plugin, however. We have progressed somewhat, especially in thanks to Mr. Muskalla.
Comment 73 Jan Matèrne CLA 2010-11-07 18:46:59 EST
Will this plugin come into Eclipse or should this bug closed as the plugin is available at GoogleCode?
Comment 74 Raffi Khatchadourian CLA 2010-11-08 10:42:29 EST
(In reply to comment #73)
> Will this plugin come into Eclipse or should this bug closed as the plugin is
> available at GoogleCode?

The plan all along was to incorporate the plugin into the standard refactorings of Eclipse, however, we have stalled due to available resources, particularly in making a suitable UI and conforming to Eclipse standards. Although there is a UI at the moment that works, it's a bit limited as it requires the user to explicitly state the constants that will be refactored into a single enum type. The plugin, however, is capable of separating a set of constants into multiple, minimal enum types. That is, it can mine an entire project for all refactorable enum constants, and refactor several enum types at once.

Are there any resources (people) available to work on this project at the moment?
Comment 75 Markus Keller CLA 2010-11-29 10:22:28 EST
(In reply to comment #74)
Sorry, we still don't have resources in JDT/UI to do real work on this. I suggest we just leave this bug open so that others can find it. I don't want to close the door, but AFAIK, the code still needs some work before we can adopt it in the Eclipse project.
Comment 76 Raffi Khatchadourian CLA 2010-11-29 12:00:06 EST
Yes, I agree. There's plenty or work to be done on the code, especially concerning the UI. Hopefully this will get done in due time. 

(In reply to comment #75)
> (In reply to comment #74)
> Sorry, we still don't have resources in JDT/UI to do real work on this. I
> suggest we just leave this bug open so that others can find it. I don't want to
> close the door, but AFAIK, the code still needs some work before we can adopt
> it in the Eclipse project.