[REFACTOR] 게시글 도메인 리팩터링

[woo-chang]

🔥 연관 이슈

• close: [REFACTOR] 게시글 도메인 리팩터링 #512

📝 작업 요약

게시글 도메인을 리팩토링하였습니다.

⏰ 소요 시간

3일 .. ?

예상치 못한 부분에서 에러가 종종 발생하여 생각보다 시간이 오래 걸리게 된 것 같습니다 🥲

많은 기능을 혼자 구현하고 개발한 아벨 존경합니다 🙇🏻‍♂️

🔎 작업 상세 설명

• 한 클래스에 1000줄이 넘던 코드를 역할에 맞게 분리하였습니다.
  • PostGuestService : 게시글과 관련한 비회원 로직
  • PostCommandService : 게시글 생성, 수정, 삭제 등과 같이 도메인의 변화를 주는 로직
  • PostQueryService : 게시글 도메인의 변화 없이 조회만 발생하는 로직
• 연관관계를 값 객체에서 상위 엔티티로 올려 연관관계가 잘 보이도록 수정하였습니다.
  • 추후에 연관관계 엔티티의 로직이 많아진다면 값 객체로 분리해봐도 좋을 것 같습니다!
• 중복되는 코드를 제거하고 중복이 발생하는 부분은 메서드로 추출하였습니다.
• 하나의 메서드에서는 하나의 역할만 수행하도록 하여 코드 가독성을 개선하였습니다.
• 코드 컨벤션이 일치하지 않았던 부분에 대해 코드 컨벤션을 수정하였습니다.
• 어노테이션 기반의 테스트 유틸이 아닌 상속 구조의 테스트 유틸을 생성하여 테스트에서 중복되는 부분은 재사용할 수 있도록 하였습니다.
• 리팩토링 전 214개의 테스트에서 100개 가량 테스트를 작성하여 329개의 테스트로 검증 안정성을 높였습니다.

🌟 논의 사항

리팩토링하면서 코드 컨벤션으로 맞추고 싶은 부분 Discussion에 올려두었는데 같이 얘기해보면 좋을 것 같아요 :)

#526

PR이 생각보다 커진 것 같아 리뷰를 해주시는 분들께 죄송하네요 🥲 리뷰를 통해 많은 질문과 커멘트 주시면 많이 배워가도록 하겠습니다!

[github-actions]

Test Results

364 tests   364 ✔️  17s ⏱️
111 suites      0 💤
111 files        0 ❌

Results for commit 443b151.

♻️ This comment has been updated with latest results.

[aiaiaiai1]

다즐 질문답변과 리뷰반영하시느라 고생많으셨습니다! 👍👍
빨리 배포됐음 좋겠네요! 근데 그 컨플릭트.. 어마무시한거같던데 .. 화이팅..입니다..!!

[tjdtls690]

P1
PostGuestService -> PostGuestQueryService 변경 후 @transactional(readOnly = true)를 클래스 위에 붙여주면 될 것 같아요 :)

[woo-chang]

현재는 해당 PR이 빨리 머지되는 것이 더 우선시되는 상황인 것 같아 해당 부분은 나중에 전체적으로 Query와 Command를 분리하면서 수정하도록 하겠습니다 !

[tjdtls690]

사실 지금도 많이 헷갈리네요. 어떤 상황에 대한 판단(true or false)까지는 도메인의 역할이 확실하다고 주장할 수 있지만, throw를 통해 예외를 던지는 행위 자체는 도메인의 역할로 볼 수 있을까? 아니면 지금처럼 Service 계층에서 던져주어야 할까? 라는 고민이 가장 큽니다.

제가 직접적으로 느낀 장단점을 말해보자면,

서비스 계층에서 예외를 던지는 입장에서의 장단점
장점

1. 서비스 계층의 한 부분에 예외 처리 로직을 응집시킬 수 있어서 파악하기 편할 수 있다.
2. 각 도메인 객체들이 상태에 대한 부분만을 갖고 있을 수 있다.

단점

1. 예외 처리 상황이 많아지면 Service의 크기가 기하급수적으로 커질 수 있다.
2. 여러 상태(도메인)에 대한 예외 처리 로직을 하나의 Service가 모두 갖고 있어서 오히려 가독성이 떨어질 수 있다.

이에 대한 다즐의 의견이 궁금합니다.

[tjdtls690]

P2

Suggested change

	private List<PostOptionUpdateRequest> extractPostOptionAdds(
	final Set<Long> postOptionIds,
	final List<PostOptionUpdateRequest> postOptionUpdates
	) {
	final boolean hasInvalidId = postOptionUpdates.stream()
	.map(PostOptionUpdateRequest::getId)
	.anyMatch(id -> id != null && !postOptionIds.contains(id));
	if (hasInvalidId) {
	throw new BadRequestException(PostOptionExceptionType.UNRELATED_POST_OPTION);
	}
	return postOptionUpdates.stream()
	.filter(postOptionUpdate -> postOptionUpdate.getId() == null)
	.toList();
	}
	private List<PostOptionUpdateRequest> extractPostOptionAdds(
	final Set<Long> postOptionIds,
	final List<PostOptionUpdateRequest> postOptionUpdates
	) {
	validatePostOptionIds(postOptionIds, postOptionUpdates);
	return postOptionUpdates.stream()
	.filter(postOptionUpdate -> postOptionUpdate.getId() == null)
	.toList();
	}
	private void validatePostOptionIds(
	final Set<Long> postOptionIds,
	final List<PostOptionUpdateRequest> postOptionUpdates
	) {
	boolean hasInvalidId = postOptionUpdates.stream()
	.map(PostOptionUpdateRequest::getId)
	.anyMatch(id -> id != null && !postOptionIds.contains(id));
	if (hasInvalidId) {
	throw new BadRequestException(PostOptionExceptionType.UNRELATED_POST_OPTION);
	}
	}

이런 식으로 분리해준다면 메인 메서드는 주된 로직에만 더 집중하고 유효성 검증 로직도 따로 분리하여 더 명확해 질 수 있을 것 같은데 어떻게 생각하시나요 :)

[tjdtls690]

혹은 이러한 방식도 생각해볼 수 있을 것 같아요.

Suggested change

	private List<PostOptionUpdateRequest> extractPostOptionAdds(
	final Set<Long> postOptionIds,
	final List<PostOptionUpdateRequest> postOptionUpdates
	) {
	final boolean hasInvalidId = postOptionUpdates.stream()
	.map(PostOptionUpdateRequest::getId)
	.anyMatch(id -> id != null && !postOptionIds.contains(id));
	if (hasInvalidId) {
	throw new BadRequestException(PostOptionExceptionType.UNRELATED_POST_OPTION);
	}
	return postOptionUpdates.stream()
	.filter(postOptionUpdate -> postOptionUpdate.getId() == null)
	.toList();
	}
	private List<PostOptionUpdateRequest> extractPostOptionAdds(
	final Set<Long> postOptionIds,
	final List<PostOptionUpdateRequest> postOptionUpdates
	) {
	validatePostOptionIds(postOptionIds, postOptionUpdates);
	return postOptionUpdates.stream()
	.filter(postOptionUpdate -> postOptionUpdate.getId() == null)
	.toList();
	}
	private void validatePostOptionIds(
	final Set<Long> postOptionIds,
	final List<PostOptionUpdateRequest> postOptionUpdates
	) {
	postOptionUpdates.stream()
	.map(PostOptionUpdateRequest::getId)
	.filter(Objects::nonNull)
	.filter(id -> !postOptionIds.contains(id))
	.findFirst()
	.ifPresent(invalidId -> {
	throw new BadRequestException(PostOptionExceptionType.UNRELATED_POST_OPTION);
	});
	}

[woo-chang]

스트림 활용을 너무 잘 하시네요 👍👍

[tjdtls690]

Q
이 부분에서 Function.identity()를 쓰기엔 가독성 측면에서 좀 안좋게 느껴지는지 궁금합니다.

[woo-chang]

이 부분 고민해봤는데 가독성이 너무 안좋더라구요 🥲 따라서 명시하는 방법을 선택했습니다 :)

[jeomxon]

8천줄의 레전드 리팩터링을 하시느라 고생하셨습니다..!
🔥좋은 코드 덕분에 저도 많이 배웠네요 ㅎㅎ🔥
이만 approve하겠습니다 :)

[tjdtls690]

많은 코드 리뷰 받고 수정하시느라 고생 많으셨어요!! 역시 ㅡ 즐 ㅡ

[github-actions]

⚡️ Lighthouse report!

Category	Score
🔴 Performance	45
🟠 Accessibilty	88
🟠 SEO	85
🟠 PWA	89

Category	Score
🟢 First Contentful Paint	0.7 s
🔴 Largest Contentful Paint	5.8 s
🔴 Total Blocking Time	4,270 ms
🟢 Cumulative Layout Shift	0
🟠 Speed Index	5.5 s