Провести ревью HR-модуля

36. Провести ревью HR-модуля

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

Вам пришел этот модуль на код ревью, посмотрите его и исправьте так, как посчитаете нужным.

Краткое описание.

Есть 4 сущности:

Company — Компания;
Department — Отдел;
Person — Сотрудник;
Job — Должность.

В Компании есть Отделы, в Отделе состоят Сотрудники, а у Сотрудников есть Должности.

Есть два контроллера: Контроллер Компаний и Контроллер Департаментов.

В задаче сотрудника было:

посчитать медианную зарплату у каждой Должности в Компании;
сделать CRUD-операции;
посчитать количество сотрудников в Отделе;
посчитать общее количество Сотрудников в Компании.

Код:

package ru.example.hr.hr1;

import jakarta.persistence.*;
import lombok.Data;
import lombok.NoArgsConstructor;

import java.util.List;
import java.util.UUID;

@Data
@NoArgsConstructor
@Entity
@Table(name = "companies")
public class Company {

    @Id
    private UUID id;

    private String name;

    private String address;

    @OneToMany(
            mappedBy = "company",
            cascade = {CascadeType.ALL},
            fetch = FetchType.EAGER
    )
    private List<Department> departments;
}

import jakarta.persistence.*;
import lombok.Data;
import lombok.NoArgsConstructor;

import java.util.Set;
import java.util.UUID;

@Data
@NoArgsConstructor
@Entity
@Table(name = "departments")
public class Department {

    @Id
    private UUID id;

    @Transient
    private String name;

    @OneToMany(
            mappedBy = "department",
            fetch = FetchType.EAGER
    )
    private Set<Person> people;

    @ManyToOne
    private Company company;
}

@Data
@NoArgsConstructor
@Entity
@Table(name = "persons")
public class Person {

    @Id
    private UUID id;

    private String fio;

    @ManyToOne(fetch = FetchType.LAZY)
    private Department department;

    @ManyToOne
    private Job job;

    private Double salary;
}

@Data
@NoArgsConstructor
@Entity
@Table(name = "jobs")
public class Job {

    @Id
    private UUID id;

    private String name;

    private Double minPayment;

    private Double maxPayment;

    @OneToMany
    private Set<Person> people;
}

@Repository
public interface CompanyRepository
        extends JpaRepository<Company, UUID> {

    @Query("SELECT c FROM Company c WHERE c.id <> :id")
    List<Company> getOtherCompanies(UUID id);
}

import jakarta.persistence.EntityNotFoundException;
import lombok.AllArgsConstructor;
import org.springframework.stereotype.Service;

import java.util.*;
import java.util.stream.Collectors;

@Service
@AllArgsConstructor
public class CompanyService {

    private CompanyRepository repository;
    private DepartmentService departmentService;

    public List<Department> findSameDepartmentsWithNameInOtherCompanies(
            Department d1
    ) {
        return repository.getOtherCompanies(d1.getCompany().getId()).stream()
                .map(Company::getDepartments)
                .flatMap(Collection::stream)
                .filter(department ->
                        d1.getName().equals(department.getName())
                )
                .collect(Collectors.toList());
    }

    public Map<Job, Double> getMedianByJobInDepartmentById(UUID id) {
        Company company = repository.findById(id)
                .orElseThrow(EntityNotFoundException::new);

        Map<Job, List<Double>> jobSalaries = new HashMap<>();

        company.getDepartments().stream()
                .map(department ->
                        departmentService
                                .getSalariesByJobInDepartment(department)
                )
                .forEach(jobListMap ->
                        jobListMap.forEach((k, v) ->
                                jobsSalaries.merge(
                                        k,
                                        v,
                                        (currentList, toAdd) -> {
                                            currentList.addAll(toAdd);
                                            return currentList;
                                        }
                                )
                        )
                );

        Map<Job, Double> medians = new HashMap<>();

        jobSalaries.forEach((k, v) -> {
            Double median = 0d;

            List<Double> sortedSalaries = v.stream()
                    .sorted()
                    .collect(Collectors.toList());

            if (sortedSalaries.size() % 2 == 0) {
                median = (
                        sortedSalaries.get(sortedSalaries.size() / 2)
                        + sortedSalaries.get(
                                sortedSalaries.size() / 2 + 1
                        )
                ) / 2;
            } else {
                median = sortedSalaries.get(
                        sortedSalaries.size() / 2 + 1
                );
            }

            medians.put(k, median);
        });

        return medians;
    }
}

@RestController
public class CompanyController {

    private CompanyService service;

    @GetMapping("/companies/{id}/median")
    public Map<Job, Double> getMedianSalariesByJobInCompany(
            @PathVariable("id") UUID id
    ) {
        return service.getMedianByJobInDepartmentById(id);
    }
}

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

Подсказки

💡 Обратите внимание на JPA-связи и тип загрузки коллекций.

💡 Проверьте назначение @Transient у поля Department.name.

💡 Проверьте индексы при вычислении медианы.

💡 Для количества сотрудников необязательно загружать все записи из БД.

💡 Проверьте внедрение CompanyService в контроллер.

Решение

В коде можно выделить следующие проблемы:

Department.name помечен @Transient, поэтому не сохраняется в БД;
коллекции загружаются через EAGER без необходимости;
в Job связь с Person должна иметь mappedBy = "job";
@Data на JPA-сущностях может создавать проблемы с equals(), hashCode() и toString();
для зарплаты предпочтительнее использовать BigDecimal;
объявлена переменная jobSalaries, но используется jobsSalaries;
медиана рассчитывается с неправильными индексами;
зависимость CompanyService в контроллере не внедряется;
количество сотрудников лучше считать непосредственно в БД.

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

@Getter
@Setter
@NoArgsConstructor
@Entity
@Table(name = "departments")
public class Department {

    @Id
    private UUID id;

    private String name;

    @OneToMany(mappedBy = "department")
    private Set<Person> people = new HashSet<>();

    @ManyToOne(fetch = FetchType.LAZY)
    private Company company;
}

@Getter
@Setter
@NoArgsConstructor
@Entity
@Table(name = "jobs")
public class Job {

    @Id
    private UUID id;

    private String name;

    private BigDecimal minPayment;

    private BigDecimal maxPayment;

    @OneToMany(mappedBy = "job")
    private Set<Person> people = new HashSet<>();
}

В расчёте медианы для нечётного количества элементов нужен элемент с индексом size / 2, а для чётного — среднее элементов с индексами size / 2 - 1 и size / 2.

Исправленный метод:

private double calculateMedian(List<Double> salaries) {
    List<Double> sorted = salaries.stream()
            .sorted()
            .toList();

    int size = sorted.size();

    if (size == 0) {
        throw new IllegalArgumentException("Salary list is empty");
    }

    if (size % 2 != 0) {
        return sorted.get(size / 2);
    }

    return (
            sorted.get(size / 2 - 1)
                    + sorted.get(size / 2)
    ) / 2;
}

Основной метод после исправления:

public Map<Job, Double> getMedianByJobInCompany(UUID companyId) {
    Company company = repository.findById(companyId)
            .orElseThrow(EntityNotFoundException::new);

    Map<Job, List<Double>> jobSalaries = new HashMap<>();

    company.getDepartments().stream()
            .map(department ->
                    departmentService
                            .getSalariesByJobInDepartment(department)
            )
            .forEach(map ->
                    map.forEach((job, salaries) ->
                            jobSalaries.merge(
                                    job,
                                    new ArrayList<>(salaries),
                                    (current, added) -> {
                                        current.addAll(added);
                                        return current;
                                    }
                            )
                    )
            );

    return jobSalaries.entrySet().stream()
            .collect(Collectors.toMap(
                    Map.Entry::getKey,
                    entry -> calculateMedian(entry.getValue())
            ));
}

Контроллер лучше реализовать через constructor injection:

@RestController
@RequestMapping("/companies")
@RequiredArgsConstructor
public class CompanyController {

    private final CompanyService companyService;

    @GetMapping("/{id}/median")
    public Map<Job, Double> getMedianSalariesByJobInCompany(
            @PathVariable UUID id
    ) {
        return companyService.getMedianByJobInCompany(id);
    }
}

Количество сотрудников в отделе и компании лучше считать отдельными запросами:

long countByDepartmentId(UUID departmentId);

@Query("""
       select count(p)
       from Person p
       where p.department.company.id = :companyId
       """)
long countByCompanyId(UUID companyId);

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