patternjavaMinor
Does setting the same element in a list does something at all?
Viewed 0 times
sametheallsettingelementdoeslistsomething
Problem
I have a code similar to this:
My question is: can I safely delete the line that sets the element that already is on the list on the same position?
Does this line does something that I don't know?
public class PlayerRound {
private final List playerStrips = new ArrayList<>();
public boolean addStrip(final Strip aStrip) {
// ...
if (playerStrips.contains(aStrip)) {
playerStrips.set(playerStrips.indexOf(aStrip), aStrip); // DOES THIS LINE CHANGE SOMETHING?
}
// ...
return true;
}
}@Entity
@Cacheable(false)
@Table(
appliesTo = "Strip",
indexes = {
@Index(name = "IDX_RoundStrip", columnNames = {"round_id"}),
@Index(name = "IDX_UserStrip", columnNames = {"user_id"})
}
)
public class Strip implements Serializable, IAnnotatedProxy {
@Id
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Integer id;
@Transient
private Integer tempId = tempIdx++;
@Override
public boolean equals(Object obj) {
Boolean areEqual = Boolean.FALSE;
if (obj != null && getClass() == obj.getClass()) {
final Strip other = (Strip) obj;
if (null == this.id && null == other.id) {
if (this.tempId.equals(other.tempId)) {
areEqual = Boolean.TRUE;
}
} else if (null != this.id) {
if (this.id.equals(other.id)
|| this.id.equals(other.tempId)) {
areEqual = Boolean.TRUE;
}
} else if (null != other.id) {
if (other.id.equals(this.id)
|| other.id.equals(this.tempId)) {
areEqual = Boolean.TRUE;
}
}
}
return areEqual;
}
}My question is: can I safely delete the line that sets the element that already is on the list on the same position?
Does this line does something that I don't know?
Solution
No, that code is totally useless. Unless....
Same element twice
As it is a List, what happens if it contains the element twice?
Let's say we have a List containing two elements of the type
stripA and stripB are equal, that is, the class have implemented
One inside, one outside, both
The same is true if the List contains
Yes.
indexOf also uses
So what happens here is that the index of
Small Improvement
There's really no reason to use both
However
If this functionality has been added because of this reason, I personally think it is a bad reason to add it. This code is more confusing than anything else. Do it differently. Either
Strip.equals
Holy.... mess!
First of, why use
Also, you could probably use
I have to question the usage of
By following the program execution, you can see that if one of those
Because of the above, there will be no reason to use any
Therefore, your
Same element twice
As it is a List, what happens if it contains the element twice?
Let's say we have a List containing two elements of the type
Strip:stripA, stripBstripA and stripB are equal, that is, the class have implemented
.equals. Then and only then, this code will do something useful. If aStrip is stripB then it will modify the list to become:stripB, stripBOne inside, one outside, both
.equalsThe same is true if the List contains
stripA only and you call the method with aStrip again as stripB. Remember that stripA.equals(stripB) is true so here's what happens:if (playerStrips.contains(aStrip)) {Yes.
.contains on a Collection uses .equals to see if it contains or not, so this will return true.playerStrips.indexOf(aStrip)indexOf also uses
.equals and the index of stripA is returned.playerStrips.set(playerStrips.indexOf(aStrip), aStrip);So what happens here is that the index of
stripA is set to stripB.Small Improvement
There's really no reason to use both
indexOf and contains. You could do this instead:int index = playerStrips.indexOf(aStrip);
if (index != -1) {
playerStrips.set(index, aStrip);
}However
If this functionality has been added because of this reason, I personally think it is a bad reason to add it. This code is more confusing than anything else. Do it differently. Either
aStrip should not really be equal to bStrip, or something else should be modified. The reason for why one would want to replace aStrip with bStrip when they are already equal goes beyond my understanding.Strip.equals
Holy.... mess!
First of, why use
Boolean when you can use boolean? Secondly, why use a boolean at all when you can use return?Also, you could probably use
instanceof instead of getClass. But you should look at the StackOverflow question about this.I have to question the usage of
tempID, it does not feel like a clean solution to me - whatever problem it is meant to solve.By following the program execution, you can see that if one of those
if (condition) return true is not true that the only possible return value is false. Therefore, it can be simplified with return condition;Because of the above, there will be no reason to use any
else as there's a return inside the previous if.Therefore, your
.equals method can be simplified to:@Override
public boolean equals(Object obj) {
if (obj instanceof Strip) {
final Strip other = (Strip) obj;
if (null == this.id && null == other.id) {
return this.tempId.equals(other.tempId);
}
if (null != this.id) {
return this.id.equals(other.id)
|| this.id.equals(other.tempId);
}
if (null != other.id) {
return other.id.equals(this.id)
|| other.id.equals(this.tempId);
}
}
return false;
}Code Snippets
stripA, stripBstripB, stripBif (playerStrips.contains(aStrip)) {playerStrips.indexOf(aStrip)playerStrips.set(playerStrips.indexOf(aStrip), aStrip);Context
StackExchange Code Review Q#55920, answer score: 5
Revisions (0)
No revisions yet.