Sonar - Store a copy - Mutable members should not be stored or returned directly

user_8597968 0
Viewed 17062

I have a list which is a private member in my class. I have used getter and setter to get and set the values. SOnar throws an error - Mutable members should not be stored or returned directly.

For example: ABC and DEF are two classes.

class ABC{
private List<DEF> defList;
public List<DEF> getDefList() { return defList; }
public void setDefList(List<DEF> defList) { this.defList = defList; }

After lot of googling and searching, I have understood that the getter can be changed as follows:

public List<DEF> getDefList() { return new ArrayList<>(defList); }

When i try to use setter similarly, 

public void setDefList(List<DEF> defList) { this.defList.addAll(defList); }

then the variable starts showing 

'private field 'defList' is never assigned.

May I know the correct way to do when it is a list, (a list of another class)

Note: Both the answers from Prasad Karunagoda and Leo Aso works. I cant mark both as accepted answer. So having a note here

sonarqube mutable arraylist java
2 Answers

The warning is because you did not give the field an initial value. This is how you should implement the code to ensure immutability using java.util.Collections.

class ABC {
    private List<DEF> defList = Collections.emptyList();

    public List<DEF> getDefList() { 
        return defList;
    }

    public void setDefList(List<DEF> defList) {
        // defensively copy, then make immutable
        defList = new ArrayList<>(defList);
        this.defList = Collections.unmodifiableList(defList);
    }
user_14264690

I believe it is better not to add additional restrictions (immutability) to the List returned from the getter. If you do that, clients using your List will not be able sort it for example.

So, my recommended approach is this:

public class ABC {
  private List<DEF> defList = new ArrayList<>();

  public List<DEF> getDefList() {
    return new ArrayList<>(defList);
  }

  public void setDefList(List<DEF> defList) {
    if (defList == null)
        throw new IllegalArgumentException("Parameter defList is null");
    this.defList.clear();
    this.defList.addAll(defList);
  }
}

From design perspective, an even better API for ABC class would be:

public List<DEF> getDefList()
public void clearDefList()
public void addAllDefs(List<DEF> defs) // Or method name appendDefs
user_14166940
Related