시작은 골라주마 코드를 서로 설명하는 시간에서 나왔다.
다른 분 코드를 보는데, 아.. 너무 분기처리가 심한데? 라는 생각이 들었다.
분기처리로 인해서 코드를 파악하기가 너무나도 힘들었고, 더 큰 문제는 골라주마에서 카테고리가 추가되었을 때 그만큼 분기처리가 늘어난다. 의 문제였다.
이 회의를 하고 돌아오는데, eeos코드를 보니
어머나.. 나도 똑같은 문제가 있다는 것을 알게 되었다.
(골라주마 팀 서로 코드 설명하기 시간 너무 좋아요.)
문제 상황
eeos 에서는 ACTIVE, END에 따라서 조회하는 쿼리가 달라진다.
따라서 ProgramStatus를 기준으로 분기처리를 해야한다.
@Override
public PageResponse<GetProgramsResponse> getPrograms(String status, int size, int page) {
LocalDate currentDate = LocalDate.now(ZoneId.of("Asia/Seoul")); // 한국 시간대로 현재 날짜 가져오기
Timestamp now = Timestamp.from(currentDate.atStartOfDay(ZoneId.systemDefault()).toInstant());
PageRequest pageRequest = PageRequest.of(page, size);
if (ProgramStatus.isSameStatus(status, ProgramStatus.ACTIVE)) {
Page<ProgramEntity> pages = programRepository.findAllByIng(now, pageRequest);
return pageResponseConverter.from(pages);
}
if (ProgramStatus.isSameStatus(status, ProgramStatus.END)) {
Page<ProgramEntity> pages = programRepository.findAllByEnd(now, pageRequest);
return pageResponseConverter.from(pages);
}
throw new NotFoundStatusException();
}나는 이 코드의 문제점을 위에서 언급한 것처럼
ProgramStatus가 늘어났을 때는 분기처리를 계속 해줘야 한다.로 보았다.
또한, getPrograms의 역할은 status에 해당하는 program들을 가져오는 것이므로
// 현 시간을 구한다.
// status에 맞는 결과를 가져온다.위와 같은 책임만 가지면 된다고 생각했다.
첫 번째 리팩터링
Map<ProgramStatus, ProgramStateService> 을 만드는 Factory를 정의하여
Service에서는 factory에서 status를 전달하여 호출한 status에 맞는 service에 요청하면 되겠다. 라고 생각했다.
이로써, service에서는 분기처리에 직접 신경을 쓰지 않아도 되고,
getPrograms의 역할에 맞게 요청한 status를 넘기면 알맞는 service를 호출할 수 있다.
(이상한 부분이 어딘지 알겠죠? 맞아요. 뒤에 리팩터링 또 나옵니다!)
@Override
public PageResponse<GetProgramsResponse> getPrograms(String status, int size, int page) {
Timestamp now = DateConverter.toEpochSecond(LocalDate.now());
PageRequest pageRequest = PageRequest.of(page, size);
Map<ProgramStatus, ProgramStateService> programStatusStrategy = programStatusFactory.make();
ProgramStateService programStateService =
programStatusStrategy.get(ProgramStatus.getStatus(status));
Page<ProgramEntity> pages = programStateService.getPages(now, pageRequest);
return pageResponseConverter.from(pages);
}@Component
@RequiredArgsConstructor
public class ProgramStatusFactory {
private final ProgramRepository programRepository;
public Map<ProgramStatus, ProgramStateService> make() {
Map<ProgramStatus, ProgramStateService> stateServices = new EnumMap<>(ProgramStatus.class);
stateServices.put(ProgramStatus.ACTIVE, new ActiveProgramStateService(programRepository));
stateServices.put(ProgramStatus.END, new EndProgramStateService(programRepository));
return stateServices;
}
}두 번째 리팩터링
위에서 잘못 처리한 부분이 있는데
이미 빈으로 등록한 service가 있기 때문에 또 다시 service를 생성하지 않고 이미 빈으로 등록된 service를 가져오면 된다.
세 번째 리팩터링
처음에 Factory라는 클래스명을 지은 이유는 말 그대로 ProgramStatusService 전체를 담고(알고) 있는 클래스를 만들고 싶었기 때문이다.
하지만, 위 방식의 단점은 처음에 내가 원하던 바를 다 충족하지 못 한다.
처음에 리팩터링을 진행한 이유 자체가
status에 맞는 결과값을 가져온다*이 였다.
하지만, 2번째 리팩터링 같은 경우에는 service(즉 클라이언트)가 어떤 service를 참조하고 있는지까지 굳이굳이 알아야 한다.
이게 service의 책임은 아니라고 생각했다.
단지, service는 status에 맞는 결과만 가지고 올 뿐!!!
그래서 처음에는 Factory 클래스에서 바로 getPages를 리턴하도록 하였다.
하지만, 이 코드의 문제점은
- Factory에서 getPages를 리턴하는 게 어색하다.
클래스명만 봤을 때는 무언가를 생성한다. 인데, ProgramStatusFactory가 아닌 다른 값을 리턴하고 있다. - ProgramStatusService가 늘어날 때마다 의존하는 클래스가 늘어난다.
- ProgramStatusServiceFactory에서 결국은 getPages를 리턴하는 게 목적이라면, ProgramStatusServiceFactory는 각 service를 관리하는 게 아니라 Map<ProgramStatus, ProgramStateService>를 관리하는 게 맞다.
@Component
public class ProgramStatusServiceFactory {
private final ActiveProgramStatusService activeProgramStatusService;
private final EndProgramStatusService endProgramStatusService;
private final ProgramRepository programRepository;
public Map<ProgramStatus, ProgramStateService> make() {
Map<ProgramStatus, ProgramStateService> stateServices = new EnumMap<>(ProgramStatus.class);
stateServices.put(ProgramStatus.ACTIVE, activeProgramStatusService);
stateServices.put(ProgramStatus.END, new endProgramStatusService);
return stateServices;
}
public Page<ProgramEntity> getPages(ProgramStatus programStatus, Timestamp now, PageRequest pageRequest) {
return getProgramStatusService(programStatus).getPages(now, pageRequest);
}
private ProgramStateService getProgramStatusService(ProgramStatus programStatus) {
return programs.get(programStatus);
}
}네 번째 리팩터링
- Factory에서 getPages를 리턴하는 게 어색하다.
클래스명만 봤을 때는 무언가를 생성한다. 인데, ProgramStatusFactory가 아닌 다른 값을 리턴하고 있다.
=> 이를 해결하기 위해 Factory에서 composite로 변경하였다.
client 코드는 각 하위 객체(service)를 아는 것이 아니라 Composite객체만 안다.
- ProgramStatusService가 늘어날 때마다 의존하는 클래스가 늘어난다.
=> 이를 해결하기 위해 ProgramStatusServiceComposite의 생성자가 Set를 받도록 하였다. 스프링이 ProgramStatusService를 구현한 객체들을 주입해줄 것이다. - ProgramStatusServiceFactory에서 결국은 getPages를 리턴하는 게 목적이라면, ProgramStatusServiceFactory는 각 service를 관리하는 게 아니라 Map<ProgramStatus, ProgramStateService>를 관리하는 게 맞다.
=> 반영하였다.
생성자를 통해서 ProgramStatusService를 구현한 객체를 전달받고, 이를 필드로 저장하였다.
@Component
public class ProgramStatusServiceComposite {
private final Map<ProgramStatus, ProgramStateService> programs;
public ProgramStatusServiceComposite(Set<ProgramStateService> programStateServices) {
this.programs = programStateServices
.stream()
.collect(Collectors.toMap(ProgramStateService::support, Function.identity()));
}
public Page<ProgramEntity> getPages(ProgramStatus programStatus, Timestamp now, PageRequest pageRequest) {
return getProgramStatusService(programStatus).getPages(now, pageRequest);
}
private ProgramStateService getProgramStatusService(ProgramStatus programStatus) {
return programs.get(programStatus);
}
}이를 사용하는 service(클라이언트)코드
처음에 getPrograms의 역할은 각 status에 맞는 program을 가져오는 것이라고 했고
그러기 위해서
현 시간을 구한다.
현 시간을 기준으로 status에 맞는 프로그램을 가져온다.로 설정하였다.
아래 클라이언트 코드를 보면 그 책임만을 수행하는 것을 알 수 있다.
@Override
public PageResponse<GetProgramsResponse> getPrograms(String status, int size, int page) {
Timestamp now = DateConverter.toEpochSecond(LocalDate.now());
PageRequest pageRequest = PageRequest.of(page, size);
Page<ProgramEntity> pages = programStatusComposite.getPages(ProgramStatus.getStatus(status), now, pageRequest);
return pageResponseConverter.from(pages);
}사실, 아직 현 시간을 구하는 건 어느 객체의 책임인지 잘 모르겠는데..
이는 추가하도록 하겠다.
