Code review системы расчёта зарплат + бизнес-вопросы

29. Провести ревью системы расчёта зарплат

Условие задачи:
Дан унаследованный класс PayrollSystem, который рассчитывает зарплаты сотрудников. Данные поступают из внешней системы в виде List<Map<String, Object>>, и изменить формат входных данных нельзя.

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

Нужно провести code review, предложить исправления и определить, какие бизнес-требования необходимо уточнить перед изменением поведения системы.

Код:

public class PayrollSystem {
static List<Map<String, Object>> employees = new ArrayList<>();

static {
Map<String, Object> emp1 = new HashMap<>();
emp1.put("id", 1);
emp1.put("name", "Иванов");
emp1.put("base_salary", 40000);
emp1.put("bonus", 10000);
emp1.put("tax_rate", 0.13);
employees.add(emp1);

Map<String, Object> emp2 = new HashMap<>();
emp2.put("id", 2);
emp2.put("name", "Петрова");
emp2.put("base_salary", 50000.54);
emp2.put("bonus", 6000);
emp2.put("tax_rate", 0.13);
employees.add(emp2);

Map<String, Object> emp3 = new HashMap<>();
emp3.put("id", 3);
emp3.put("name", "Сидоров");
emp3.put("base_salary", 30000.25);
emp3.put("bonus", null);
emp3.put("tax_rate", 0.13);
employees.add(emp3);

Map<String, Object> emp4 = new HashMap<>();
emp4.put("id", 4);
emp4.put("name", "Кузнецов");
emp4.put("base_salary", 70000);
emp4.put("bonus", 20000.30);
emp4.put("tax_rate", 0.15);
employees.add(emp4);
}

static double calculateSalary(Map<String, Object> employee) {
double baseSalary = ((Number) employee.get("base_salary")).doubleValue();
double bonus = ((Number) employee.get("bonus")).doubleValue();
double total = baseSalary + bonus;
double tax = ((Number) employee.get("tax_rate")).doubleValue();
total = total - total * tax;
return total;
}

static List<Map<String, Object>> generatePayroll() {
List<Map<String, Object>> payroll = new ArrayList<>();
for (Map<String, Object> emp : employees) {
try {
double netSalary = calculateSalary(emp);
Map<String, Object> entry = new HashMap<>();
entry.put("name", emp.get("name"));
entry.put("salary", netSalary);
payroll.add(entry);
} catch (Exception e) {
continue;
}
}
return payroll;
}

static void printPayroll() {
List<Map<String, Object>> payroll = generatePayroll();
for (Map<String, Object> p : payroll) {
System.out.println(p.get("name") + ": " + p.get("salary") + " руб.");
}
}

static void updateBaseSalary(int employeeId, double newSalary) {
for (Map<String, Object> emp : employees) {
if ((int) emp.get("id") == employeeId) {
emp.put("base_salary", newSalary);
break;
}
}
}

public static void main(String[] args) {
System.out.println("Расчет зарплат:");
printPayroll();
}
}

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

Подсказки

💡 Посмотри, что происходит с сотрудником при любом исключении во время расчёта.
💡 bonus уже в примере может быть null.
💡 Map<String, Object> позволяет получить значение неожиданного типа.
💡 Для денежных расчётов лучше использовать BigDecimal.
💡 Подумай, где на самом деле должны храниться изменения оклада.
💡 Не все требования к отрицательным суммам и округлению можно определить только по коду.

Решение

Проблемы:

catch (Exception) полностью скрывает ошибки и просто удаляет сотрудника из ведомости;
bonus == null приводит к NullPointerException;
неправильный тип поля приводит к ClassCastException;
нет явной валидации обязательных полей;
для денег используется double;
отсутствует правило округления;
не проверяются отрицательные оклад, бонус и итоговая зарплата;
не валидируется диапазон tax_rate;
Map<String, Object> используется во всей бизнес-логике вместо преобразования в типизированную модель;
updateBaseSalary() меняет только локальный static-список;
обновление оклада потеряется при повторной загрузке данных из внешнего источника;
static-хранилище плохо подходит для состояния реальной системы;
printPayroll() смешивает расчёт и представление результата;
ошибка одного сотрудника не должна оставаться полностью незаметной.

Как лучше исправить

Формат внешних данных изменить нельзя, поэтому Map<String, Object> лучше оставить только на границе системы и сразу преобразовывать в типизированную модель.

record EmployeePayData(
        Integer id,
        String name,
        BigDecimal baseSalary,
        BigDecimal bonus,
        BigDecimal taxRate
) {
}

record PayrollEntry(
        Integer id,
        String name,
        BigDecimal salary,
        String error
) {
}

Преобразование входных данных:

static EmployeePayData mapEmployee(
        Map<String, Object> raw
) {
    Integer id = toInteger(raw.get("id"), "id");
    String name = String.valueOf(raw.get("name"));

    BigDecimal baseSalary =
            toDecimal(raw.get("base_salary"), "base_salary");

    BigDecimal bonus = raw.get("bonus") == null
            ? BigDecimal.ZERO
            : toDecimal(raw.get("bonus"), "bonus");

    BigDecimal taxRate =
            toDecimal(raw.get("tax_rate"), "tax_rate");

    if (baseSalary.signum() < 0) {
        throw new IllegalArgumentException(
                "base_salary must not be negative"
        );
    }

    if (taxRate.compareTo(BigDecimal.ZERO) < 0
            || taxRate.compareTo(BigDecimal.ONE) > 0) {
        throw new IllegalArgumentException(
                "tax_rate must be between 0 and 1"
        );
    }

    return new EmployeePayData(
            id,
            name,
            baseSalary,
            bonus,
            taxRate
    );
}

static BigDecimal toDecimal(
        Object value,
        String field
) {
    if (value == null) {
        throw new IllegalArgumentException(
                field + " must not be null"
        );
    }

    if (value instanceof BigDecimal decimal) {
        return decimal;
    }

    if (value instanceof Number number) {
        return new BigDecimal(number.toString());
    }

    if (value instanceof String string) {
        try {
            return new BigDecimal(string);
        } catch (NumberFormatException e) {
            throw new IllegalArgumentException(
                    "Invalid value for " + field,
                    e
            );
        }
    }

    throw new IllegalArgumentException(
            "Invalid type for " + field
    );
}

Расчёт зарплаты:

static BigDecimal calculateSalary(
        EmployeePayData employee
) {
    BigDecimal gross = employee.baseSalary()
            .add(employee.bonus());

    BigDecimal net = gross.multiply(
            BigDecimal.ONE.subtract(employee.taxRate())
    );

    if (net.signum() < 0) {
        throw new IllegalArgumentException(
                "Salary must not be negative"
        );
    }

    return net.setScale(
            2,
            RoundingMode.HALF_UP
    );
}

Ошибку конкретного сотрудника лучше не скрывать:

static List<PayrollEntry> generatePayroll() {
    List<PayrollEntry> payroll = new ArrayList<>();

    for (Map<String, Object> raw : employees) {
        Integer id = null;
        String name = String.valueOf(raw.get("name"));

        try {
            EmployeePayData employee = mapEmployee(raw);
            id = employee.id();

            payroll.add(
                    new PayrollEntry(
                            id,
                            employee.name(),
                            calculateSalary(employee),
                            null
                    )
            );
        } catch (RuntimeException e) {
            payroll.add(
                    new PayrollEntry(
                            id,
                            name,
                            null,
                            e.getMessage()
                    )
            );
        }
    }

    return payroll;
}

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

Что нужно уточнить у бизнеса

Что делать с сотрудником с некорректными данными: исключать из ведомости, показывать с ошибкой или останавливать весь расчёт?
bonus == null означает отсутствие бонуса и 0 или ошибку данных?
Может ли бонус быть отрицательным, например как удержание?
Допустима ли отрицательная итоговая выплата?
Какой диапазон допустим для tax_rate?
Как именно округлять денежные значения и на каком этапе расчёта?
Кто является источником истины для оклада: наша система или внешняя?
Должно ли изменение оклада сохраняться постоянно и передаваться обратно во внешнюю систему?
Если часть сотрудников рассчиталась с ошибками, считается ли вся ведомость успешно сформированной?

Главные технические изменения — не скрывать ошибки, преобразовывать внешние Map в типизированные данные и использовать BigDecimal для денежных расчётов. А правила отрицательных выплат, null-бонуса, округления и изменения оклада нужно зафиксировать вместе с бизнесом.