This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 111441 - [60cat]Generated equals() method uses != to compare strings
Summary: [60cat]Generated equals() method uses != to compare strings
Status: RESOLVED FIXED
Alias: None
Product: java
Classification: Unclassified
Component: Editor (show other bugs)
Version: 6.x
Hardware: All All
: P4 blocker with 6 votes (vote)
Assignee: malfunction84
URL:
Keywords:
: 118767 118783 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-07-31 10:26 UTC by Andrei Badea
Modified: 2008-08-28 06:40 UTC (History)
4 users (show)

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments
patch - proposed solution (2.40 KB, text/plain)
2008-08-25 04:59 UTC, malfunction84
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrei Badea 2007-07-31 10:26:04 UTC
070730.

1. Create a new Java class NewClass.
2. Add

    private String foo;

to the class.
3. Invoke Source - Insert Code - equals(). Include the foo field in the equals() method. Press Generate.

Part of the generated code is

    if (this.foo != other.foo && (this.foo == null || !this.foo.equals(other.foo))) {

for which the IDE displays a "Comparing Strings using == or !=" hint.
Comment 1 Jan Lahoda 2007-07-31 12:50:00 UTC
I do not see anything incorrect on the generated code itself. The main reason for the this.foo != other.foo expression
is this.foo == null && other.foo == null detection.
Comment 2 Andrei Badea 2007-07-31 13:29:59 UTC
Sure. But generating code which triggers a hint looks dumb.
Comment 3 Jan Becicka 2007-09-12 08:48:43 UTC
P4 imo
Comment 4 rationalpi 2007-10-06 17:32:54 UTC
The generated code would fail any serious code review and would be flagged by every static analysis tool.
Comment 5 Jan Lahoda 2007-10-13 10:02:42 UTC
*** Issue 118783 has been marked as a duplicate of this issue. ***
Comment 6 deeptinker 2007-10-13 11:49:47 UTC
IMHO, hints in NB are like a spelling checker in a word processing program.  They frequently catch mistakes, but not
aways.  In the case of the generated code, it is doing exactly what it should do - check the pointer to the object, not
the contents of the object.  However, this *looks like* a common code mistake - thus the hint.

So, what to do about the editor hint?  Let's borrow a solution from that word processing program.  I suggest that NB
allow me to "override" the editor hint.  Let me mark the hint on this line so that this hint doesn't display any more. 
If I later change this line of code, the editor should throw away the exclusion and show me the hint again.

This solution, to me, would be far more widely useful.  It would let me turn off hints at the specific places where they
are *not* helpful.  Editors and static analysis tools can only go so far.  It is us humans that must determine if the
code is correct or not.

If NB had this enhancement, then the generated code could generate the exclusion, too.  I have changed this issue to an
enhancement.  If you disagree, please add more comments and change it again.
Comment 7 rationalpi 2007-10-13 15:09:07 UTC
I think the generated code should be changed so that it does not use the == or != comparators. 

It looks silly for NetBeans to detect a problem and then to fix it by generating another problem. Even if this could be
fixed so that NetBeans ignored the generated code, that would still not be good enough in my opinion. As soon as someone
opened the same code in another IDE or ran automated tools like Checkstyle or FindBugs, it would flagged as well. To
completely suppress it a developer would have to use a handful of suppression comments to make all of the tools happy.
Then, after they got the tools happy they'd have to go to a code review and argue their case there as well.

It's just not worth the hassle. Quick-fixes are supposed to save a developer time. This one does not.
Comment 8 gugrim 2007-10-13 15:38:16 UTC
Don't agree. The comparision improves performance and should not be thrown out because some tools warn against it.
Comment 9 _ wadechandler 2007-10-13 15:41:32 UTC
I think it should just be left as is. If it is to ignore generated code then it would have to wrapped in some type of a
comment. If someone using another IDE removes this comment the warning will appear in NB, and it would appear in some
other IDE anyways. Then, if we tried to make it smarter...analyze string comparisons to try to detect this pattern then
we just slow down the editor. I think the warnings are just part of it, but in this case we are talking about an
artificial warning anyways. There are plenty of cases when the comparisons are perfectly valid. So, to me, the warning
for strings is something which can be turned off, and probably should be turned off by default. Look, people new to Java
probably aren't going to know they can not use == and != with their own classes. Someone who has been working with C/C++
for years...I would say shame on them ;-) should have known C/C++ == is just an override. Other languages...they will
need to know this anyways. So, there are good arguments really for either way, but the main problem here is that a
warning is being generated for code which the compiler won't be generating a warning, so it is already doing something
outside the bounds of what it needs to do if you ask me as it ends up putting the development of something so small into
such a context as it capitalizes on more time that it really needs just to generate a warning for something which is
actually part of the language...generating a warning for comparing references. If anything is to be removed...I would
argue the warning itself should just be removed all together, and if not, then this warning in this case should be lived
with and maybe for this warning the warning message can have text appended to it explaining what it means in Java and
that in some instances, but only for comparing references, it is OK, and maybe point the user to some specifications so
they can learn the difference.
Comment 10 rationalpi 2007-10-13 16:21:26 UTC
I understand the optimization, but also know that the String pool already takes care of this optimization. There is no
performance gain with this code.

I'm having flashbacks to tabs vs. spaces and placement of braces debates. :-)

Josh/RationalPi
Comment 11 Jiri Prox 2007-10-15 08:22:47 UTC
*** Issue 118767 has been marked as a duplicate of this issue. ***
Comment 12 Petr Hrebejk 2007-10-19 12:33:53 UTC
The generated code is obviously OK. The only think I can do is to add something like:
@SuppressWarnings("string-comparison-by-operator). 

What?
Comment 13 _ theanuradha 2007-10-19 12:39:37 UTC
>>The generated code is obviously OK. The only think I can do is to add something like:
>>@SuppressWarnings("string-comparison-by-operator). 
Please do that I suggest add small comment way this @SuppressWarnings("string-comparison-by-operator)
Comment 14 Petr Hrebejk 2007-10-19 12:51:25 UTC
What?

"I suggest add small comment way this @SuppressWarnings("string-comparison-by-operator)"

I don't understand the sentence. 
Comment 15 _ theanuradha 2007-10-19 12:57:11 UTC
Sorry 

I suggest to add comment to generated method like  @SuppressWarnings("string-comparison-by-operator")//why we add this
 
Comment 16 Andrei Badea 2007-10-22 14:04:47 UTC
Pardon my French, but generating code which needs @SuppressWarnings() and then a comment explaining why that
@SuppressWarnings() is there looks stupid. I also suppose "string-comparison-by-operator" would be a NetBeans-specific
token, right? Yuck.

I would suggest generating different code when String's would be compared. The generated code is nice and works fine for
other types than String, so keep it. Just do it another way for String's.
Comment 17 deniss 2008-05-10 00:20:50 UTC
I think that reference comparing should be removed because equals method should handle this, there is an unwritten(or
not?) law that equals method should be null safe.

if (this.foo == null || !this.foo.equals(other.foo)) { ... }

may be some settings option for this?
Comment 18 malfunction84 2008-08-01 19:42:08 UTC
If comparing primitives, it should use ==.  If comparing Objects, it should use .equals().  Before using .equals(), it
should check for null.  It's silly to have to rewrite the same parts of a generated method every time.  The following
block is great for a short-circuit comparison of Object fields.

public boolean equals(Object obj) {
    /* SNIP */
    if ((this.foo == null) ? other.foo != null : !this.foo.equals(other.foo)) {
        return false;
    }
    return true;
}

For Object comparison, == is the exception, not the rule.  If foo's .equals() method wants to perform a == comparison
internally, then that's the best place for that optimization to happen: inside its own .equals() method.

But you know, everyone is going to have a different preference.  Why not just let the code template itself be editable?
 Then you can set it up however you want.  I have filed Issue #142630 to that end.
Comment 19 _ wadechandler 2008-08-01 20:26:39 UTC
Let's be clear though, there is no doubt that unless o == null (o.equals should throw an exception as you can't call
methods on null references) then o.equals(o) should return true, and thus this argument is mute in itself because there
is no need to make another method call to see whether o==o should end up being equal to o.equals(o). Thus:
o.equals(o)&&(o==o)==true.

and if this is ever not the case, then someone's code is wrong...there really isn't anything left to preference with
this issue. At no point should o.equals(o) not return true in any circumstance which would make logical sense.

Thus it is also true that if o==x, and x is an Object instance, they are references to the same instance, and a
superfluous call to o.equals(x) is gong to return true, and the logic which uses == on the instance variables is thus
correct and not wrong. This is the case with both local and remote instances.

There should be no reason to rewrite logic which simply determines whether x==o on object instances as a basis for
equality even with Objects. Obviously if Object o is the same instance as Object x they are equal. Now, the other way
x!=o can not be correctly determined for object instances because the field values are not compared by this operation,
as is the case with String and other classes, but if x==o then the field instances have to be equal as one can not
change field y of instance x without changing field y in o when x==o. It is simply not possible in normal circumstances,
and in the chance we're talking multi-processing, not using volatile can result in loss of information regardless of
instance or method comparison.

Also, I have no objection to this being configurable, but I do want the issue to be clear. The real issue is that the
editor shows a hint to change something which is perfectly valid, and something which can keep the stack from being
accessed unnecessarily. The stack and efficiency stuff is just icing as in almost all cases that little difference in
speed and memory usage won't make a difference, but it is still a perfectly valid use of ==.
Comment 20 malfunction84 2008-08-01 21:43:49 UTC
Of course a call to .equals() (equality test) can be avoided by testing for == (identity test) first.  This is because
the reflexive property of .equals() is a must.  If x == y, then x.equals(y) is equivalent to x.equals(x).  This
short-circuit identity test is already done by the currently generated code.  But there is nothing *wrong* with moving
that short-circuit identity test to inside the .equals() method.  In fact, you you look at the implementation for
String.equals(), you'll see it's the first thing in there.

Okay, now this has digressed into a philosophical discussion on proper OO design.  Let's try to get back to the issue at
hand.  We already know that a != test by itself isn't sufficient to rule out equality, which is why it calls .equals().
 But for comparing Strings specifically, a short-circuit call to != or == actually IS unnecessary.  That's why I think
the snippet above is ideal for String comparison.

One obvious fix is to disable the warning about String comparison using ==.  If you're okay with comparing Strings with
== instead of .equals(), disable the hint:

Tools > Options > Java Code > Hints > General > Comparing Strings using == or !=

Another option is to just have the .equals() code generation spit out different for comparing Strings which omits any ==
or != operations.
Comment 21 _ wadechandler 2008-08-02 01:50:40 UTC
The point I'm making isn't about whether it is good OO design or not, this really doesn't have anything to do with OO as
much as Java equality mechanisms, but simply that the code being produced is not incorrect, and giving the reason why it
is correct regardless of preference. The short circuit simply isn't "incorrect" whether this becomes user configurable
or not, and the fact that a hint is being generated for something which is perfectly fine, correct within the context
used, and provides less memory and process overhead than a method call in the times the instances are the same, which
must copy the reference and use the stack, is the thing I'm stressing.
Comment 22 _ wadechandler 2008-08-02 01:55:03 UTC
Also, the short circuit works for other Object types besides String.
Comment 23 malfunction84 2008-08-02 08:38:09 UTC
I'm sorry if I wasn't clear before.  Let me reiterate.  Using == to compare Strings is *always* unnecessary, since
String.equals() performs a == test already.  There is no reason to ever use == to compare Strings, and the hint exists
to help you avoid it.

Let me be clear about this too: the short-circuit == test to avoid a potentially expensive call to .equals() is
perfectly fine when comparing any other kind of Object.  The only time it's truly unnecessary is when comparing Strings.

If you're that concerned about efficiency, consider this.  Using the short-circuit == to compare Strings before invoking
.equals() would actually be less efficient.  In the case that they are not the same instance, the code would perform the
== comparison twice, once for your external short-circuit, and once inside the String.equals() method.  Why perform ==
externally when you know it is already performed by String.equals()?

In short, when NetBeans generates code, it shouldn't then complain about the code it generates.  There should be a
special case for comparing String fields in a generated .equals() method, a case which avoids the redundant ==
short-circuit on Strings.
Comment 24 Jan Lahoda 2008-08-04 12:09:00 UTC
Please note that the main reason for the current code is not performance. The main intent of != is handling of situation
when this.foo == null && other.foo == null. Any performance gains are purely accidental. The condition can be written in
many ways (including the one proposed by malfunction84), but this one seemed to be the simplest/clearest to me.

I do not think that we should compare Strings in a different way than other objects - if the test is good enough for any
(and all) custom object(s), then it is good enough for String, IMO.
Comment 25 Max Sauer 2008-08-04 13:41:49 UTC
Fixed. Improper hint wont be shown anymore.
---
http://hg.netbeans.org/main/rev/0c21dec795cf
Comment 26 malfunction84 2008-08-04 19:03:25 UTC
I *really* don't think suppressing the hint inside .equals() is the best solution.  The hint exists to remind the
programmer that != and == do not perform String comparisons.  Normally, if they make this mistake, they will see this
very useful hint.  Now, if they happen to make the mistake in a .equals() method, they will not see the hint.  This is
not consistent behavior.

If the intent of using != in the generated code is to test for null, why not explicitly test for null?  It will clarify
the code, work for any type of Object (including String), and avoid using code which causes this hint to be displayed,
while still allowing the hint to be displayed in the cases for which it was designed, even in a .equals() method.
Comment 27 Max Sauer 2008-08-05 08:37:58 UTC
We've discussed it long time before choosing this solution.

Explicit null check will be necessary for every String field (or every variable, in case String wouldn't be treated specifically, as you suggested) included in 
equals -- which will enlarge amount of generated code. IMHO, everyone is able to comprehend the code currently generated.

Besides, the pattern (inside equals() and inside logical AND tree) looks narrow enough to me.

Any other opinions? Thanks.
Comment 28 Petr Dvorak 2008-08-05 10:57:34 UTC
I am following this discussion with a little smile on my face - it is one of the tiny (so, so tiny) problems in the
universe... :)

I will give it a shot - excuse me if I write nonsense (I am a lame programmer...):

0. Original statement: A & (B | C)

this.foo != other.foo && (this.foo == null || !this.foo.equals(other.foo))

1. Using 1st order predicate logics: (A & B) | (A & C)

(this.foo != other.foo && this.foo == null) || (this.foo != other.foo && !this.foo.equals(other.foo))


> Explicit null check will be necessary for every String field (or every variable, in case String wouldn't be treated 
> specifically, as you suggested) included in equals -- **which will enlarge amount of generated code**.

No, I don't think so - code that is equivalent does not need more formulas, see bellow:

2. First sub-statement of 1. can be rewritten with the same number of statement, as "null" is a logical constant:

this.foo == null && other.foo != null

3. Second sub-statement of 1. can (don't have to be) be rewritten so that you omit the reference check (equals does not
compare references but objects), but I don't know if leaving 2ns sub-statement unchanged does not gain some performance:

!this.foo.equals(other.foo)

Therefore, the equivalent statement (that is not triggering the warning) would be:

(this.foo != null && other.foo == null) || (!this.foo.equals(other.foo))

[Note: this is equivalent to the ternary operator mentioned above (that I actually like quite a lot, as I did most of my
code in C++/WinAPI), just in a little more human-friendly form]

> Besides, the pattern (inside equals() and inside logical AND tree) looks narrow enough to me.
I think code above is also well readable and narrow... Right? ;)
Comment 29 Petr Dvorak 2008-08-05 11:35:49 UTC
... sorry - the 3rd step would also require the null check on "this.foo"...
Comment 30 Petr Dvorak 2008-08-05 11:45:28 UTC
... and a typo in the resume... hmmm... nice...
Comment 31 malfunction84 2008-08-05 15:52:24 UTC
joshis, the second clause in your statement would throw a NullPointerException if this.foo is null.  The first clause
would return false, but because it's ORed with the second, it would not short-circuit and the second clause would still
execute, generating the NPE.

Would you be willing to change the generated code for Object comparisons?  I believe a simple change meets everyone's
criteria.

Current code:
if (this.foo != other.foo && (this.foo == null || !this.foo.equals(other.foo)))

Proposed code:
if ((this.foo == null) ? (other.foo != null) : !this.foo.equals(other.foo))

This code is clear, has an explicit null check, avoids the warning if foo is a String, and uses fewer characters than
the original code.  It also allows you to undo the previous change which causes inconsistent behavior for the String
comparison warning.
Comment 32 Petr Dvorak 2008-08-05 15:55:22 UTC
> joshis, the second clause in your statement would throw a NullPointerException if this.foo is null.

This is why I added a comment bellow saying that the 3rd step needs a null check...

> ... sorry - the 3rd step would also require the null check on "this.foo"...
Comment 33 Petr Dvorak 2008-08-05 15:57:33 UTC
> if ((this.foo == null) ? (other.foo != null) : !this.foo.equals(other.foo))

I personally like this - why not to give a renaissance to a ternary operator?
Comment 34 malfunction84 2008-08-05 16:05:40 UTC
You're right.  I'm sorry; I missed that part.

So you're proposing:
(this.foo != null && other.foo == null) || (this.foo != null && !this.foo.equals(other.foo))

Since both clauses contain "this.foo != null", by the distributive property, this can become:
((this.foo != null) && ((other.foo == null) || !this.foo.equals(other.foo))

Which is actually *very* similar to:
((this.foo == null) ? (other.foo != null) : !this.foo.equals(other.foo))
Comment 35 Petr Dvorak 2008-08-05 16:28:06 UTC
I made that confusing - sorry, I will cite myself again:
>... and a typo in the resume... hmmm... nice.

I made a typo in the first substatement of the resume (wrong negations) - but if you read the contents of the long
comment ignoring the resume and comments bellow, you will see I proposed:
(this.foo == null && other.foo != null) || (this.foo != null && !this.foo.equals(other.foo))

I also placed a note bellow the incorrectly concluded resume:
> [Note: this is equivalent to the ternary operator mentioned above (that I actually like quite a lot,
> as I did most of mycode in C++/WinAPI), just in a little more human-friendly form]
Comment 36 Quality Engineering 2008-08-05 16:38:52 UTC
Integrated into 'main-golden', available in build *200808051401* on http://bits.netbeans.org/dev/nightly/
Changeset: http://hg.netbeans.org/main/rev/0c21dec795cf
User: Max Sauer <msauer@netbeans.org>
Log: #111441: [60cat]Generated equals() method uses != to compare strings
Comment 37 Jan Lahoda 2008-08-05 16:58:01 UTC
As long as we are solving issue "a warning is shown for auto generated code", I see two possible solutions:
-change the generated code: e.g. by use the ternary operator (which seems quite strange in Java to me and I would guess
it would be confusing for a lot of users), or by using some other proposed expression (considering only the valid ones,
which are usually more cumbersome than one that is used currently). Moreover, the currently generated code is perfectly
valid, to my knowledge, so this seems more like a workaround.
-fix the hint to understand the generated expression and not issue the warning. I would strongly prefer this solution
(even if the hint would be change to ignore this specific expression).
Comment 38 Petr Dvorak 2008-08-05 17:09:29 UTC
> fix the hint to understand the generated expression and not issue the warning. I would strongly prefer this solution
> (even if the hint would be change to ignore this specific expression).

I have personally no problem with this solution...

> Moreover, the currently generated code is perfectly valid, to my knowledge, so this seems more like a workaround.

Well, but imagine that you would not use the code completion and you would type equals method manually. Then you would
see the hint in the IDE. You wouldn't try to avoid this warning? I know that the code is correct... but I don't like
warnings in my code - what I would think: "There are cool Sun programmers that know Java perfectly and they are trying
to tell me that this type of comparison I am making is not fully OK"...

> cumbersome

???

(this.foo == null && other.foo != null) || (this.foo != null && !this.foo.equals(other.foo))

This literally says:
(this.foo == null && other.foo != null)           ~ If this.foo is null, is other.foo not null? If yes,return false.
||                                                ~ If the previous condition does not hold, check the one bellow...
(this.foo != null && !this.foo.equals(other.foo)) ~ If this.foo is non-null, are the objects equal? If yes,return false.

I will cite myself one more time:
> I am following this discussion with a little smile on my face - it is one of the tiny (so, so tiny) problems in the
> universe... :)
Comment 39 malfunction84 2008-08-05 18:17:01 UTC
jlahoda,

I agree that those are the only two solutions.  We just happen to prefer different approaches.

What code is better or makes more sense is completely subjective.  We may all have different opinions about that.

But objectively, causing the hint to behave differently based on where the code is will introduce inconsistency. 
Whether inconsistency is good or bad is subjective.  I foresee this being brought up as a separate issue down the road
("No hint for 'Comparing Strings using == or !=' in .equals() method") because people will expect consistency.  Someone
might legitimately mean to use String.equals() in their .equals() implementation and accidentally use == or !=.  With
the most recent change (which was just merged into main-golden), they will not receive this hint.

Please roll back the most recent change, as it introduces an inconsistency in how the hint is displayed.  Let us address
this issue in a way that does not compromise consistency.

And to that end, I think we should change the generated code.

The currently generated code is indeed perfectly valid, even for Strings.  The reality is that if you're performing a ==
on a String and then perform a String.equals() on it in the same clause, it's probably fine.  But the hint currently
isn't smart enough to statically evaluate your code in that way.  (Is this another option, perhaps?)

I honestly believe we can agree on new code that is valid, clear, consistent, and won't generate this warning for
Strings.  Several proposals have been made already.
Comment 40 Jan Lahoda 2008-08-05 18:23:42 UTC
I am not proposing to disable the hint in the equals method. I am proposing to learn the hint that the (specific)
condition is correct (which, honestly, is the most difficult solution that was proposed so far).
Comment 41 Petr Dvorak 2008-08-05 18:44:02 UTC
> which, honestly, is the most difficult solution that was proposed so far

Indeed it is, because you need to teach/explain NB users why this specific condition is OK...

I can see it in colors - issues saying "Reference comparison does not trigger warning" etc. I believe it is always a
problem when the software is smarter than people who use it...
Comment 42 malfunction84 2008-08-05 19:45:07 UTC
The revision which was merged into "main-golden" *does* disable the hint in .equals().  Sorry, I assumed you were
involved with that change.  I still believe that revision should be undone, due to the inconsistency it introduces.

I'm not sure having the hint recognize specifically the generated code is a good idea either.  It introduces the
possibility that this issue will come up again if that code is ever changed, especially if that generated code is ever
made to be editable (see Issue #142630).

If the hint could recognize that a String.equals() is used in conjunction with a == or != comparison, that would be a
more generalized, consistent solution that everyone might agree on.  And it would also be, by far, the most difficult
solution to implement.  It would be difficult enough just to explain.

I don't understand why you so adamantly oppose changing the generated code.  I agree that the current code is valid, but
so is alternative code which avoids a direct == or != comparison between the fields.  Many of the commenters on this
issue agree that the generated code should be changed.

Strings are special, which is why the hint exists in the first place.  If you oppose changing the generated code for all
cases, please consider changing it for String comparisons.
Comment 43 Jan Lahoda 2008-08-05 23:07:08 UTC
Making the hint clever enough to recognize when the reference comparison is OK and when not is out-of-scope for this
issue (and I would argue it is generally impossible). It is possible, though, to disable the hint altogether (and I am
actually growing into supporting this option :-) ).

Could you please list the advantages of changing the code (and keeping the hint as it is)? I see quite a few disadvantages:
-currently existing code will still be marked by hint
-if the user changes the code to the current one, it will still be marked by the hint (considering also issue #142630)
-people that are satisfied with the current code will complain

Also, I do not remember any other place people would be complaining about the code, only this issue. So it appears to me
the main problem is that the generated code is marked with the hint.

I strongly disagree with making the code for Strings more complex than for arbitrary user's object. We know much more
about Strings than about user's objects. If we can use the knowledge to make the code simpler for Strings, that is OK.
But I disagree with using the knowledge to make the code more complex.
Comment 44 malfunction84 2008-08-06 00:42:00 UTC
All very good points.  I agree that making the hint clever would be nigh-impossible.  :)

Before listing what I believe are the advantages, let me first address your disadvantages:

> currently existing code will still be marked by hint
Sure, and why not?  The hint is there to discourage String comparison using ==.  The problem is not that the hint *is*
displayed (people do have the option to disable it), but that it is displayed on code that was just generated by the
IDE.  People who see this wonder to themselves, "Why doesn't the IDE consider its own hint and use different code?"

> if the user changes the code to the current one, it will still be marked by the hint (considering also issue #142630)"
Yes, as it should be (see above).  This is consistent behavior for the hint, discouraging use of == on Strings anywhere.
 String.equals() already performs this test, so there's really no reason to do it unless testing for null.  I don't see
this as a disadvantage either.  A user whose custom code does this should see a hint, too.

> people that are satisfied with the current code will complain
People are going to complain either way.  I present exhibit A: myself.  :)  There is always the option to change the
generated comparison for String fields only.  And for the really hardcore complainers, there is always Issue #142630.

There is a fourth disadvantage you mentioned, indirectly.
> Making the code more complex for String comparison is bad.
Simplicity does not always mean clarity.  Assuming it would be more complex to handle Strings differently, why is this
bad?  Besides, how much complexity are we talking about, here?  One more clause?  We want the IDE to generate smart,
usable code.  I think most developers *would* and *do* handle String comparisons in an .equals() method differently than
comparisons of other types.  (Especially when a hint in their IDE suggests it!)  But even so, I don't think this
alternative approach for Strings would necessarily be any more complex than the currently generated code.

Alright, the advantages, as I see them, to using a special comparison for Strings, or changing the base comparison code:
- Allows more consistent behavior for the display of the hint.  It can simply always be displayed, without the need for
any fancy rules which users will not intuitively understand.
- The code can be made simpler.  Most of the proposed clauses use fewer characters than the current code.  But even if
there *are* more characters, that doesn't mean the code is more complex.
- The code can be smarter.  There's no reason why we shouldn't use the knowledge we have to make the IDE generate the
best possible code.  If this involves conditionally generating different code in different circumstances, that is a
*good* thing for the user.
- The code can be made clearer.  The clause "this.foo != other.foo" does not look like a test for null at first glance,
but it serves this purpose (as well as being a short-circuit).  This is *less clear* than an explicit test for null.  In
contrast, my ternary proposal uses three very straightforward clauses, with parentheses to help make it readable.  Or
joshis's proposal, which includes explicit null checks and is really just an expansion of the ternary (for those of you
who hate the ternary operator).

Thanks for hearing me out.  I hope I'm not frustrating you guys.
Comment 45 Jan Lahoda 2008-08-07 09:13:22 UTC
I have disabled the for currently generated condition:
http://hg.netbeans.org/main/rev/0b3b94004d91
Lets see what will be the outcomes.

I personally do not think there is a good reason to change the generated code. If you disagree, please feel free to file
an enhancement (against java.editor).
Comment 46 malfunction84 2008-08-07 15:58:42 UTC
I don't understand what your changes do.  Can you explain?

A separate issue would simply be a duplicate of this one.  Recall the summary for this bug: "Generated equals() method
uses != to compare strings."  That really is the core of the problem.  As we discussed before, there are two ways to
resolve this issue: changing the hint or changing the generated code.  Out of curiosity, I have gone back through the
comments and tallied who is in favor of which solution.

5 - Change generated code for Strings (abadea*, deniss, joshis, malfunction84, rationalpi)
2 - Change how hint is applied (jlahoda**, msauer**)
2 - Allow developers to suppress hint on case-by-case basis (deeptinker, theanuradha)

* Note that this is who originally filed the issue.
** Note that these are both NB developers.  No other users have voiced their support for this solution.

Based on this, I have changed the subcomponent on this bug to "editor" and reopened.

The code which is generated for comparing Strings should be changed to avoid using !=, hint or no hint.  For those who
would like to change how comparisons of other types are generated, I have already filed Issue #142630.

I will stress again, the hint was already working perfectly, so there was no reason to change it.  The changes made in
http://hg.netbeans.org/main/rev/0c21dec795cf should be reverted.  Do you agree that they introduce an inconsistency?  Do
you agree that inconsistency should be avoided?
Comment 47 Jan Lahoda 2008-08-07 16:27:58 UTC
I pasted an incorrect link above. The correct one is:
http://hg.netbeans.org/main?cmd=changeset;node=4f6c5daf99d6
(thanks to Max for notice)
The hint simply checks whether the condition is the same as the condition generated by the equals generator.

I still do not think the code should be changed (and, currently, I would be the one responsible of resolving any
problems that would arise from such a change). If you can provide a patch and promise you will resolve any issues
arising from the change, I will be happy to integrate it. Otherwise, please give me a very specific and very good reason
to why change the code to a particular form that will be better than the current one (no references to the hint please -
the code should be better than the current one itself, regardless of the hint).

<rant>If we will change the code to read: ((this.foo == null) ? (other.foo != null) : !this.foo.equals(other.foo)), and
someone will create a hint that will claim that using ternary operator inside if condition is not a good practice (which
is true in my opinion), will you challenge the hint or the generated code?</rant>

Changes from 0c21dec795cf do not exist anymore, to my knowledge.
Comment 48 Quality Engineering 2008-08-07 16:36:46 UTC
Integrated into 'main-golden', available in build *200808071401* on http://bits.netbeans.org/dev/nightly/
Changeset: http://hg.netbeans.org/main/rev/4f6c5daf99d6
User: Jan Lahoda <jlahoda@netbeans.org>
Log: #111441: disabling the "Incorrect String reference comparison" hint for the code that is autogenerated in the equals method.
Comment 49 _ wadechandler 2008-08-07 17:02:44 UTC
The issue with String versus other Object doesn't matter in the context of the hint. The hint is there because some
people falsely think == and != are the same as .equals and !.equals which would be incorrect. This rule applies to both
String and non-String class types just the same. The hint isn't based on the fact that String just happens to have a is
this==instance inside of its equal method, is it based on one not being able to use it for value comparison. Reference
comparison is a perfectly legal and valid use. 

However, as someone mentioned, this has really become a mountain out of a mole hill type issue now, and it really is
about preference at this point. In reality, tools, marking this as an error in all situations, is the real cause of the
issue; that coupled with operator overloading in other languages, whether hidden from the user or not (C++). The
shortest method to a resolution at this point, beyond repetitive back and forth, seems to be the best solution. I don't
think constantly reopening and closing the issue is the best answer, and someone isn't going to be happy with any given
solution, I think we just need to pick one which covers the situation, and is legal valid and working code which gets
the job done.
Comment 50 malfunction84 2008-08-07 19:41:15 UTC
Yes, 4f6c5daf99d6 has overwritten the changes introduced by 0c21dec795cf.

jlahoda, your terms are acceptable.  :)  I will begin work on a patch.

In regards to your rant, I would not challenge either.  I believe there is a way to avoid pitting generated code against
hints: simply have the code generation be aware of which hints are enabled.  Bear with me, now.  The hints represent
developer code preferences, right?  If we honor those preferences when generating code, we should never run into the
situation where the IDE generates code which is marked with hints.  And the hints will still be displayed in existing
code as expected.

So for instance, if the "Comparing Strings using == or !=" hint is enabled, we'll generate code that avoids using == or
!= to compare Strings.  Otherwise, we'll use the default code.  The same would be true of your ternary avoidance hint. 
If the hint is enabled, the code generation would not generate code which uses a ternary.

I think this is actually a really good compromise that will please everyone.  If no one objects, I will focus my efforts
on implementing this.
Comment 51 Jan Lahoda 2008-08-07 20:32:55 UTC
(Not counting technical difficulties) I am not sure if creating code depending on enabled/disabled hints is a good idea:
what if the user will generate some code and then enable a hint? Suddenly, his/hers code will be covered with warnings.

Customizable code would be a better solution, IMO (but cannot be done for NB6.5, and a good default is needed anyway -
most people will not change it).
Comment 52 malfunction84 2008-08-08 00:40:11 UTC
I don't see a problem with that.  That's why the warnings exist.  It would be giving warnings on code that *was*
generated, not warnings on code that *is being* generated.  The IDE can't tell what code *was* generated, but it can
tell what code *is being* generated because it's doing the generating.

I don't know.  I can see the hint setting acting more like a universal "treat Strings differently than other Objects
when using == or != to compare" setting.  It could control comparison warnings, code generation, and perhaps other
behavior as well.  And of course, you could always turn if off.  Thoughts?  Maybe that's a separate issue, literally.  :P

Anyway, for now, I will only work to change the generated code for String comparisons in a .equals() method.  It won't
be keyed off hint settings or anything.  That has the smallest impact, and it's the simplest resolution.
Comment 53 malfunction84 2008-08-25 04:59:35 UTC
Created attachment 68216 [details]
patch - proposed solution
Comment 54 Jan Lahoda 2008-08-25 16:32:10 UTC
Thanks for the patch, it seems fine - I will integrate it with your name if you agree.

BTW: I still do not understand what is so special about j.l.String (compared to e.g. j.l.Integer) that we generate a
special code for it, but that is your problem now :-).
Comment 55 malfunction84 2008-08-25 22:13:39 UTC
Yes, please integrate the patch with my name.  This also means you can revert 4f6c5daf99d6 since it no longer generates
that code for Strings.

But yeah, I understand your point.  I consider String to be different because (1) its .equals() method is different and
(2) Java treats Strings differently than other other languages, particularly PHP.

The .equals() methods for the primitive wrapper classes don't internally perform a == comparison.  This means the two
types of comparison are exclusive.  For primitive wrappers (as with most other objects), .equals() is for comparing the
values that instances contain, whereas == is used to discriminate between instances representing the same value.

With Strings, .equals() performs a == comparison internally, so using .equals() is sufficient by itself for both
comparing represented values and discriminating between instances.  This renders == unnecessary.  That and the fact that
those new to Java use it incorrectly (the reason the warning exists) suggests to me that Strings need special consideration.

I say this only to summarize my position for the sake of anyone who comes into this new, not to reignite the debate.

Hooray.  I get to field all objections.
Comment 57 Quality Engineering 2008-08-28 06:40:16 UTC
Integrated into 'main-golden', available in build *200808280201* on http://bits.netbeans.org/dev/nightly/
Changeset: http://hg.netbeans.org/main/rev/baf21a21ca77
User: Mark Lewis <malfunction84@netbeans.org>
Log: #111441: [60cat]Generated equals() method uses != to compare strings