Community
Participate
Working Groups
foo.equals(bar) can be changed into bar.equals(foo) (as long as bar is not the null literal)
Depends on 3.0 planning
Created attachment 7360 [details] fix @@ implementation notes @@ All inversions are symmetric. b.equals((Object) o) will be inverted to ((Object) o).equals(b) ((Object) o).equals(b) will be inverted b.equals((Object) o) @@ test notes @@ I could not find any JUnit test classes for jdt-ui :/ This is the class I used to test by hand: public class Test extends Super { void test() { b.equals(c); sA.equals(this.b); "a".equals("b"); String s= "a"; "a".equals(s); "a".equals(A.get()); "a".equals(A.A); "a".getClass().equals(String.class); boolean x= false && "a".equals(get()); c.equals((Object) "a"); //should wrap in parentheses new Test().equals(this); "a".equals(null); //should not offer quick assist new Test().equals("a", false); //should not offer quick assist new Test().equals(false); //should not offer quick assist new Test().equals(sBool); //should not offer quick assist equals("a"); //should not offer quick assist } private String b= "b"; private String c= "c"; static String get() { return "a"; } boolean equals(boolean a) { //should not offer quick assist return false; } boolean equals(Object o, boolean a) { //should not offer quick assist return false; } public boolean equals(Object o) { //should not offer quick assist return super.equals(o); //should not offer quick assist } static boolean equals(String a) {//should not offer quick assist return false; } } class Super { protected String sA= "a"; protected boolean sBool= false; } class A { static String get() { return "a"; } static String A= "a"; }
I'll write some unit tests once you point me to the right place :D
Created attachment 7368 [details] fixed String concatenation, object assignment,tenary operator The following are properly handled now (surrounded by parenthesis if inverted out of the equals()): -String concatenation -object assignment -tenary operator resulting in an object Added 6 more tests: public class Test extends Super { void test() { b.equals(c); sA.equals(this.b); "a".equals("b"); String s = "a"; "a".equals(s); "a".equals(A.get()); "a".equals(A.A); "a".getClass().equals(String.class); boolean x = false && "a".equals(get()); new Test().equals(this); //parenthesis cases below c.equals((Object) "a"); "a".equals(s = "a"); "aaa".equals("a" + "a" + "a"); "a".equals(true ? "a" : "b"); //below should not offer quick assist "a".equals(null); new Test().equals("a", false); new Test().equals(false); new Test().equals(sBool); equals("a"); new Test().equals(true ? true : false); new Test().equals(1 + 1); int i= 0; new Test().equals(i + i); } private String b= "b"; private String c= "c"; static String get() { return "a"; } boolean equals(int i) { //should not offer quick assist return false; } boolean equals(boolean a) { //should not offer quick assist return false; } boolean equals(Object o, boolean a) { //should not offer quick assist return false; } public boolean equals(Object o) { //should not offer quick assist return super.equals(o); //should not offer quick assist } static boolean equals(String a) {//should not offer quick assist return false; } } class Super { protected String sA= "a"; protected boolean sBool= false; } class A { static String get() { return "a"; } static String A= "a"; }
The following is not symmetric: "a".equals(("a")) -> ("a").equals("a"); ("a").equals("a") -> "a".equals("a"); The extra parentheses are not necessary in the first place, so the "code cleanup" should not hurt .)
Great, thanks! Oh, yes, please add tests! :-) In plugin 'org.eclipse.jdt.ui.tests' look at class 'AssistQuickFixTest' how to do it. From a quick look at the code I wasn't sure if you cover the case when Expression left= method.getExpression(); is null if (equals(x)) {} -> if (x.equals(this)) {} Seems to me that runs in an NPE later Also note that parentized expressions can be nested multiple times. ((("a"))).equals(s); BTW: The replacing element must always be a copy rewrite.markAsReplaced(right, left); -> rewrite.markAsReplaced(right, rewrite.createCopy(left)); patch released > 20030108
Created attachment 7385 [details] now handles equals(x) and x.equals(this) equals(x) -> x.equals(this) x.equals(this) -> equals(x)
Well, I really like writing test but the way they're currently written is *rolls his eyes* What do you think about an API String getFileAsString(File) in QuickFixTest? Having those TestClasses als plain text files instead of the ugly StringBuffer contatenation would make writing those tests a lot easier and maintainable IMO. I could add that method and adjust existing code if you want me to. Maybe there's already that kind of functionality burried somewhere in the Eclipse code base ... If not I'll bite into the sour apple and do it by churning out those StringBuffer lines *smiles*
I personally think the StringBuffer story is _much, much_ better. The refactoring tests are using the scheme you're suggesting, but I have real problems with that. The thing is that this spread your tests in 3 files. If a test fails, and you want to debug it always takes me minutes to get the correspoding sources files (original and expected) and I'm swirling bewteen 3 editors. Bite in the source apple, I'm sure you'll see what I mean. Most of it is copy/paste anyways. Just keep the source examples small and have a test per tested example. Thanks a lot!
patch 7385 released > 20040109
The 3-editor-juggling is a good point didn't think of that :| Smells like a new feature "Insert File as StringBuffer statements" to me *smiles* It takes a file as an input and converts it to the StringBuffer scheme (one append()-statement per line in the input. Should make our work easier--one does not have to escape all that stuff anymore. Once I've taken all my tests in college I could work on it. What do you think?
Would definitly be nice to have! I think this should work with the new reg-exp find replace, but I'm not the expert. I mostly do Format the code to using spaces, indent it once. Select the indent, find/replace on one of these indent, replace with buf.append(" and add the end.
Created attachment 7403 [details] the test cases
Just an implementation note as a reminder for future questions: >>>Also note that parentized expressions can be nested multiple times. >>>((("a"))).equals(s); Each invocation of this quick assist will strip one pair of unnecessary braces. Example: ((("a"))).equals(s); -> s.equals((("a"))); -> (("a")).equals(s); -> s.equals(("a")); -> ("a").equals(s); -> s.equals("a"); -> "a".equals(s); It will create braces if needed: s.equals(true ? "a" : "b"); -> (true ? "a" : "b").equals(s); -> s.equals(true ? "a" : "b");
tests released > 20041012 Awesome, thanks a lot Sebastian! I modified the patch for the test by using a new 'collectAssists' method so you don't get any of the 'AssignToVariableAssistProposal''s.
