Провести ревью класса Transaction

38. Провести ревью класса Transaction

Условие задачи:

Дан класс Transaction.

Необходимо провести ревью кода, найти потенциальные проблемы и предложить исправленный вариант.

Код:

static public final class Transaction<T extends Number>
        implements Comparable<Transaction> {

    private final T id;
    private final Double amount;
    private final java.util.Date timestamp;

    public Transaction(T id, double amount, java.util.Date timestamp) {
        this.id = id;
        this.amount = amount;
        this.timestamp = timestamp;
    }

    public T getId() {
        return id;
    }

    public Double getAmount() {
        return amount;
    }

    public java.util.Date getTimestamp() {
        return timestamp;
    }

    @Override
    public boolean equals(Object o) {
        if (this == o) return true;
        if (!(o instanceof Transaction t)) return false;

        return id == t.id
                && Double.compare(amount, t.amount) == 0
                && timestamp.equals(t.timestamp);
    }

    @Override
    public int hashCode() {
        return Objects.hash(id, timestamp);
    }

    @Override
    public int compareTo(Transaction other) {
        return this.timestamp.compareTo(other.timestamp);
    }
}

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

Подсказки

💡 Проверьте сравнение объектов id в методе equals().

💡 Сравните поля, участвующие в equals(), hashCode() и compareTo().

💡 Обратите внимание на использование generic-типа в Comparable.

💡 java.util.Date является изменяемым объектом.

💡 Для денежных значений Double подходит не всегда.

Решение

В коде есть несколько проблем:

id == t.id сравнивает ссылки, а не значения объектов;
используется raw type Comparable<Transaction>;
compareTo() сравнивает только timestamp, поэтому две разные транзакции с одинаковым временем будут считаться равными с точки зрения сортировки;
hashCode() не учитывает amount, хотя это поле участвует в equals();
Date изменяемый, поэтому состояние объекта можно изменить через переданный объект или getter;
для денежных значений предпочтительнее использовать BigDecimal;
желательно явно проверять обязательные поля на null.

Например, класс можно исправить следующим образом:

public final class Transaction<T extends Number>
        implements Comparable<Transaction<T>> {

    private final T id;
    private final BigDecimal amount;
    private final Instant timestamp;

    public Transaction(
            T id,
            BigDecimal amount,
            Instant timestamp
    ) {
        this.id = Objects.requireNonNull(id);
        this.amount = Objects.requireNonNull(amount);
        this.timestamp = Objects.requireNonNull(timestamp);
    }

    public T getId() {
        return id;
    }

    public BigDecimal getAmount() {
        return amount;
    }

    public Instant getTimestamp() {
        return timestamp;
    }

    @Override
    public boolean equals(Object o) {
        if (this == o) {
            return true;
        }

        if (!(o instanceof Transaction<?> other)) {
            return false;
        }

        return Objects.equals(id, other.id)
                && Objects.equals(amount, other.amount)
                && Objects.equals(timestamp, other.timestamp);
    }

    @Override
    public int hashCode() {
        return Objects.hash(id, amount, timestamp);
    }

    @Override
    public int compareTo(Transaction<T> other) {
        int result = timestamp.compareTo(other.timestamp);

        if (result != 0) {
            return result;
        }

        return id.toString().compareTo(other.id.toString());
    }
}

Главная ошибка в equals():

id == t.id

Для объектов необходимо использовать сравнение по значению:

Objects.equals(id, other.id)

Также важно согласовать compareTo() с equals(). Если сравнивать только время, то в TreeSet две разные транзакции с одинаковым timestamp могут восприниматься как один элемент.

Если класс используется для финансовых операций, BigDecimal предпочтительнее Double, поскольку позволяет избежать ошибок представления чисел с плавающей точкой.