HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Does setting the same element in a list does something at all?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
sametheallsettingelementdoeslistsomething

Problem

I have a code similar to this:

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 Strip:

stripA, stripB


stripA 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, stripB


One inside, one outside, both .equals

The 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, stripB
stripB, stripB
if (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.