1

SonarQube rules are enforced in my dev project, and I am not sure that the rule "S1698" ([MAJOR] Change this comparison to use the equals method) is relevant in several of my cases (and I still need to do something for these warnings). Do you see why my code/algo would be armful and why it should be mitigated by implementing equals or revisiting the algo?

First simple case

I have an array of (mildly complex) objects. I need to find the most relevant and display them all in a combo-box (the most relevant being selected). Pseudo-code:

List<MyObject> list = ...;
MyObject bestMatch = findBestMatch(list);
for(obj : list) {
  addToCombo(obj, obj == bestMatch /* isSelected */);
}

I could use "obj.equals(bestMatch)" (which I did not need to implement), which in turn would call ==, but if someone one day implements "equals", my code may call a deep comparison which is not needed.

Other cases

I have a graph of objects which are never copied, are defined by their instance and for which the comparison by full value is not really relevant. In my graph I could have 2 objects with the same content, but I need them to be distinct objects (the user will have to edit them until their value become different).

I need to index them in several manners (ex: by name, which may not be unique) to make fast requests like "is there any other object in the graph with the same name and that is not disabled". I have written such search method like this pseudo-code:

boolean isNameInConflict(MyObject testedObject) {
 List<MyObject> list = nameIndex.get(testedObject.getName());
 for(obj : list) {
  if (obj != testedObject && !obj.isDisabled()) { return true; }
 }
 return false;
}

}

Once again, I could call equals (which I don't need to implement), but I feel it could make the maintainer think that I did implement it. Or I could generate for each object a unique auto-incremented ID (my objects only live in a well-defined scope and are never saved or transmitted).

Fabimaru
  • 423
  • 3
  • 9
  • 3
    In my view a warning is just a hint to have a closer look at the code in question. If you find it is ok as it is than that's fine. – Henry Oct 19 '16 at 07:16
  • 3
    On the other hand, a good equals method should always check for reference equality first, so calling equals should not be a performance problem at all. – JB Nizet Oct 19 '16 at 07:27
  • I should have written that the "critical" and "major" warnings are **strictly** enforced, and that the use of '@SuppressWarning' will be monitored. – Fabimaru Oct 19 '16 at 07:44
  • @JBNizet Stupid me, I think you answered my question. Using the default "equals" looks like the right solution. – Fabimaru Oct 19 '16 at 07:46
  • @JBNizet if the two objects are different (which is probably the case most of the time) the more expensive check will be done. While this overhead my be negligible my point actually is that fixing false positive warnings does not help quality at all. It just causes useless work. – Henry Oct 19 '16 at 09:32
  • @Henry I agree. I overlooked that problem. – JB Nizet Oct 19 '16 at 10:58

0 Answers0