I have an immutable class called Employees like this:
public final class Employees {
private final List<Person> persons;
public Employees() {
persons = new LinkedList<Person>();
}
public List<Person> getPersons() {
return persons;
}
}
How do I keep this class immutable?
I made the field private and final, and I didn't provide a setter method. Is this enough to achieve immutability?
Answer edited to explain not only the case with a mutable version of Person, but also with an immutable version of Person.
Your class is mutable because you can do that:
Employees employees = new Employees();
employees.getPersons().add(new Person());
Note that not passing a list of persons to the constructor if you change your code to create an immutable class you will have a not useful class holding an ever empty list of persons, so I assume that is necessary to pass a List<Person> to the constructor.
Now there are two scenarios, with different implementations:
Person is immutablePerson is mutableScenario 1 - Person is immutable
You need only to create an immutable copy of the persons parameter in the constructor.
You need also to make final the class or at least the method getPersons to be sure that nobody provide a mutable overwritten version of the getPersons method.
public final class Employees {
private final List<Person> persons;
public Employees(List<Person> persons) {
persons = Collections.unmodifiableList(new ArrayList<>(persons));
}
public List<Person> getPersons() {
return persons;
}
}
Scenario 2 - Person is mutable
You need to create a deep copy of persons in the getPersons method.
You need to create a deep copy of persons on the constructor.
You need also to make final the class or at least the method getPersons to be sure that nobody provide a mutable overwritten version of the getPersons method.
public final class Employees {
private final List<Person> persons;
public Employees(List<Person> persons) {
persons = new ArrayList<>();
for (Person person : persons) {
persons.add(deepCopy(person)); // If clone is provided
// and creates a deep copy of person
}
}
public List<Person> getPersons() {
List<Person> temp = new ArrayList<>();
for (Person person : persons) {
temp.add(deepCopy(person)); // If clone is provided
// and creates a deep copy of person
}
return temp;
}
public Person deepCopy(Person person) {
Person copy = new Person();
// Provide a deep copy of person
...
return copy;
}
}
This part of the answer is to show why a not deep copy of the personsParameter passed to the constructor can create mutable versions of Employees:
List<Person> personsParameter = new ArrayList<>();
Person person = new Person();
person.setName("Pippo");
personsParameter.add(person);
Employees employees = new Employees(personsParameter);
// Prints Pippo
System.out.println(employees.getPersons().get(0).getName());
employees.getPersons().get(0).setName("newName");
// Prints again Pippo
System.out.println(employees.getPersons().get(0).getName());
// But modifiyng something reachable from the parameter
// used in the constructor
person.setName("Pluto");
// Now it prints Pluto, so employees has changed
System.out.println(employees.getPersons().get(0).getName());
No, it is not enough because in java even references are passed by value. So, if your List's reference escapes ( which will happen, when they call get), then your class is no longer immutable.
You have 2 choices :
List and return it when get is called.List and return it (or replace your original List with this, then you can return this safely without further wrapping it)Note : You will have to ensure that Person is either immutable or create defensive copies for each Person in the List
The answer is found in the docs - A Strategy for Defining Immutable Objects:
Don't provide "setter" methods — methods that modify fields or objects referred to by fields.
Make all fields final and private.
Don't allow subclasses to override methods.
If the instance fields include references to mutable objects, don't allow those objects to be changed:
4.1. Don't provide methods that modify the mutable objects.
4.2. Don't share references to the mutable objects. Never store references to external, mutable objects passed to the constructor; if necessary, create copies, and store references to the copies. Similarly, create copies of your internal mutable objects when necessary to avoid returning the originals in your methods.
You could do even better with
import java.util.Collections;
...
public List<Person> getPersons() {
return Collections.unmodifiableList( persons);
}
In this case I would favour not exposing the List or Map at all. If you pass out the collection are you breaking encapsulation by assuming it must be stored as a List.
If you don't expose the List, you can avoid needing to wrap it or provide a defensive copy.
public final class Employees {
private final List<Person> persons;
public Employees() {
persons = new LinkedList<Person>();
}
public Person getPerson(int n) {
return persons.get(n);
}
public int getPersonCount() {
return persons.size();
}
}
The reason you want to avoid defensive copies is they can be called a lot. Someone could easily write (you would be surprised how often this is written)
for (int i = 0; i < employees.getPersons().size(); i++) {
Person p = employees.getPersons().get(i);
}
By giving specific methods you can optimise the code for that use case.
I think the problem with that approach is that I still can do getPersons().add(x). For not allowing that, you have to create a constructor that receives the list of persons and then instantiate your internal final list as, for example, ImmutableList. The rest of the code seems alright for me.
EDIT:
as Davide Lorenzo MARINO points, this is not enough. You also would need to return a deep copy of the list in the getter, so users can't modify your internal list by the getter returned reference.
By doing this you can just have a normal LinkedList as your itnernal List (private final). On your constructor you create a deep copy of the List and on your getter you return a deep copy of your internal List.
