Java S1871
SonarQube Cloud and IDE
Sonar thinks “father = d” is duplicated, but ‘d’ is not the same in both branches.
class Parent {}
class Mom extends Parent {}
class Dad extends Parent {}
public final class Test
{
private Test(){}
public static void main(String[] args)
{
Dad father;
Parent a = new Dad();
Parent b = new Mom();
if (a instanceof Dad d)
{ father = d; }
else if (b instanceof Dad d)
{father = d;}
}
}
Hi,
In my opinion there is real code duplication, you have same Dad test and initialization logic twice.
You can refactor, maybe create a auxiliar method, lets call it getFirstDad that receives all parent dad candidates and iterates over them and retuns the first Dad instance he finds. Or on more modern style use a stream of parents to replace the explicit iteration.
Or if you want it short maybe this could work:
class Parent {}
class Mom extends Parent {}
class Dad extends Parent {}
public final class Test
{
private Test() {}
public static void main(String[] args)
{
Parent a = new Dad();
Parent b = new Mom();
Dad father = (a instanceof Dad d) ? d : (b instanceof Dad d2) ? d2 : null;
}
}
The code example was simply a way to show the issue, it is not the real code which is much more complicated.
It really is not code duplicated because ‘d’ is referring to different objects in both cases. Yes I could refactor the code as you indicated by not naming the variables the same that is just getting around the FP of sonar.
I should be able to name them the same. Naming them different proves my point that Sonar is incorrect and should NOT flag as duplicate.
It might be a matter of opinion, but I still have strong opinion it is a correct duplicated code. I will give you an example, if we change father variable name to firstFather, we have to change both assignments of both if cases. For me it is duplication even if the Dad instance created by each instanceof has a different name.
The actual code is as follows:
if (loc.containsCarrier(carrierId))
{
if (loc **instanceof** Port port)
{ currentLocation = port;}
else if (loc instanceof Transit && (workItem != null) &&
workItem.getNextLoc() **instanceof** Port port)
{currentLocation = port;}
}
That code is more complex, there is more logic involved, not just the name of the variable, I see your point now.
I argue that rule might still make sense for other cases, and here you have opportunity to try to make the code more clear, renaming port might be the best approach.
I dislike having 3 conditions in the else if… not sure if this would be more clear, not sonar issue free ensured 
if (loc.containsCarrier(carrierId)) {
if (loc instanceof Port port) {
currentLocation = port;
} else if (loc instanceof Transit && workItem != null) {
if(workItem.getNextLoc() instance of Port nextPort) {
currentLocation = nextPort;
}
}
}
Creating more complex things does not feel justified just for an assignment of a variable.
As I stated I could rename the variable, but that is not the point. The point is, that this is a false Positive.
Not sure why you don’t like having 3 conditions in the else if.
If coded as you have shown than sonar will suggest combining the else if with the if (S1066)
I don’t like it because it makes the code harder for me to understand at a quick glance; it takes more effort to read.
Sorry, I tried to help. Rooting for you. Good luck.
Hi Kevin,
Thank you for the interesting code snippet! One could argue that it is a true positive on a syntactic level (because the code is the same), but a false positive on a semantic level (because the same variable name refers to different things).
I am glad that the issue can be worked around easily by renaming variables.
I am not yet entirely sure how we want to approach this case, so I created a ticket (SONARJAVA-5216) for us to examine it more closely.