Рефакторинг класса Cat4

10. Провести рефакторинг класса Cat4

Условие задачи:
Дан класс Cat4, в котором есть проблемы с компиляцией, инициализацией полей, многопоточностью и работой с JDBC.

Необходимо провести code review и исправить класс так, чтобы:

поля корректно инициализировались;
счётчик прыжков был потокобезопасным;
создание потоков не происходило через new Thread() для каждой операции;
работа с профилем была корректна при конкурентном доступе;
JDBC-ресурсы гарантированно закрывались;
SQL-запрос не был уязвим для SQL Injection;
результат запроса корректно обрабатывался.

Код:

public class Cat4 {
    private final ConcurrentHashMap<byte[], BigDecimal> CACHE =
            new ConcurrentHashMap<>();

    public int jumpsCount = 0;
    private var cat4Profile;

    private final List<Integer> list;
    private final DataSource dataSource;

    public Cat4(
            DataSource dataSource,
            List<? extends Integer> list
    ) {
        dataSource = this.dataSource;
        list = this.list;
    }

    public void doRandomJump(int maxJumps) {
        Random rnd = new Random();
        int jumpsToDo =
                Math.abs(rnd.nextInt()) % maxJumps;

        for (int i = 0; i < jumpsToDo; i++) {
            new Thread(() -> {
                doJump();
            }).start();
        }
    }

    public void setCat4Profile(Cat4Profile cat4Profile) {
        this.cat4Profile = cat4Profile;
    }

    public String getCat4Name() {
        try {
            return this.cat4Profile.getCatName();
        } catch (NullPointerException e) {
            return "Max";
        }
    }

    public void doJump() {
        this.jumpsCount++;

        Logger.getLogger(Cat4.class.getName())
                .fine("Jump!");
    }

    public void doMeow() {
        Logger.getLogger(Cat4.class.getName())
                .fine("Meow!");
    }

    public BigDecimal doQuery(byte[] parameters)
            throws SQLException {

        Connection conn = null;
        Statement stmt = null;

        try {
            conn = dataSource.getConnection();
            stmt = conn.createStatement();

            ResultSet resultSet = stmt.executeQuery(
                    "select weight from Cat where name = '"
                            + new String(parameters)
                            + "')"
            );

            resultSet.next();

            return resultSet.getBigDecimal("weight");
        } finally {
            if (stmt != null) {
                stmt.close();
            }

            if (conn != null) {
                conn.close();
            }
        }
    }

    public int getJumpsCount() {
        int result = jumpsCount;
        jumpsCount = 0;
        return result;
    }

    public void setJumpsCount() {
        this.jumpsCount++;
    }
}

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

Подсказки

💡 var нельзя использовать для объявления поля класса.
💡 В конструкторе присваивания сделаны в обратную сторону.
💡 Операция jumpsCount++ не атомарна.
💡 Получение значения счётчика и его сброс тоже должны быть одной атомарной операцией.
💡 Не создавай отдельный Thread на каждый прыжок — используй Executor.
💡 Math.abs(Integer.MIN_VALUE) остаётся отрицательным.
💡 Не следует ловить NullPointerException для обычной проверки на null.
💡 JDBC-запрос нужно выполнять через PreparedStatement и try-with-resources.
💡 Массив byte[] плохо подходит в качестве ключа HashMap, потому что массивы сравниваются по ссылке.

Решение

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

public class Cat4 {

    private static final Logger LOGGER =
            Logger.getLogger(Cat4.class.getName());

    private final AtomicInteger jumpsCount =
            new AtomicInteger();

    private volatile Cat4Profile cat4Profile;

    private final List<Integer> list;
    private final DataSource dataSource;
    private final Executor executor;

    public Cat4(
            DataSource dataSource,
            List<? extends Integer> list,
            Executor executor
    ) {
        this.dataSource =
                Objects.requireNonNull(dataSource);

        this.list = List.copyOf(
                Objects.requireNonNull(list)
        );

        this.executor =
                Objects.requireNonNull(executor);
    }

    public void doRandomJump(int maxJumps) {
        if (maxJumps <= 0) {
            throw new IllegalArgumentException(
                    "maxJumps должен быть больше 0"
            );
        }

        int jumpsToDo = ThreadLocalRandom.current()
                .nextInt(maxJumps);

        for (int i = 0; i < jumpsToDo; i++) {
            executor.execute(this::doJump);
        }
    }

    public void setCat4Profile(
            Cat4Profile cat4Profile
    ) {
        this.cat4Profile = cat4Profile;
    }

    public String getCat4Name() {
        Cat4Profile profile = cat4Profile;

        return profile == null
                ? "Max"
                : profile.getCatName();
    }

    public void doJump() {
        jumpsCount.incrementAndGet();
        LOGGER.fine("Jump!");
    }

    public void doMeow() {
        LOGGER.fine("Meow!");
    }

    public BigDecimal doQuery(byte[] parameters)
            throws SQLException {

        Objects.requireNonNull(
                parameters,
                "parameters"
        );

        String name = new String(
                parameters,
                StandardCharsets.UTF_8
        );

        String sql = """
                SELECT weight
                FROM Cat
                WHERE name = ?
                """;

        try (Connection connection =
                     dataSource.getConnection();
             PreparedStatement statement =
                     connection.prepareStatement(sql)) {

            statement.setString(1, name);

            try (ResultSet resultSet =
                         statement.executeQuery()) {

                if (!resultSet.next()) {
                    throw new SQLException(
                            "Cat not found: " + name
                    );
                }

                return resultSet.getBigDecimal(
                        "weight"
                );
            }
        }
    }

    public int getJumpsCount() {
        return jumpsCount.getAndSet(0);
    }

    public void incrementJumpsCount() {
        jumpsCount.incrementAndGet();
    }
}

В конструкторе исходные присваивания были перепутаны:

dataSource = this.dataSource;
list = this.list;

Должно быть наоборот:

this.dataSource = dataSource;
this.list = ...;

Кроме того, поскольку список передаётся извне, создаётся собственная неизменяемая копия:

this.list = List.copyOf(list);

Это не позволяет вызывающему коду неожиданно изменить внутреннее состояние Cat4.

Поле:

private var cat4Profile;

не компилируется.

var разрешён для локальных переменных, но не для полей класса.

Нужен явный тип:

private volatile Cat4Profile cat4Profile;

volatile обеспечивает видимость нового значения между потоками.

В getCat4Name() нет необходимости использовать исключение для обычного управления логикой:

try {
    return cat4Profile.getCatName();
} catch (NullPointerException e) {
    return "Max";
}

Корректнее явно проверить значение:

Cat4Profile profile = cat4Profile;

return profile == null
        ? "Max"
        : profile.getCatName();

Локальная переменная также гарантирует, что в рамках метода используется одно прочитанное значение ссылки.

Счётчик:

jumpsCount++;

не является атомарной операцией.

Фактически выполняются три действия:

прочитать значение
↓
увеличить
↓
записать новое значение

Два потока могут прочитать одно значение одновременно, и одно увеличение потеряется.

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

private final AtomicInteger jumpsCount =
        new AtomicInteger();

и:

jumpsCount.incrementAndGet();

Особенно важно исправить:

int result = jumpsCount;
jumpsCount = 0;

Здесь между чтением и обнулением другой поток мог выполнить doJump(), и его увеличение потерялось.

AtomicInteger позволяет выполнить обе операции атомарно:

return jumpsCount.getAndSet(0);

Создавать поток для каждого прыжка:

new Thread(this::doJump).start();

плохо.

При большом количестве вызовов приложение может создать огромное количество нативных потоков и исчерпать ресурсы.

Вместо этого используется общий:

Executor

а задача передаётся ему:

executor.execute(this::doJump);

Размер пула потоков и очереди можно контролировать снаружи.

Конструкция:

Math.abs(rnd.nextInt()) % maxJumps

имеет две проблемы.

Во-первых:

Math.abs(Integer.MIN_VALUE)

по-прежнему возвращает отрицательное число из-за переполнения int.

Во-вторых, при:

maxJumps = 0

возникнет ArithmeticException.

Корректнее использовать готовый ограниченный генератор:

ThreadLocalRandom.current()
        .nextInt(maxJumps);

Он возвращает значение:

0 <= jumpsToDo < maxJumps

что сохраняет семантику исходного % maxJumps.

В JDBC есть сразу несколько проблем.

Конкатенация:

"WHERE name = '" + name + "'"

позволяет внедрить произвольный SQL через параметр.

Поэтому нужен PreparedStatement:

String sql = """
        SELECT weight
        FROM Cat
        WHERE name = ?
        """;

PreparedStatement statement =
        connection.prepareStatement(sql);

statement.setString(1, name);

Кроме того, в исходной строке запроса присутствовала лишняя закрывающая скобка.

Все JDBC-ресурсы закрываются через try-with-resources:

Connection
PreparedStatement
ResultSet

даже если во время выполнения запроса возникает исключение.

Также нельзя без проверки делать:

resultSet.next();
return resultSet.getBigDecimal("weight");

Если запрос ничего не вернул, next() даст false.

Поэтому результат проверяется:

if (!resultSet.next()) {
    throw new SQLException(
            "Cat not found: " + name
    );
}

Для преобразования byte[] в строку явно задаётся кодировка:

new String(
        parameters,
        StandardCharsets.UTF_8
);

а не используется platform default charset.

Отдельная проблема:

ConcurrentHashMap<byte[], BigDecimal>

Даже несмотря на потокобезопасность ConcurrentHashMap, byte[] — плохой ключ.

Например:

byte[] first = {1, 2};
byte[] second = {1, 2};

System.out.println(
        first.equals(second)
); // false

Массивы используют equals() и hashCode() из Object, поэтому два массива с одинаковым содержимым считаются разными ключами.

Кроме того, сам массив изменяемый.

В данном коде CACHE вообще нигде не используется, поэтому его лучше удалить как мёртвое состояние.

Если такой кэш действительно нужен, следует использовать неизменяемый ключ с корректными equals() и hashCode(), например отдельный value object, а не сырой byte[].

Основные проблемы исходного класса:

поле с var не компилируется;
поля конструктора инициализируются в неправильную сторону;
внутренний список напрямую связан с внешней коллекцией;
jumpsCount публичный и непотокобезопасный;
jumpsCount++ приводит к lost update;
чтение и сброс счётчика неатомарны;
создаётся отдельный поток на каждый прыжок;
генерация случайного числа содержит edge cases;
NullPointerException используется как часть обычной логики;
изменение cat4Profile между потоками не обеспечивает видимость;
Logger каждый раз запрашивается заново;
SQL собирается конкатенацией;
возможен SQL Injection;
SQL содержит синтаксическую ошибку;
JDBC-ресурсы управляются вручную;
ResultSet.next() не проверяется;
при преобразовании byte[] не задан charset;
byte[] некорректен как обычный ключ Map;
CACHE и list в представленном коде фактически не используются;
setJumpsCount() имеет неверное имя — метод увеличивает счётчик, поэтому incrementJumpsCount() точнее отражает его назначение.