
그룹 가입 서비스 로직은 아래와 같이 작성되어있다.
@Transactional
public void joinGroup(GroupJoinRequestDto dto, Long groupId) {
Group group = findGroupById(groupId);
if (joinRequestService.pendingRequestExists(group.getId())) {
throw new DuplicateElementException("이미 가입 요청한 그룹입니다.");
}
try {
Profile profile = profileService.getLoggedInProfile(groupId);
switch (profile.getState()) {
case RESTRICTED -> {
throw new UnAuthorizedException("추방당한 회원입니다. userId : " + profile.getUserId());
}
case GENERAL -> {
throw new DuplicateElementException("이미 가입한 그룹입니다.");
}
case DELETED -> {
if (group.getJoinCondition() == JoinCondition.OPEN) {
profileService.checkGroupSize(group);
profile.setState(State.GENERAL);
} else {
joinRequestService.createNewGroupJoinRequest(dto, group);
}
}
}
} catch (NoLoggedInProfileException e) {
if (group.getJoinCondition() == JoinCondition.OPEN) {
profileService.checkGroupSize(group);
profileService.createNewProfile(dto.getNickname(), GroupRole.MEMBER, group);
} else {
joinRequestService.createNewGroupJoinRequest(dto, group);
}
}
}
흠... 작성하고보니 어디서 똥냄새가 스멀스멀 올라오는게 뭔가 별로인 것 같다. 어디서 냄새가 나는지 알아보고, 향기 나는 보다 읽기 편한 코드로 리팩토링하는 과정을 정리해본다.
가장 처음 생각한 문제는 읽기 불편함, 즉 가독성이 떨어진다는 것이다. 타 개발자가 해당 코드를 리뷰한다면 어떤 생각이 들까?
- 그룹을 찾는다.
- 가입 요청이 있는지 확인한다.
- 프로필을 조회한다.
- 프로필 상태가
RESTRICTED일 때의 처리- 프로필 상태가
GENERAL일 때의 처리- 프로필 상태가
DELETED일 때
6-1. 그룹의JoinCondition이OPEN일 때의 처리
6-2. 이외의 처리- 프로필을 찾지 못했을 때
7-1. 그룹의JoinCondition이OPEN일 때의 처리
7-2. 이외의 처리
하나의 메소드를 리뷰함에 있어서 모든 비즈니스 요구사항을 알아야 한다.
또한, 흐름에 따른 절차를 나열하는 식으로 구현되었기에 가독성이 더욱 저하된다고 생각한다.
띄어쓰기를하지않으면읽기불편해지는것과같은느낌이라고생각한다.
현재는 30~40줄의 코드이기에 금방 읽을 수 있지만, 100줄, 200줄이 넘어가면 리뷰 난이도가 기하급수적으로 올라갈 것이다.
만약 비즈니스 요구사항이 변경된다면 어떻게 될까? 현재 상황에선 서비스 로직 자체가 수정되어야 한다. 이는 변경에 닫혀있어야 한다 라는 OCP를 위반하며, 변경에 취약한 프로그램을 만드는데 일조한다.
@Transactional
public void joinGroup(GroupJoinRequestDto dto, Long groupId) {
...
try {
Profile profile = profileService.getLoggedInProfile(groupId);
switch (profile.getState()) {
case SOMETHING -> {
// 새로운 상태 추가
}
case RESTRICTED -> {
// 혹은 기존 처리 로직 변경
// throw new UnAuthorizedException("추방당한 회원입니다. userId : " + profile.getUserId());
}
case GENERAL -> {
throw new DuplicateElementException("이미 가입한 그룹입니다.");
}
case DELETED -> {
...
}
}
}
...
}
전략 패턴이란 행위를 캡슐화한 인터페이스를 두고, 해당 인터페이스를 구현하는 전략 클래스를 통해 객체의 행위를 동적으로 바꾸는 디자인 패턴이다.
전략 패턴은 특히 전략이 빈번하게 추가되어야 할 때 빛을 발하는데, 기존 비즈니스 로직을 수정할 필요 없이 전략 클래스를 새로 생성하는 것만으로도 처리 로직을 추가할 수 있기 때문이다.
Consumer는 Java에서 함수형 프로그래밍을 구현하기 위해 Java 1.8부터 도입된 함수형 인터페이스이다.
이를 통해 특정 상태에 대한 처리 로직을 추출할 수 있다.
위에서 알아본 두 방법 중 무엇을 선택해야 할까?
현재의 요구 사항을 확인하며 어떤 선택이 더 효율적일지 생각해보자.
- 처리 상태가 추가될 가능성은 희박하다.
- 처리 로직이 변경될 가능성은 존재한다.
필자는 Consumer를 선택했다. 이유는 다음과 같다.
- 상태가 추가될 가능성이 희박하다. 전략 패턴의 장점이 빛을 발하지 못한다.
- 전략 패턴은 인터페이스를 분리해야한다. 3개의 상태를 추가하기 위해 인터페이스와 전략 클래스 등을 추가로 구현해야 한다.
결론은 전략 패턴을 사용하여 얻는 이득보다 구현에 필요한 노력이 더 크다고 생각했기에, Consumer를 사용했다.
먼저 Consumer들을 관리할 범용 클래스를 만들어주었다.
@Component
@RequiredArgsConstructor
public class Consumers {
private final GroupService groupService;
private final GroupJoinRequestService joinRequestService;
private final ProfileService profileService;
private Map<State,BiConsumer<Profile, GroupJoinRequestDto>> groupJoinConsumerMap;
public BiConsumer<Profile, GroupJoinRequestDto> getGroupJoinConsumer(State state) {
return groupJoinConsumerMap.get(state);
}
@PostConstruct
public void initGroupJoinConsumerMap() {
groupJoinConsumerMap = new EnumMap<>(State.class);
groupJoinConsumerMap.put(State.RESTRICTED, (profile, dto) -> {
throw new UnAuthorizedException("추방당한 회원입니다. userId : " + profile.getUserId());
});
groupJoinConsumerMap.put(State.GENERAL, (profile, dto) -> {
throw new DuplicateElementException("이미 가입한 그룹입니다.");
});
groupJoinConsumerMap.put(State.DELETED, (profile, dto) -> {
Group group = profile.getGroup();
if (group.getJoinCondition() == JoinCondition.OPEN) {
groupService.checkGroupSize(group);
group.increaseProfileCount();
profile.setState(State.GENERAL);
} else {
joinRequestService.createNewGroupJoinRequest(dto, group);
}
});
}
}
Consumers 클래스는 빈으로 등록해 관리해준다. @PostConstruct를 통해 어플리케이션 실행 시에 처리할 로직들을 등록해준다.
@Transactional
public void joinGroup(GroupJoinRequestDto dto, Long groupId) {
Group group = groupService.findGroupById(groupId);
if (joinRequestService.pendingRequestExists(group.getId())) {
throw new DuplicateElementException("이미 가입 요청한 그룹입니다.");
}
try {
Profile profile = profileService.getLoggedInProfile(groupId);
// 상태에 맞는 컨슈머를 받아옴
BiConsumer<Profile, GroupJoinRequestDto> action =
consumers.getGroupJoinConsumer(profile.getState());
if (action != null) {
action.accept(profile, dto);
} else {
log.warn(NoStateExceptionMessage, "그룹 가입", profile.getState());
throw new IllegalStateException("No action found : " + profile.getState());
}
} catch (NoLoggedInProfileException e) {
firstJoinProcess(dto, group);
}
}
private void firstJoinProcess(GroupJoinRequestDto dto, Group group) {
if (group.getJoinCondition() == JoinCondition.OPEN) {
groupService.checkGroupSize(group);
profileService.createNewProfile(dto.getNickname(), jwtReader.getUserId(), GroupRole.MEMBER, group);
} else {
joinRequestService.createNewGroupJoinRequest(dto, group);
}
}
트랜잭션 스크립트 형식으로 나열되었던 기존 비즈니스 로직들이 모두 사라지고 Consumers 클래스에게 책임을 위임했다. 이를 통해 기존 40줄 정도였던 코드를 20줄 내외로 단축시킬 수 있었다.
바뀐 코드를 리뷰할 때 드는 생각은 어떨까.
- 그룹을 찾는다.
- 가입 요청이 있는지 확인한다.
- 프로필을 조회한다.
- 프로필에 따라 가입 로직을 실행한다.
- 프로필을 찾지 못했을 때 첫 가입 로직을 실행한다.
이전보다 확연히 간단해졌고, 비즈니스 요구사항을 알지 못해도 코드 자체를 리뷰할 수 있도록 변경되었다.
이번 리팩토링을 진행하면서 가장 많으 느꼈던 것은
남이 보기 좋은 코드를 작성해야 한다.
였다. 항상 그렇게 코드를 짜고 있다고 생각은 했지만, 그건 나만의 생각 전혀 그렇지 않다는 것을 깨달은 아주 값진 경험이었다..!
