[JAVA] Exposing mutable internals

programming

ACME corporation needs to implement a new platform to manage its list of employees. The software will have to:

  1. every 3 seconds, send the updated list of employees to the HR
  2. every 1 second update, the employee registry with the updates received
  3. Every 0.5 seconds, create a list of alphabetically ordered surnames and print it.

Wile E. Coyote is in charge of such development. After a few minutes he implements 3 classes:

  • Employee: this class will contain the details of each employee
  • EmployeeRegistry: this class will manage the list of employees
  • Main: this class will send the list of employees to HR and will receive the employee list updates.

Wile: "Let's make one step at a time. First step: send the updated list every second. Initially I will simulate it by just dumping it on the screen".

After some time, Wile E. comes out with the following code

////// Employee.java
public class Employee {
    private final String name;
    private final String surname;

    public Employee(String name, String surname) {
        this.name = name;
        this.surname = surname;
    }

    public String getName() {
        return name;
    }

    public String getSurname() {
        return surname;
    }

    @Override
    public String toString() {
        return this.name + " " + this.surname;
    }
}

////// EmployeeRegistry.java
public class EmployeeRegistry {
    private final List<Employee> employeeList;

    public EmployeeRegistry() {
        this.employeeList = new ArrayList<>();
    }

    public EmployeeRegistry(List<Employee> employeeList) {
        this.employeeList = employeeList;
    }

    public List<Employee> getContactList() {
        return employeeList;
    }

    public void addEmployee(Employee e) {
        synchronized (employeeList) {
            employeeList.add(e);
        }
    }

    public void sendToHR() {
        synchronized (employeeList) {
            employeeList.forEach(c -> System.out.println("sendToHR" + " " + c));
        }
    }
}

////// Main.java
public class Main {
    public static void main(String[] args) {
        List<Employee> employees = new ArrayList<>(Arrays.asList(new Employee("Wile E.", "Coyote"),
                new Employee("Beep.", "Beep")));

        EmployeeRegistry employeeRegistry = new EmployeeRegistry(employees);
        sendToHR(employeeRegistry);
    }

    private static void sendToHR(EmployeeRegistry employeeRegistry) {
        ScheduledExecutorService exec = Executors.newScheduledThreadPool(10);
        exec.scheduleAtFixedRate(employeeRegistry::sendToHR, 0, 3, TimeUnit.SECONDS);
    }
}

Wile runs the code and gets the following output:

sendToHR Wile E. Coyote
sendToHR Beep. Beep
sendToHR Wile E. Coyote
sendToHR Beep. Beep
sendToHR Wile E. Coyote
sendToHR Beep. Beep

Wile: "Too easy! Works like a charm! Let's add the code to receive the updates now..."
Wile changes the code as follows

////// Employee.java - no changes
////// EmployeeRegistry.java - no changes

public class Main {
    public static void main(String[] args) {
        List<Employee> employees = new ArrayList<>(Arrays.asList(new Employee("Wile E.", "Coyote"),
                new Employee("Beep.", "Beep")));

        EmployeeRegistry employeeRegistry = new EmployeeRegistry(employees);
        sendToHR(employeeRegistry);
        update(employeeRegistry);
    }

    private static void sendToHR(EmployeeRegistry employeeRegistry) {
        // ... no changes
    }

    private static void update(EmployeeRegistry employeeRegistry) {
        AtomicInteger index = new AtomicInteger();
        ScheduledExecutorService exec = Executors.newScheduledThreadPool(10);
        exec.scheduleAtFixedRate(() -> {
            int idx = index.getAndIncrement();
            Employee e = new Employee("NAME" + idx, "SURNAME" + idx);
            System.out.println("update: " + e);
            employeeRegistry.addEmployee(e);
        }, 0, 1, TimeUnit.SECONDS);
    }
}

Wile: "That's been easy! Let's test it now!"

sendToHR Wile E. Coyote
sendToHR Beep. Beep
update: NAME0 SURNAME0
update: NAME1 SURNAME1
update: NAME2 SURNAME2
sendToHR Wile E. Coyote
sendToHR Beep. Beep
sendToHR NAME0 SURNAME0
sendToHR NAME1 SURNAME1
sendToHR NAME2 SURNAME2

Wile: "No fun! That's too easy hahaha! Let's dump the list of surnames now!"
The new code looks like this:

////// Employee.java - no changes
////// EmployeeRegistry.java - no changes

public class Main {
    public static void main(String[] args) {
        List<Employee> employees = new ArrayList<>(Arrays.asList(new Employee("Wile E.", "Coyote"),
                new Employee("Beep.", "Beep")));

        EmployeeRegistry employeeRegistry = new EmployeeRegistry(employees);
        sendToHR(employeeRegistry);
        update(employeeRegistry);
        printSurnames(employeeRegistry);
    }

    private static void sendToHR(EmployeeRegistry employeeRegistry) {
        // ... no changes
    }

    private static void update(EmployeeRegistry employeeRegistry) {
        // ... no changes
    }

    private static void printSurnames(EmployeeRegistry employeeRegistry) {
        ScheduledExecutorService exec = Executors.newScheduledThreadPool(10);
        exec.scheduleAtFixedRate(() -> {
            System.out.println("Printing Surnames");
            List<Employee> employeeList = employeeRegistry.getContactList();
            employeeList.sort(Comparator.comparing(Employee::getSurname));
            employeeList.stream().map(Employee::getSurname).forEach(surname -> System.out.println("printSurname: " + surname));
        }, 0, 500, TimeUnit.MILLISECONDS);
    }

}

Wile runs the code and gets this output:

Printing Surnames
printSurname: Beep
printSurname: Coyote
sendToHR Wile E. Coyote
update: NAME0 SURNAME0
Printing Surnames
printSurname: Beep
printSurname: Coyote
printSurname: SURNAME0
update: NAME1 SURNAME1
Printing Surnames
printSurname: Beep
printSurname: Coyote
printSurname: SURNAME0
printSurname: SURNAME1
Printing Surnames
printSurname: Beep
... (cut)

Wile thinks loud: "Hmmm... weird... something is not working as expected. The sendToHR ran only one time."
Beep Beep is passing nearby and hears Wile, look at the code and says: "I know where the issue is!"
Wile: "Aaargh... I hate this. Show me, please. What do you think the issue is?"

Beep Beep: "First: when you schedule something with the scheduler service, always surround your code with a try...catch to avoid it being silently killed in case of error..."

Beep Beep changes the code adding the exception handling:

    private static void update(EmployeeRegistry employeeRegistry) {
        AtomicInteger index = new AtomicInteger();
        ScheduledExecutorService exec = Executors.newScheduledThreadPool(10);
        exec.scheduleAtFixedRate(() -> {
            try {
                int idx = index.getAndIncrement();
                Employee e = new Employee("NAME" + idx, "SURNAME" + idx);
                System.out.println("update: " + e);
                employeeRegistry.addEmployee(e);
            } catch (Throwable t) {
                t.printStackTrace();
            }
        }, 0, 1, TimeUnit.SECONDS);
    }

    private static void printSurnames(EmployeeRegistry employeeRegistry) {
        ScheduledExecutorService exec = Executors.newScheduledThreadPool(10);
        exec.scheduleAtFixedRate(() -> {
            try {
                System.out.println("Printing Surnames");
                List<Employee> employeeList = employeeRegistry.getContactList();
                employeeList.sort(Comparator.comparing(Employee::getSurname));
                employeeList.stream().map(Employee::getSurname).forEach(surname -> System.out.println("printSurname: " + surname));
            } catch (Throwable t) {
                t.printStackTrace();
            }
        }, 0, 500, TimeUnit.MILLISECONDS);
    }

    private static void sendToHR(EmployeeRegistry employeeRegistry) {
            ScheduledExecutorService exec = Executors.newScheduledThreadPool(10);
            exec.scheduleAtFixedRate(() -> {
                try {
                    employeeRegistry.sendToHR();
                } catch (Throwable t) {
                    t.printStackTrace();
                }
            }, 0, 3, TimeUnit.SECONDS);
    }

He runs the code and...

Printing Surnames
printSurname: Beep
printSurname: Coyote
sendToHR Wile E. Coyote
update: NAME0 SURNAME0
java.util.ConcurrentModificationException
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1513)
    at EmployeeRegistry.sendToHR(EmployeeRegistry.java:27)
    at Main.lambda$sendToHR$3(Main.java:51)
    at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539)
    at java.base/java.util.concurrent.FutureTask.runAndReset(FutureTask.java:305)
    at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:305)
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
    at java.base/java.lang.Thread.run(Thread.java:833)
Printing Surnames
printSurname: Beep
printSurname: Coyote

Beep Beep: "Voilà! The error has surfaced!"
Wile: "How can it be? The sendToHR is synchronised on employeeList!"
Beep Beep: "You are right, but look at the getContactList method"
Wile: "What's wrong with that?"
Beep Beep: "You are returning a reference to the internal list. You are exposing a mutable internal, and that is a bad practice because you have no control of what the API client will do with that reference. If some modification is done you can get the concurrent modification exception"
Wile: "Oh my! How can I solve it?"
Beep Beep: "You are lucky. That's very easy: return a copy." 

////// Employee.java - no changes
////// EmployeeRegistry.java
public class EmployeeRegistry {
    public class EmployeeRegistry {
    private final List<Employee> employeeList;

    public EmployeeRegistry() {
        this.employeeList = new ArrayList<>();
    }

    public EmployeeRegistry(List<Employee> employeeList) {
        this.employeeList = employeeList;
    }

    public List<Employee> getContactList() {
        synchronized (employeeList) {
            return new ArrayList<>(employeeList); // <- Return a copy of the current employee list
        }
    }

    public void addEmployee(Employee e) {
        synchronized (employeeList) {
            employeeList.add(e);
        }
    }

    public void sendToHR() {
        synchronized (employeeList) {
            employeeList.forEach(c -> System.out.println("sendToHR" + " " + c));
        }
    }
}

}
////// Main.java - no changes

Beep Beep: "Let's try to run the code now!"

Printing Surnames
update: NAME0 SURNAME0
sendToHR Wile E. Coyote
sendToHR Beep. Beep
printSurname: Beep
printSurname: Coyote
printSurname: SURNAME0
Printing Surnames
printSurname: Beep
printSurname: Coyote
printSurname: SURNAME0
update: NAME1 SURNAME1
Printing Surnames
printSurname: Beep
printSurname: Coyote
printSurname: SURNAME0
printSurname: SURNAME1

Wile: "Great! Now it works!! But can't we just return an unmodifiable collection?"
Beep Beep: "I will let the code answer you. Let's change it to use an unmodifiable collection"

They work for some minutes and produce the following code:

////// Employee.java - no changes
////// EmployeeRegistry.java
public class EmployeeRegistry {
    /// ... cut
    public List<Employee> getContactList() {
        return Collections.unmodifiableList(employeeList);
    }
////// Main.java
public class Main {
    /// ... cut
    private static void printSurnames(EmployeeRegistry employeeRegistry) {
        ScheduledExecutorService exec = Executors.newScheduledThreadPool(10);
        exec.scheduleAtFixedRate(() -> {
            try {
                System.out.println("Printing Surnames");
                employeeRegistry
                        .getContactList()
                        .stream()
                        .sorted(Comparator.comparing(Employee::getSurname))
                        .forEach(surname -> System.out.println("printSurname: " + surname));
            } catch (Throwable t) {
                t.printStackTrace();
            }
        }, 0, 500, TimeUnit.MILLISECONDS);
    }
}

=== OUTPUT ===
---8x---cut many rows
sendToHR NAME6 SURNAME6
sendToHR NAME7 SURNAME7
sendToHR NAME8 SURNAME8
Printing Surnames
update: NAME9 SURNAME9
java.util.ConcurrentModificationException
    at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1631)
    at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509)
    at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
    at java.base/java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150)
    at java.base/java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173)
    at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
    at java.base/java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596)
    at Main.lambda$printSurnames$2(Main.java:42)
    at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539)
    at java.base/java.util.concurrent.FutureTask.runAndReset(FutureTask.java:305)
    at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:305)
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
    at java.base/java.lang.Thread.run(Thread.java:833)
Printing Surnames
printSurname: Beep. Beep
printSurname: Wile E. Coyote

Beep Beep: "It is always safer to return a copy of the internal state. Even returning a copy, there is still one issue with your code. Can you see it?"
Wile: "Yes. The EmployeeRegistry constructor should copy the received List instead of saving its reference"
Beep Beep: "Good boy!"

Conclusion

Each object should be responsible for its internal state. Returning a reference to a mutable internal makes the object unaware of changes to its state and is likely to lead to unexpected, hard-to-identify issues. If you can, always prefer returning a copy. If you can't, return an unmodifiable collection.

The same applies to receiving mutable objects: if you can, copy them and store a reference to the copy.

Whole code:

Employee.java

public class Employee {
    private final String name;
    private final String surname;

    public Employee(String name, String surname) {
        this.name = name;
        this.surname = surname;
    }

    public String getName() {
        return name;
    }

    public String getSurname() {
        return surname;
    }

    @Override
    public String toString() {
        return this.name + " " + this.surname;
    }
}

EmployeeRegistry.java

import java.util.ArrayList;
import java.util.List;

public class EmployeeRegistry {
    private final List<Employee> employeeList;

    public EmployeeRegistry() {
        this.employeeList = new ArrayList<>();
    }

    public EmployeeRegistry(List<Employee> employeeList) {
        this.employeeList = new ArrayList<>(employeeList);
    }

    public List<Employee> getContactList() {
        synchronized (employeeList) {
            return new ArrayList<>(employeeList);
        }
    }

    public void addEmployee(Employee e) {
        synchronized (employeeList) {
            employeeList.add(e);
        }
    }

    public void sendToHR() {
        synchronized (employeeList) {
            employeeList.forEach(c -> System.out.println("sendToHR" + " " + c));
        }
    }
}

Main.java

import java.util.*;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;

public class Main {
    public static void main(String[] args) {
        List<Employee> employees = new ArrayList<>(Arrays.asList(new Employee("Wile E.", "Coyote"),
                new Employee("Beep.", "Beep")));

        EmployeeRegistry employeeRegistry = new EmployeeRegistry(employees);
        sendToHR(employeeRegistry);
        update(employeeRegistry);
        printSurnames(employeeRegistry);
    }

    private static void update(EmployeeRegistry employeeRegistry) {
        AtomicInteger index = new AtomicInteger();
        ScheduledExecutorService exec = Executors.newScheduledThreadPool(10);
        exec.scheduleAtFixedRate(() -> {
            try {
                int idx = index.getAndIncrement();
                Employee e = new Employee("NAME" + idx, "SURNAME" + idx);
                System.out.println("update: " + e);
                employeeRegistry.addEmployee(e);
            } catch (Throwable t) {
                t.printStackTrace();
            }
        }, 0, 1, TimeUnit.SECONDS);
    }

    private static void printSurnames(EmployeeRegistry employeeRegistry) {
        ScheduledExecutorService exec = Executors.newScheduledThreadPool(10);
        exec.scheduleAtFixedRate(() -> {
            try {
                System.out.println("Printing Surnames");
                employeeRegistry
                        .getContactList()
                        .stream()
                        .sorted(Comparator.comparing(Employee::getSurname))
                        .forEach(surname -> System.out.println("printSurname: " + surname));
            } catch (Throwable t) {
                t.printStackTrace();
            }
        }, 0, 500, TimeUnit.MILLISECONDS);
    }

    private static void sendToHR(EmployeeRegistry employeeRegistry) {
            ScheduledExecutorService exec = Executors.newScheduledThreadPool(10);
            exec.scheduleAtFixedRate(() -> {
                try {
                    employeeRegistry.sendToHR();
                } catch (Throwable t) {
                    t.printStackTrace();
                }
            }, 0, 3, TimeUnit.SECONDS);
    }
}
Previous Next