Spring Tool Suite 4 - Version: 4.11.0.RELEASE - Build Id: 202106180608
SonarLint for Eclipse 5.9.0.31414 org.sonarlint.eclipse.feature.feature.group SonarSource
Here we have simple code to add an item to the queue. Item is passed to some methods which make some changes and return the item ready for save to the repository. But the assignment inside “else” is called redundand despit it returns different object than any ohter assignment.
/**
* Assignments should not be redundant (java:S4165)
*/
void processItem( final MyEntity incomingEntity ) {
MyEntity targetEntity =
this.myEntityRepository.findById( incomingEntity.getId() );
if (targetEntity == null) {
targetEntity = addToQueueNewItem( incomingEntity );
}
else {
// HINT: Remove this useless assignment; "targetEntity" already
// holds the assigned value along all execution paths.
targetEntity = addToQueueExistingItem( targetEntity, incomingEntity );
}
targetEntity = this.myEntityRepository.save( targetEntity );
// ... continue using Hibernate managed version of entity 'targetEntity'
}
MyEntity addToQueueNewItem(
final MyEntity incomingEntity
) {
incomingEntity.setDateAdded( new Date() );
incomingEntity.setCreateReason( "some reason" );
return incomingEntity;
}
MyEntity addToQueueExistingItem(
final MyEntity existingEntity,
final MyEntity incomingEntity
) {
existingEntity.setDateLastUpdate( new Date() );
existingEntity.setOldUpdateReason( existingEntity.getUpdateReason() );
existingEntity.setUpdateReason( incomingEntity.getUpdateReason() );
return entity;
}
Can you confirm to me that there is a Typo in the returned expression of your method MyEntity addToQueueExistingItem?
I would expect it to return existingEntity instead of entity (not declared anywhere in your code snippet)
MyEntity addToQueueExistingItem(
final MyEntity existingEntity,
final MyEntity incomingEntity
) {
existingEntity.setDateLastUpdate( new Date() );
existingEntity.setOldUpdateReason( existingEntity.getUpdateReason() );
existingEntity.setUpdateReason( incomingEntity.getUpdateReason() );
return entity; // HERE I would expect to return "existingEntity"
}
If it’s the case, then it looks to me that it’s not a False-Positive, but a rather counter-intuitive True-Positive. I tried to simplify your case to show what is happening, keeping only what is meaningful for our issue.
Can you double-check the following simplification to see if it corresponds to what is happening in your case, following steps 1) to 7) ?
public abstract class A {
@javax.annotation.CheckForNull
abstract B findB(A a);
abstract B newBFromA(A a);
static class B {
void doSomething() {}
void doSomethingWithA(A a) {}
}
interface C {
B findBFromA(A a);
B save(B b);
}
private C c;
void processItem(A a) {
B b = this.c.findBFromA(a); // 1) findBFromA returns a value (SV_1), assigned to b
if (b == null) { // 2) we assume SV_1 is not null, and go directly to the ELSE branch
b = newBFromA(a);
} else {
b = addBToA(/* 3) enter method with SV_1 */ b, a); // 7) method returns an object, but it is also its first argument: b == SV_1
// and we reassign it to b, which is ALREADY SV_1
}
b = this.c.save(b);
// continuing code
b.doSomething();
}
private B addBToA(B b, A a) {
b.doSomething(); // 4) b his modified, but still points to symbolic value SV_1
b.doSomethingWithA(a); // 5) b his modified, but still points to symbolic value SV_1
return b; // 6) return b, which is still SV_1
}
}
Sorry for delay - missed the notification abut your answer.
Yes, I did a typo as I had to rename and filter out the rest of the code - I’m not allowed to share original code as it’s not mine.
You did it right, the main idea is to try find in repository, if not found use existing item, but if the item is found, then update it with only few properties from incoming item.
Now that you pointed out - yes, that’s my bad, especially because I have “final” modifiers on both arguments in “addToQueueExistingItem()” signature.
Should I delete myself or this ticket?
