코드 리뷰

MVP 기능 구현 완료 이후 프로젝트의 코드를 일괄적으로 튜터님께 리뷰 받았다. 칭찬도 받았지만 지적도 왕창 받았다. 그래서 그 부분에 대해 분석하고 리팩토링 했다.

지적받은 내용은 크게 다음과 같다.

• 메소드 이름을 좀 더 명확하게
  • get~, remove~ 등에서 뒤의 entity는 불필요함
  • 메서드명은 동사적인 개념을 가지고 있도록
• 불필요한 else 제거
• 어떤 동작을 수행한 후에 Optional로 반환하는 것은 지양해야 함
  • 옵셔널 값의 검증 부분을 클라이언트(호출부)에게 넘겨버리는 모양이 되기 때문
  • 검증 이후 최종결과를 담은 객체를 전달해야 함
  • 그렇지 않으면 옵셔널을 처리해야 하는 코드들이 여기 저기 생김
• Application Service와 Domain Service
  • Application Service는 다른 도메인 서비스와의 조합을 만들기 위해 만든 레이어인데, 이를 애플리케이션 단에서 수행하고 있음
  • 때문에 코드의 역할이 중구난방이 됨
  • 인수인계 했을 때 문서로만 넘김
    • 코드를 보고 해석하는 관점과 문서를 보고 해석하는 관점이 다름
    • 가장 중점적으로 봐야하는 곳 → 애플리케이션
    • 애플리케이션 코드만으로 비즈니스 로직을 이해할 수 있어야 함
    • 항상 이러한 부분 감안하며 코드 작성하자
  • 이에 대한 자세한 내용은 프로젝트 아키텍처 포스팅에 정리해뒀다.

• 알림 객체 생성 시 사용되는 mapper의 책임이 너무 많음
  • 전략 패턴을 사용해 알림 이벤트 객체를 생성 중
  • 이 때 사용되는 converter, mapper 등은 일반적으로 단순히 객체를 다른 타입의 객체로 변환하기만 함
  • 그러나 현재 사용되고 있는 mapper는 알림 이벤트 종류에 따라 객체를 다르게 생성하고 있는 자체적인 비즈니스 로직을 가지고 있음
  • 이는 유틸성을 띄고 있는 mapper 클래스에게 너무 큰 책임이라고 생각됨
  • 따라서 이를 분리해야 함

코드 개선

코드를 전부 가져오기는 많아서 맡은 도메인의 주요 포인트만 작성했다.

커뮤니티

ReplyService.java (Application Layer)

• 같은 도메인 서비스를 연속으로 사용할 경우 하나의 도메인 서비스 메서드로 묶기
• 비즈니스 로직 흐름에서 불필요한 동작은 도메인 서비스에게 위임
• 알림 이벤트 발행 로직 간소화

• Before

@Transactional
public ReplyResponse createReply(
	Long problemId,
	Long discussionId,
	ReplyCreateRequest request,
	Long userId
) {

	User user = userDomainService.getUserById(userId);
	Discussion discussion = discussionDomainService.getDiscussionById(discussionId);

	discussionDomainService.validateProblemMatches(discussion, problemId);

	Reply parent = null;
	if (request.parentReplyId() != null) {
		parent = replyDomainService.getReplyById(request.parentReplyId());
		replyDomainService.validateDiscussionMatches(parent, discussion);
	}

	Reply reply = replyDomainService.createReply(
		ReplyCreateRequest.toEntity(discussion, user, parent, request)
	);

	User discussionAuthor = discussion.getUser();	// TODO: get depth 하나로 줄이기
	User parentAuthor = reply.getParent() != null ? reply.getParent().getUser() : null;

	notify(user, discussionAuthor, reply);
	notify(user, parentAuthor, reply);

	return ReplyResponse.fromEntity(reply);
}

...

@Transactional
public ReplyResponse modifyReply(
	Long problemId,
	Long discussionId,
	Long replyId,
	ReplyModifyRequest request,
	Long userId
) {

	Discussion discussion = discussionDomainService.getDiscussionById(discussionId);
	discussionDomainService.validateProblemMatches(discussion, problemId);

	Reply reply = replyDomainService.getReplyById(replyId);
	replyDomainService.validateDiscussionMatches(reply, discussion);
	replyDomainService.validateIsAuthor(reply, userId);

	replyDomainService.modify(reply, request.content());

	return ReplyResponse.fromEntity(reply);
}

• After

@Transactional
public ReplyResponse createReply(
	Long problemId,
	Long discussionId,
	ReplyCreateRequest request,
	Long userId
) {

	User user = userDomainService.getUserById(userId);

	Discussion discussion = discussionDomainService.getDiscussionForProblem(discussionId, problemId);

	Reply reply = replyDomainService.createReply(discussion, user, request.parentReplyId(), request.content());

	List<User> notificationTargets = reply.generateNotificationTargets();

	if (!notificationTargets.isEmpty()) {
		for (User target : notificationTargets) {
			NotificationCreateEvent notificationEvent = replyDomainService.createReplyNotification(target, reply);
			notificationEventService.saveAndNotify(notificationEvent);
		}
	}

	return ReplyResponse.fromEntity(reply);
}

...

@Transactional
public ReplyResponse modifyReply(
	Long problemId,
	Long discussionId,
	Long replyId,
	ReplyModifyRequest request,
	Long userId
) {

	Discussion discussion = discussionDomainService.getDiscussionForProblem(discussionId, problemId);

	Reply reply = replyDomainService.modify(replyId, discussion, userId, request.content());

	return ReplyResponse.fromEntity(reply);
}

위와 같이 코드를 개선함으로써, createReply 메서드의 유저 엔티티 조회 - 토론글 엔티티 조회 - 댓글 엔티티 생성 및 저장 - 알림 발행라는 비즈니스 로직 흐름을 쉽게 파악할 수 있게 됐다.

그리고 중요하지 않은 동작의 코드들은 모두 도메인 서비스에게 위임하면서 가독성도 좋아졌다.

알림

기존에 적용했던 알림 이벤트 객체 생성을 구현한 방식 자체는 잘 만들었지만, 단순히 컨버팅, 매핑 역할만 해야 할 클래스들이 아래처럼 필요 이상의 책임을 가지고 있다고 튜터님이 말씀하셨다.

@Override
public NotificationCreateEvent map(ReplyCreateEvent event) {

	return new NotificationCreateEvent(
		event.principalName(),
		NotificationType.COMMUNITY_REPLY,
		"새로운 댓글이 달렸습니다.",
		new ReplyCreatePayload(event.replyId(), event.discussionId(), event.content()),
		"/redirect",
		false,
		LocalDateTime.now()
	);
}

또한 고정된 값들로 필드에 할당하고 있는데, 추후 여러 파라미터에 따라 redirect url, 메시지 등을 수정할 수 있도록 해야 한다.

ReplyDomainService

public NotificationCreateEvent createReplyNotification(User target, Reply reply) {

	ReplyCreatePayload payload = new ReplyCreatePayload(
		reply.getProblemId(),
		reply.getId(),
		reply.getDiscussionId(),
		reply.getContent()
	);

	return NotificationCreateEvent.of(
		target.getEmail(),
		NotificationType.COMMUNITY_REPLY,
		payload
	);
}

기존의 mapper, converter 파일들을 삭제하고 알림 이벤트 객체 생성은 각자 도메인 서비스에서 처리하도록 위임했다.

이제 애플리케이션 서비스에서는 도메인 서비스를 통해 알림 이벤트 객체를 생성하고 알림 서비스(port)를 통해 이벤트를 발행한다.

알림 객체

알림 이벤트 enum에 message, redirect url 등의 필드를 추가해 관련 정보를 담을 수 있도록 했다.

• Before

@Getter
public enum NotificationType {
	/* 커뮤니티 */
	COMMUNITY_REPLY("댓글"),					// 작성한 discussion, reply, solution 등에 댓글 달림
	COMMUNITY_DISCUSSION_VOTED_UP("자유글 추천"),					// 작성한 discussion, reply, solution 등에 추천 받음
	COMMUNITY_REPLY_VOTED_UP("댓글 추천"),					// 작성한 discussion, reply, solution 등에 추천 받음
	COMMUNITY_MENTIONED("멘션"),				// 누군가 discussion, reply에서 멘션함

	/* 코딩 문제 */
	SUBMISSION_COMPLETED("제출 완료"),	// 제출 및 채점 완료
	;

	private final String description;

	NotificationType(String description) {
		this.description = description;
	}
}

public record NotificationCreateEvent(

	String principalName,

	NotificationType notificationType,

	String message,

	NotificationPayload payload,

	String redirectUrl,

	boolean isRead,

	LocalDateTime createdAt

) {
}

• After

@Getter
public enum NotificationType {
	/* 커뮤니티 */
	COMMUNITY_REPLY("새로운 댓글이 달렸습니다.", "/problems/{problemId}/discussions/{discussionId}"),
	COMMUNITY_DISCUSSION_VOTED_UP("토론글에 추천을 받았습니다.", "/problems/{problemId}/discussions/{discussionId}"),
	COMMUNITY_REPLY_VOTED_UP("댓글에 추천을 받았습니다.", "/problems/{problemId}/discussions/{discussionId}/replies/{replyId}"),
	COMMUNITY_MENTIONED("멘션", ""),
	;

	private final String message;

	private final String redirectUrl;

	NotificationType(String message, String redirectUrl) {
		this.message = message;
		this.redirectUrl = redirectUrl;
	}
}

public record NotificationCreateEvent(

	String principalName,

	NotificationType notificationType,

	NotificationPayload payload,

	boolean isRead,

	LocalDateTime createdAt

) {

	public static NotificationCreateEvent of(String principalName, NotificationType notificationType, NotificationPayload payload) {
		return new NotificationCreateEvent(
			principalName,
			notificationType,
			payload,
			false,
			LocalDateTime.now()
		);
	}
}

추후 개선점

• 알림 객체 생성 로직 트랜잭션 분리
  • 알림 객체 생성 또는 알림 발행 중 예외가 발생하더라도 댓글 저장, 추천 반영 등의 핵심 비즈니스 로직은 롤백되지 않아야 한다.
  • 때문에 알림 관련 로직의 트랜잭션 분리가 필요하다.
  • 추후 이벤트큐 전환 시 함께 적용할 예정