Why assign a new list to a list reference shows "Never used"

Question

I want to move some special object in the list to the bottom of it. I tried to used below code to implement it but it shows last assignment is never been used... I have walkaround but I really want to know why my code is wrong? After the method proceed the list I passed in, it still the old sequence. No impact...

private void moveNullBrokerToEnd(List<Broker> brokers) {
if (brokers != null) {
  List<Broker> nullTypeBrokers = new ArrayList<>();
  List<Broker> commonBrokers = new ArrayList<>();
  for (Broker broker : brokers) {
    if (broker.getMemberType()==null) {
      nullTypeBrokers.add(broker);
    } else {
      commonBrokers.add(broker);
    }
  }
  brokers = ListUtils.union(commonBrokers, nullTypeBrokers);
}

}

Would it be simpler to sort the list, putting null values as first? — Beri, Oct 24 '22 at 08:26
For sure, especially because `List.sort` would change the actual `brokers` list instead of creating a copy only inside the method, like this implementation does. — cyberbrain, Oct 24 '22 at 08:31

score 0 · Answer 1 · answered Oct 24 '22 at 08:28

0

Parameters of methods are passed by-value (that means, the reference to objects are passed by value). So when you modify the reference in the brokers parameter (the object it is pointing to) this change only has impact inside the method.

The caller of the method will not notice any change.

And in your method, you don't use the brokers parameter after the re-assignment and so you don't use the actual result of the ListUtils.union call.

answered Oct 24 '22 at 08:28

cyberbrain

3,433
1
12
22

thanks cyberbrain, I thought the method parameter type is ArrayList, so I think it is by-reference. – Jason Oct 24 '22 at 08:45

Beri · Accepted Answer · 2022-10-24T09:12:15.450

You are assigning new Object to brokers reference. brokers is a reference to this list Object, but when you assign new object to it, it's not visible outside of the function. You could say that it's pointing to a new object from now on.

You need to return brokers as new list (recommended) or clear the list then add all values from union.

Example:

brokers.clear();
brokers.addAll(ListUtils.union(commonBrokers, nullTypeBrokers))

But this has many drawbacks:

violates conventions for clean code
will not work for null reference.
You might change original list - mutating is wrong

You can use sort function here instead:

@Test
public void setNullsFirst() {

    List<Broker> brokers = List.of(
            new Broker("1."),
            new Broker(null),
            new Broker("2."),
            new Broker(null)
    );

    System.out.println(moveNullBrokerToEnd(brokers));

}
private List<Broker> moveNullBrokerToEnd(List<Broker> brokers) {
    return Optional.ofNullable(brokers).orElseGet(Collections::emptyList)
            .stream()
            .sorted(Comparator.comparing(broker->broker.memberType==null?-1:0))
            .toList();
}

The results are:

[Broker(memberType=null), Broker(memberType=null), Broker(memberType=1.), Broker(memberType=2)]

Why assign a new list to a list reference shows "Never used"

2 Answers2