Code-review и исправление ошибок

2. Провести code review и исправить класс Test

Условие задачи:
Необходимо провести code review класса Test, найти проблемы и предложить исправленную реализацию.

Нужно обратить внимание на:

потокобезопасность;
корректное закрытие файловых ресурсов;
выбор структуры данных;
эффективность формирования строки;
безопасное предоставление содержимого списка наружу.

Код:

public class Test {
    private List<String> list = new LinkedList<String>();

    public synchronized void addString(String s) {
        list.add(s);
    }

    public void addStringsFromFile(String pathToFile) throws IOException {
        File file = new File(pathToFile);
        FileInputStream fis = new FileInputStream(file);
        InputStreamReader isr =
                new InputStreamReader(fis, StandardCharsets.UTF_8);
        Scanner scanner = new Scanner(isr);

        String str;

        while (scanner.hasNextLine()) {
            str = scanner.nextLine();

            if (!str.isBlank()) {
                addString(str);
            }
        }
    }

    public void removeString(int i) {
        list.remove(i);
    }

    public List<String> getList() {
        return list;
    }

    public synchronized String formatString() {
        String s = "";

        for (int i = 0; i < list.size(); i++) {
            s = s + list.get(i);
        }

        return s;
    }
}

Спойлеры к решению

Подсказки

💡 Синхронизация должна защищать все операции с общим изменяемым состоянием, а не только часть методов.
💡 Возвращать внутренний изменяемый список напрямую нельзя — вызывающий код сможет менять его без синхронизации.
💡 Для чтения файла удобно использовать Files.newBufferedReader() вместе с try-with-resources.
💡 Последовательная конкатенация строк через + внутри цикла создаёт множество промежуточных объектов.
💡 Здесь ArrayList подходит лучше LinkedList, особенно если список часто обходится целиком.

Решение

Основные проблемы:

addString() и formatString() синхронизированы, а removeString() и getList() — нет;
getList() возвращает внутреннюю коллекцию, поэтому её можно изменить в обход любой синхронизации;
файловые ресурсы не закрываются;
используется слишком низкоуровневая цепочка FileInputStream → InputStreamReader → Scanner;
конкатенация через s = s + ... внутри цикла неэффективна;
LinkedList здесь не даёт преимуществ;
обращение list.get(i) для LinkedList само по себе стоит O(i), поэтому индексный обход дополнительно ухудшает производительность.

Один из вариантов рефакторинга:

public class Test {

    private final List<String> list = new ArrayList<>();
    private final Object lock = new Object();

    public void addString(String value) {
        synchronized (lock) {
            list.add(value);
        }
    }

    public void addStringsFromFile(String pathToFile)
            throws IOException {

        try (BufferedReader reader = Files.newBufferedReader(
                Path.of(pathToFile),
                StandardCharsets.UTF_8
        )) {
            String line;

            while ((line = reader.readLine()) != null) {
                if (!line.isBlank()) {
                    addString(line);
                }
            }
        }
    }

    public void removeString(int index) {
        synchronized (lock) {
            list.remove(index);
        }
    }

    public List<String> getList() {
        synchronized (lock) {
            return Collections.unmodifiableList(
                    new ArrayList<>(list)
            );
        }
    }

    public String formatString() {
        synchronized (lock) {
            StringBuilder result = new StringBuilder();

            for (String value : list) {
                result.append(value);
            }

            return result.toString();
        }
    }
}

Все обращения к изменяемому состоянию:

private final List<String> list

теперь выполняются под одним и тем же приватным монитором:

private final Object lock = new Object();

Использование отдельного объекта блокировки предпочтительнее синхронизации на самой коллекции: внешний код не получает к нему доступа и не может случайно вмешаться в протокол синхронизации.

В getList() возвращается снимок:

return Collections.unmodifiableList(
        new ArrayList<>(list)
);

Это важно. Вариант:

Collections.unmodifiableList(list)

запрещает вызывающему коду изменять список напрямую, но всё равно возвращает представление над исходной изменяемой коллекцией. Её содержимое может параллельно изменяться самим Test.

Снимок полностью отделён от внутреннего списка.

Для файла используется:

try (BufferedReader reader = Files.newBufferedReader(...))

try-with-resources гарантирует закрытие BufferedReader даже при возникновении IOException.

В formatString() вместо:

s = s + list.get(i);

используется:

StringBuilder result = new StringBuilder();

for (String value : list) {
    result.append(value);
}

Это устраняет создание множества промежуточных строк.

Также ArrayList лучше соответствует текущему сценарию использования: элементы добавляются в конец и список часто обходится целиком.

Для некорректного индекса:

list.remove(index);

естественным образом будет выброшен IndexOutOfBoundsException. Молча игнорировать неправильный индекс обычно не стоит, если такое поведение явно не требуется контрактом.

После рефакторинга:

доступ к общему состоянию синхронизирован единообразно;
внутренний список не утекает наружу;
ресурсы закрываются автоматически;
убрана лишняя низкоуровневая работа с потоками;
формирование строки выполняется эффективно;
коллекция выбрана в соответствии с характером операций.