Newsletter 패키지 구조 리팩터링 #125
Conversation
📝 Jacoco Test Coverage
|
| */ | ||
| @GetMapping("/seminar-notice-basicFrom") | ||
| public ResponseEntity<ResponseBody<SeminarNoticeBasicForm>> getSeminarNoticeBasicForm() { | ||
| return ResponseEntity.ok(createSuccessResponse(newsletterAdminService.getSeminarNoticeBasicForm())); | ||
| return ResponseEntity.ok(createSuccessResponse(new SeminarNoticeBasicForm(newsletterAdminService.getSeminarNoticeBasicForm().getHtmlContent()))); | ||
| } |
There was a problem hiding this comment.
p5: 이건 코드 스타일 취향차이이지만 new 연산자로 객체를 생성하는거보다 static 메서드로 of 메서드 네이밍으로 생성하면 더 깔끔할거 같습니다
There was a problem hiding this comment.
의견 반영하여 of 메서드를 추가하도록 하겠습니다!
| @Component | ||
| @RequiredArgsConstructor | ||
| public class NewsletterHandler { | ||
| private final NewsletterRepository newsletterRepository; | ||
|
|
||
| public void saveNewsletterSubscription(NewsletterSubscription newsletterSubscription) { | ||
| newsletterRepository.saveNewsletterSubscription(newsletterSubscription); | ||
| } | ||
|
|
||
| public void updateNewsletterSubscription(NewsletterSubscription newsletterSubscription) { | ||
| newsletterRepository.updateNewsletterSubscription(newsletterSubscription); | ||
| } | ||
|
|
||
| public void deleteNewsletterSubscription(String email) { | ||
| newsletterRepository.deleteNewsletterSubscription(email); | ||
| } | ||
|
|
||
| public void validateNewsletterSubscription(String email) { | ||
| newsletterRepository.validateNewsletterSubscription(email); | ||
| } |
There was a problem hiding this comment.
P3: 저번에 설명한 의도대로 구현하신거 같습니다. 다만 validate 관련 에러 핸들링을 도메인 Layer에서 하는게 더 직관적이여 보입니다. service나 implement 에서 처리하는게 옳을거 같아요
이유는 에러 또한 비즈니스 로직의 한 부분이기 때문에 에러를 핸들링하는 것 또한 도메인 로직이라 생각합니다
어떻게 생각하시나요
메서드 이름이 validate라면 해당 메서드에서 처리하는게 좋아보이네요
There was a problem hiding this comment.
동의합니다. 도메인 레이어에서 에러 핸들링 및 검증을 할 수 있도록 수정하겠습니다.
| @Override | ||
| @Transactional | ||
| public void updateNewsletterSubscription(NewsletterSubscription newsletterSubscription) { | ||
| Newsletter newsletter = newsletterJpaRepository.findByEmail(newsletterSubscription.getEmail()) | ||
| .orElseThrow(() -> new ServiceException(SUBSCRIBE_NOT_FOUND)); | ||
|
|
||
| newsletter.updateNewsletter(newsletterSubscription.getEmail(), | ||
| newsletterSubscription.getSeminarContentNotice(), | ||
| newsletterSubscription.getStudyNotice(), | ||
| newsletterSubscription.getProjectNotice(), | ||
| newsletterSubscription.getMentoringNotice()); | ||
| } | ||
|
|
||
| @Override | ||
| @Transactional | ||
| public void deleteNewsletterSubscription(String email) { | ||
| Newsletter newsletter = newsletterJpaRepository.findByEmail(email) | ||
| .orElseThrow(() -> new ServiceException(SUBSCRIBE_NOT_FOUND)); | ||
|
|
||
| newsletterJpaRepository.delete(newsletter); | ||
| } | ||
|
|
||
| @Override | ||
| public void validateNewsletterSubscription(String email) { | ||
| if (newsletterJpaRepository.existsByEmail(email)) { | ||
| throw new ServiceException(SUBSCRIBE_EMAIL_CONFLICT); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
p3: validate 로직이 전부 해당 Repository 에서 처리하는게 어색해 보입니다 어떻게 생각하시나요
There was a problem hiding this comment.
확실히 Repository에서 검증을 처리하는게 어색하네요. 수정하겠습니다.
| @RequiredArgsConstructor | ||
| public class NewsletterEventPublisher { | ||
| private final NewsletterRepository newsletterRepository; | ||
| private final EmailService emailService; | ||
| private final ApplicationEventPublisher applicationEventPublisher; | ||
|
|
||
| public void publish(EmailNotificationEvent event) { | ||
| applicationEventPublisher.publishEvent(event); | ||
| } |
There was a problem hiding this comment.
P5 : NewsletterEventPublisher 라는 class에 EventListener가 같이 있는게 조금 어색한 것 같아 보입니다. SRP에 따라 한 클래스는 하나의 책임만 갖는건 어떤가요?? applicationEventPublisher.publishEvent 도입 자체는 좋아보입니다!! 대신 의존성 주입없이 사용할 수 있게 static 메서드로 사용해도 괜찮아 보입니다.
There was a problem hiding this comment.
단일 책임 원칙에 따라 하나의 책임만 가질 수 있도록 NewsletterEventPublisher와 NewsletterEventListener로 분리하겠습니다!
🌱 작업 사항
❓ 리뷰 포인트
🦄 관련 이슈
resolves #이슈번호