[BE] refactor: 서비스 리팩터링 #540

Kimprodp · 2024-08-29T03:56:46Z

resolves [BE] Service 패키지의 기능별 분리 #384

🚀 어떤 기능을 구현했나요 ?

서비스 레이어의 구조를 리팩터링 했습니다.
주요 기능에 대한 서비스들이 존재하고 (등록, 상세 조회, 목록 조회 등)
각 서비스에서 책임별로 분리하여 각각의 모듈을 활용하여 구현하는 형태로 변경했습니다.

주요 변경 부분

기존 : 요청이 발생하면 요청을 검증하고 엔티티를 생성하여 바로 저장하는 형태
변경 : 요청이 발생하면 들어온 요청으로 엔티티를 매핑하고 매핑된 엔티티를 저장하기 전에 검증하는 형태

로 변경했습니다.

기존의 방식은 DTO 단계에서 모든 검증이 수행되기 때문에 단순화될 수 있지만, DTO에 너무 의존적인 서비스가 되어버리는 것 같았습니다. DTO가 변경되면 DTO를 검증하는 로직을 변경해야 하므로 서비스의 유연성이 떨어지게 됩니다.
변경된 방식은 변환, 검증과 같은 로직이 각각 책임에 따라 분리되어 있어 수정이 발생하는 부분만 수정할 수 있습니다.
따라서 DTO가 변경된다면 변환 부분의 로직만 수정되고 엔티티는 변경되지 않으니 검증하는 로직은 수정이 필요 없게 됩니다.
이 과정에서 도메인 간에 협력을 좀 더 넣고 싶었으나, id로 간접 참조한 부분 때문에 쉽지 않았네요..
리뷰 상세 조회와, 리스트 조회 같은 경우 '조회' 응답 객체는 재사용할 수 있을 것 같습니다.
예를 들어, 추후 내가 쓴 리뷰 조회가 추가된다면 reviewDetailMapper를 통해 ReviewDetailResponse를 만들고, 외부 응답에 맞는 부분을 추가한 새로운 응답을 생성하여 반환하는 방식으로 '리뷰 상세 내용' 과 같은 부분을 재사용할 수 있을 것 같습니다.

🔥 어떻게 해결했나요 ?

커밋이 중간에 꼬여서 순서가 좀 다를 수 있는데 해결 방법은 커밋 참고 부탁합니다.

📝 어떤 부분에 집중해서 리뷰해야 할까요?

각 서비스 네이밍과 메서드명이 적절한지 봐주세요
엔티티를 생성하고, 검증하는 부분이 괜찮은지, 변경에 유연한지 봐주세요.

📚 참고 자료, 할 말

사실 해당 이슈는 이전에 진행되었어야 했으나, 머지할 이슈들이 많이 남아 기다리고 있었습니다.
그러던 중 우리 팀에 맞는 도메인 설계는 무엇일까? 라는 글을 작성하며 리팩터링에 대한 감을 잡고 진행하게 되었습니다.

우리는 확장에 유리한 구조를 위해 객체 참조를 끊는 방식들을 사용했지만, 그런 이점들을 많이 사용하고 있지 않은 것 같았습니다.
도메인 간에 협력과 같은 로직은 존재하지 않고, 비즈니스 로직은 DTO에 너무나도 의존적이면서 객체 참조를 끊었을 때의 단점인 연관된 객체를 필요로 할 때마다 별도의 쿼리를 통해 조회해야 하기 때문에 성능 저하가 발생할 수 있다, 데이터 정합성을 유지하기 위한 추가적인 로직이 필요할 수 있다.와 같은 부분들만 가져가고 있는 것 같았습니다.

따라서 단점을 가져가면서도 우리가 설정한 목표에 맞게 어느 정도 서비스를 리팩터링할 필요성을 느꼈습니다.
짧은 시간 동안 적용하려다 보니 좀 급하게 된 감이 있을 것 같네요.. 부족한 부분이나 좀 더 도메인 로직이 추가될 수 있는 부분이 있다면 의견 부탁드립니다.

양이 많아서 리뷰 천천히 해주셔도 됩니다.

구현하면서 중간에 develop 병합해서 산스쳐를 사용해봤는데요. 정말 편하네요 👍👍

- 서비스명 변경 : CreateReviewService → ReviewRegisterService

…factor/384-2-service-refactor

github-actions · 2024-08-29T03:58:00Z

Test Results

91 tests +8 91 ✅ +8 4s ⏱️ -1s
31 suites +6 0 💤 ±0
31 files +6 0 ❌ ±0

Results for commit 610b785. ± Comparison against base commit 6099b31.

This pull request removes 23 and adds 31 tests. Note that renamed tests count towards both.

reviewme.review.service.CreateCheckBoxAnswerRequestValidatorTest ‑ 선택형_질문에_텍스트_응답을_하면_예외가_발생한다()
reviewme.review.service.CreateCheckBoxAnswerRequestValidatorTest ‑ 옵션그룹에서_정한_최대_선택_수_보다_많이_선택하면_예외가_발생한다()
reviewme.review.service.CreateCheckBoxAnswerRequestValidatorTest ‑ 옵션그룹에서_정한_최소_선택_수_보다_적게_선택하면_예외가_발생한다()
reviewme.review.service.CreateCheckBoxAnswerRequestValidatorTest ‑ 옵션그룹에서_제공하지_않은_옵션아이템을_응답하면_예외가_발생한다()
reviewme.review.service.CreateCheckBoxAnswerRequestValidatorTest ‑ 응답한_질문과_대응하는_옵션그룹이_존재하지_않으면_예외가_발생한다()
reviewme.review.service.CreateCheckBoxAnswerRequestValidatorTest ‑ 저장되지_않은_질문에_대한_응답이면_예외가_발생한다()
reviewme.review.service.CreateCheckBoxAnswerRequestValidatorTest ‑ 필수_선택형_질문에_응답을_하지_않으면_예외가_발생한다()
reviewme.review.service.CreateReviewServiceTest ‑ 적정_글자수인_텍스트_응답인_경우_정상_저장된다()
reviewme.review.service.CreateReviewServiceTest ‑ 체크박스가_포함된_리뷰를_저장한다()
reviewme.review.service.CreateReviewServiceTest ‑ 텍스트가_포함된_리뷰를_저장한다()
…

reviewme.review.domain.CheckboxAnswerTest ‑ 답변이_없는_경우_예외를_발생한다()
reviewme.review.domain.ReviewTest ‑ 리뷰에_등록된_답변의_모든_질문들을_반환한다()
reviewme.review.domain.ReviewTest ‑ 리뷰에_등록된_모든_선택형_답변의_옵션들을_반환환다()
reviewme.review.domain.ReviewTest ‑ 리뷰에_특정_질문에_대한_답변이_있는지_여부를_반환한다()
reviewme.review.domain.TextAnswerTest ‑ 답변이_없는_경우_예외를_발생한다()
reviewme.review.service.ReviewListLookupServiceTest ‑ 그룹_액세스_코드가_일치하지_않는_경우_예외가_발생한다()
reviewme.review.service.ReviewListLookupServiceTest ‑ 리뷰_요청_코드가_존재하지_않는_경우_예외가_발생한다()
reviewme.review.service.ReviewListLookupServiceTest ‑ 확인_코드에_해당하는_그룹이_존재하면_내가_받은_리뷰_목록을_반환한다()
reviewme.review.service.ReviewRegisterServiceTest ‑ 요청한_내용으로_리뷰를_등록한다()
reviewme.review.service.module.AnswerMapperTest ‑ 답변_요청을_서술형_답변으로_매핑한다()
…

♻️ This comment has been updated with latest results.

donghoony

일단 RC를 드립니다. 파일 변경 사항이 많아서 리뷰도 엄청 많네요 😓

리뷰를 생성하면서 검증할 때 DB를 너무 많이 찌르고 있다고 생각해요. 어떻게 줄이면 좋을까요...
질문/답변에 대한 추상화를 진행해보고 싶다는 생각이 드네요. if/else 밉다..

각각 질문에 대한 답변 적어주시면 같이 생각해볼게요

donghoony · 2024-08-29T17:03:32Z

backend/src/main/java/reviewme/review/service/ReviewRegisterService.java

+    private final ReviewMapper reviewMapper;
+    private final ReviewValidator reviewValidator;
+    private final TextAnswerValidator textAnswerValidator;
+    private final CheckBoxAnswerValidator checkBoxAnswerValidator;


validator가 mapper에서만 사용된다면, mapper 필드에 두어도 괜찮겠습니다

mapper와 validator의 책임을 완전히 분리해야 된다고 생각합니다.
mapper 에서는 검증 없이 단순히 매핑만 하도록 하고(리뷰와 답변 모두 생성만),
validator에서는 비즈니즈 정책에 따른 검증만 하도록 변경했습니다.(리뷰와 답번 검증 진행)

이 두 로직이 서비스 단에서 나눠져서 보인다면 깊게 접근하지 않아도 변경할 부분을 쉽게 찾을 수 있을 것 같아요😊

donghoony · 2024-08-29T17:04:30Z

backend/src/main/java/reviewme/review/service/ReviewRegisterService.java

+    public long registerReview(CreateReviewRequest request) {
+        Review review = reviewMapper.mapToReview(request, textAnswerValidator, checkBoxAnswerValidator);
+        reviewValidator.validate(review);
+        Review registeredReview = reviewRepository.save(review);


위 코멘트와 같은 맥락으로, mapper가 매핑한 리뷰를 검증된 리뷰라고 바라보는 것도 좋겠네요. 추상화 측면에서도 리뷰를 등록하는 것이니, 검증의 책임을 아래로 내리는 것이 자연스럽게 느껴져요

위 코멘트에서 함께 설명드렸습니다!

donghoony · 2024-08-29T17:06:39Z

backend/src/main/java/reviewme/review/service/module/TextAnswerValidator.java

+    }
+
+    private static void validateAnswerExist(TextAnswer textAnswer) {
+        if (textAnswer.getContent() == null) {


Suggested change

if (textAnswer.getContent() == null) {

if (textAnswer.hasNoContent()) {

이 부분은 수정되면서 없어진 코드네요..😢

donghoony · 2024-08-29T17:07:44Z

backend/src/main/java/reviewme/review/service/module/TextAnswerValidator.java

+
+    private void validateLength(TextAnswer textAnswer) {
+        int answerLength = textAnswer.getContent().length();
+        if (answerLength < MIN_LENGTH || answerLength > MAX_LENGTH) {


순서가 코드에서는 보장돼있긴 한데, 실제로 해당 메서드에 content == null인 게 들어오면 의도대로 동작하지 않을 듯해요. 그냥 두는 게 좋을까요?

모듈을 엄청나게 나눈 상태에서 '호출되는 순서에 관계 없이 항상 똑같이 작동하게' 하려면 중복되는 코드가 많을 것 같아요. 그런데 한편으로는 외부에서 어떤 순서로 호출되는지에 따라서 맡기는 것은 변화에 유연한 설계가 아니니... 트레이드오프를 해야겠네요😵‍💫

개인적으로는 모듈을 나누겠다는 선택을 했고, 이게 재활용되는 것이 아니니 절차지향적으로 생각해도되지 않나~ 싶습니다!

=> 이미 절차지향적이니 절차지향적으로 가도 괜찮겠다는 의미!

이 부분은 엔티티에 nullable = false 로 정의되어 있으니, 생성자에서 검증하는 것이 맞을 것 같아요. 엔티티에 null 값이 들어가지 못하도록 생성자에 검증 로직을 추가했습니다.

생성자에 검증로직 추가한 것 좋네요~

donghoony · 2024-08-29T17:09:02Z

backend/src/main/java/reviewme/review/domain/Review.java

@@ -55,6 +58,20 @@ public Review(long templateId, long reviewGroupId,
        this.createdAt = LocalDateTime.now();
    }

+    public Set<Long> getAllQuestionIdsFromAnswers() {


제안) getAnsweredQuestionIds

반영 완료👍

donghoony · 2024-08-29T17:25:27Z

backend/src/main/java/reviewme/review/service/module/ReviewDetailMapper.java

+    private QuestionAnswerResponse mapToTextQuestionResponse(Review review, ReviewGroup reviewGroup,
+                                         Question question) {


개행 맞춰주세요! 👀

반영 완료👍

donghoony · 2024-08-29T17:26:29Z

backend/src/main/java/reviewme/review/service/module/ReviewDetailMapper.java

+
+    private QuestionAnswerResponse mapToCheckboxQuestionResponse(Review review, ReviewGroup reviewGroup,
+                                                                 Question question) {
+        OptionGroup optionGroup = optionGroupRepository.findByQuestionId(question.getId())


이쪽도 각각의 OptionGroup마다 쿼리가 나가겠네요! (머리가 지끈지끈)

이 부분 DB 접근을 최소화 하면서 매핑하도록 리팩토링하려 하는데 파일 변경이 쫌 생길 것 같아서 따로 이슈 파서 진행하도록 하겠습니다 (해당 PR과 다른 내용이 될 수 있을 것 같아요)

donghoony · 2024-08-29T17:28:44Z

backend/src/main/java/reviewme/review/service/module/ReviewDetailMapper.java

@@ -38,6 +38,7 @@ public ReviewDetailResponse mapToReviewDetailResponse(Review review, ReviewGroup
        List<Section> sections = sectionRepository.findAllByTemplateId(templateId);
        List<SectionAnswerResponse> sectionResponses = sections.stream()
                .map(section -> mapToSectionResponse(review, reviewGroup, section))
+                .filter(sectionResponse -> !sectionResponse.questions().isEmpty())


Dto라도 이런 질문은 던져도 괜찮다고 생각해요~!~!

만들어진 sectionResponse의 질문들이 없는 경우를 거르고 있는데 이 부분은 왜 필요한 건지 궁금해요!

@donghoony 반영 완료👍
@skylar1220 질문이 있는 섹션만 보여주기 위함입니다

donghoony · 2024-08-29T17:28:57Z

backend/src/main/java/reviewme/review/service/module/ReviewMapper.java

+        if (answerRequest.text() == null) {
+            throw new QuestionNotAnsweredException(answerRequest.questionId());
+        }
+


donghoony · 2024-08-29T17:29:43Z

backend/src/main/java/reviewme/review/service/module/TextAnswerValidator.java

    public void validate(TextAnswer textAnswer) {
-        validateAnswerExist(textAnswer);
+        validateExistQuestion(textAnswer);
        validateLength(textAnswer);
    }

-    private static void validateAnswerExist(TextAnswer textAnswer) {
-        if (textAnswer.getContent() == null) {
-            throw new QuestionNotAnsweredException(textAnswer.getId());
+    private void validateExistQuestion(TextAnswer textAnswer) {
+        if (!questionRepository.existsById(textAnswer.getQuestionId())) {
+            throw new SubmittedQuestionNotFoundException(textAnswer.getQuestionId());
        }


여기도 어렵네요,, 각 서술형 답변마다 다시 DB를 찌르게 될 듯해요. 😨

위에 말씀드린 DB 접근 최소화 리팩토링에 함께 진행하겠습니다~

skylar1220

방학동안 구조에 대해 고민을 많이 했겠네요 테드 짱...
확실히 Mapper가 깔끔하게 묶어주는 부분의 장점을 느꼈어요, 모듈 방식도 좋음(조회시!)👍
RC 남긴 부분에 대해서 함께 얘기 더 나눠봐요!!

skylar1220 · 2024-09-02T20:47:52Z

backend/src/main/java/reviewme/review/service/ReviewDetailLookupService.java

+        ReviewGroup reviewGroup = findReviewGroupByRequestCodeOrThrow(reviewRequestCode);
+        validateGroupAccessCode(groupAccessCode, reviewGroup);


mapper로 상세한 구현이 다 숨겨져서 validate도 짧지만 메서드로 빼는 게 깔끔하네요👍

skylar1220 · 2024-09-02T21:08:15Z

backend/src/main/java/reviewme/review/service/module/ReviewDetailMapper.java

@@ -38,6 +38,7 @@ public ReviewDetailResponse mapToReviewDetailResponse(Review review, ReviewGroup
        List<Section> sections = sectionRepository.findAllByTemplateId(templateId);
        List<SectionAnswerResponse> sectionResponses = sections.stream()
                .map(section -> mapToSectionResponse(review, reviewGroup, section))
+                .filter(sectionResponse -> !sectionResponse.questions().isEmpty())


만들어진 sectionResponse의 질문들이 없는 경우를 거르고 있는데 이 부분은 왜 필요한 건지 궁금해요!

skylar1220 · 2024-09-02T21:19:41Z

backend/src/main/java/reviewme/template/service/module/TemplateMapper.java

@@ -28,12 +30,20 @@
 @RequiredArgsConstructor
 public class TemplateMapper {

+    public static final String REVIEWEE_NAME_PLACEHOLDER = "{revieweeName}";


ReviewDetailMapper에도 REVIEWEE_NAME_PLACEHOLDER가 똑같이 상수로 선언되어있는데
이 두 부분이 모두 dataInitializer를 통해 초기에 기본적으로 들어가있는 "{revieweeName}" 문자열을 뜻하고 있네요.
그렇다면 변경이 생긴다면 두 mapper를 모두 바꿔주어야하는데 어떻게 하면 좋을까요..!

이 부분은 View 와 관련된 로직이라 사실 convertHeader가 도메인 기능에 있는 것 부터 다시 생각해 봐야 할 것 같아요. 일단 리팩토링 사항으로 남겨두겠습니다!

이건 #545 를 생각하고 넘어가도 되겠습니다

skylar1220 · 2024-09-02T21:30:30Z

backend/src/main/java/reviewme/review/service/ReviewDetailLookupService.java

+    private ReviewGroup findReviewGroupByRequestCodeOrThrow(String reviewRequestCode) {
+        return reviewGroupRepository.findByReviewRequestCode(reviewRequestCode)
+                .orElseThrow(() -> new ReviewGroupNotFoundByReviewRequestCodeException(reviewRequestCode));
    }


findReviewGroupByRequestCodeOrThrow 이 메서드가 여러 서비스, mapper에서 중복되게 선언되고 있고 던지고 있는 예외도 동일해요.
reviewerGroup에 requestCode가 컬럼으로 있으니 정책적인 예외가 아니라 reviewerGroup의 데이터 정합성 문제 상황이라고 보고, repo선에서 검증하는 것에 대해서 어떻게 생각하시나요~~?

음 저번에 repo 단에서 예외를 처리하지 않기로 했던 것 같아서 밖으로 뺐는데요, 이 부분 뿐만 아니라 다른 부분도 검토하고 통일하는 게 맞을 것 같습니다!

skylar1220 · 2024-09-02T21:50:59Z

backend/src/main/java/reviewme/review/service/module/ReviewMapper.java

+            if (question.getQuestionType() == QuestionType.TEXT) {
+                TextAnswer textAnswer = mapToTextAnswer(answerRequest);
+                textAnswerValidator.validate(textAnswer);
+                textAnswers.add(textAnswer);
+            }


textAnswer를 mapper로 생성함(null 검증은 mapper안에서 함) → 그 다음에 validator로 보내서 나머지 검증함

현재 검증이 1. mapper 2. validator 두 위치에 나눠져있는 부분이 고민됩니다.

현재는 textAnswer 객체를 생성하기 전에 해야하는 검증이 null만 있지만,
이전 로직을 비교해보면 아래와 같은 검증들도 textAnswer 객체를 생성하기 전에 mapper에서 이뤄져야하기에
그렇다면 검증이 더더욱 1. mapper 2. validator 이 두 부분으로 나눠지게 된다고 생각해요.

private void validateNotIncludingOptions(CreateReviewAnswerRequest request) { if (request.selectedOptionIds() != null) { throw new TextAnswerIncludedOptionItemException(); } } private void validateQuestionRequired(Question question, CreateReviewAnswerRequest request) { if (question.isRequired() && request.text() == null) { throw new RequiredQuestionNotAnsweredException(question.getId()); } }

객체 생성 전에 한 곳에서 검증을 싹 하고, 후에 객체를 생성하는 방식으로 풀어갈 수 있지 않을까요?(아직 안해봄.. 생각대로 될지는 해봐야합니다😂)

기존의 방식은 DTO 단계에서 모든 검증이 수행되기 때문에 단순화될 수 있지만, DTO에 너무 의존적인 서비스가 되어버리는 것 같았습니다. DTO가 변경되면 DTO를 검증하는 로직을 변경해야 하므로 서비스의 유연성이 떨어지게 됩니다.
변경된 방식은 변환, 검증과 같은 로직이 각각 책임에 따라 분리되어 있어 수정이 발생하는 부분만 수정할 수 있습니다.
따라서 DTO가 변경된다면 변환 부분의 로직만 수정되고 엔티티는 변경되지 않으니 검증하는 로직은 수정이 필요 없게 됩니다.

추가로 이 부분을 읽어보았는데요.
요청 DTO 변경되어서 DTO를 검증하는 로직을 변경해야하는 경우가 떠오르지 않아서 위와 같은 코멘트를 남겼어요!
(객체 생성 전에 한 곳에서 검증을 싹 하고, 후에 객체를 생성하는 방식 = dto로 검증을 하자)

저 장점을 가져가기엔 떠오르는 변경으로 인해 검증 로직에 변경이 생기는 것이 떠오르는 게 현재 시점에서 없고(발생한 적이 없고)

하지만 객체 생성후 검증 방식으로 한다면 검증 위치가 여러곳으로 분리되고 유지보수시 파악이 어렵겠다는 이유라고 정리할 수 있겠네요!

같이 대화 나눈 내용 코멘트로 남겨놓겠습니다.

Mapper와 Validator는 각각의 책임을 지님

Mapper는 요청을 받아 객체를 매핑하는 책임 -> 이 과정에서 요청이 올바르게 들어왔는지에 대한 검증을 같이 진행

Validator는 생성된 객체를 비즈니스 정책에 따라 검증

수정이 필요한 부분이 요청과 관련된 부분인지, 비즈니스 정책에 관련된 부분인지에 따라서 개발자가 봐야 하는 부분을 명확하게 해보자

skylar1220 · 2024-09-02T21:55:50Z

backend/src/main/java/reviewme/review/service/module/ReviewDetailMapper.java

detail, listLookUp과 같은 조회를 위한 응답 객체 생성에 mapper 방식 좋네요!👍👍
응답 객체를 생성한다는 책임이 분리되어 서비스는 좀 더 흐름적인 부분에 집중, mapper 객체는 repo와 교류하며 응답생성. 이렇게 응집도가 올라가네요!

다만 리뷰 생성 과정에서 review 객체 생성에 mapper를 사용하는 부분은 고민이 돼요. 아래에 남긴 textAnswer 코멘트와 연관되어있습니다!

👍👍👍👌

nayonsoso

우선, 서비스 코드에 대해서 깊이 고민하셨군요!!! 증말 대단하고 감사합니다🥹
서비스나 dto 이름 변경을 한 쪽이 훨씬 가독성이 좋았습니다.
그리고 서비스에서 모듈을 호출하는 부분도 읽기 편했습니다.
그래서 분량은 많더라도 잘 따라가면서 읽을 수 있었습니다👍

저는 이런식으로 서비스가 모듈을 호출하게 하는 구조로 가도 좋을 것 같습니다.
다소 절차지향적이긴 하지만, ‘읽기 편하고 유지보수 쉬운 절차지향 코드’이니 이걸로 충분하죠!

개항이나 논리적 개행들은 맞추는게 좋을 것 같아 RC 드립니다!
(다만, 지금은 서비스 하위에 모듈이 있는 구조같은데, 지금과 같은 service, module 패키지 구조가 맞는건지는 더 고민해봐야겠네요. 그런데 전반적인 코드는 동의합니다👌)

nayonsoso · 2024-09-03T11:07:45Z

backend/src/main/java/reviewme/review/service/ReviewListLookupService.java

+                .orElseThrow(() -> new ReviewGroupNotFoundByReviewRequestCodeException(reviewRequestCode));
+    }
+
+    private static void validateGroupAccessCode(String groupAccessCode, ReviewGroup reviewGroup) {


@donghoony 저는 둘 다 괜찮다 주의입니다..!

nayonsoso · 2024-09-03T11:19:41Z

backend/src/main/java/reviewme/review/service/dto/request/ReviewRegisterRequest.java

@@ -4,12 +4,12 @@
 import jakarta.validation.constraints.NotEmpty;
 import java.util.List;

-public record CreateReviewRequest(
+public record ReviewRegisterRequest(


전반적으로 바꾼 이름이 더 이해하기 쉬운 것 같습니다👍
잘 바꾸셨네요!

nayonsoso · 2024-09-03T11:25:49Z

backend/src/main/java/reviewme/review/service/module/TextAnswerValidator.java

+
+    private void validateLength(TextAnswer textAnswer) {
+        int answerLength = textAnswer.getContent().length();
+        if (answerLength < MIN_LENGTH || answerLength > MAX_LENGTH) {


모듈을 엄청나게 나눈 상태에서 '호출되는 순서에 관계 없이 항상 똑같이 작동하게' 하려면 중복되는 코드가 많을 것 같아요. 그런데 한편으로는 외부에서 어떤 순서로 호출되는지에 따라서 맡기는 것은 변화에 유연한 설계가 아니니... 트레이드오프를 해야겠네요😵‍💫

개인적으로는 모듈을 나누겠다는 선택을 했고, 이게 재활용되는 것이 아니니 절차지향적으로 생각해도되지 않나~ 싶습니다!

=> 이미 절차지향적이니 절차지향적으로 가도 괜찮겠다는 의미!

nayonsoso · 2024-09-03T11:42:20Z

backend/src/main/java/reviewme/review/service/module/ReviewMapper.java

+            Question question = questionRepository.findById(answerRequest.questionId())
+                    .orElseThrow(() -> new SubmittedQuestionNotFoundException(answerRequest.questionId()));


저도 같은 생각입니다

nayonsoso · 2024-09-03T11:45:06Z

backend/src/test/java/reviewme/review/service/module/CheckBoxAnswerValidatorTest.java

+//    @Test
+//    void 필수_선택형_질문에_응답을_하지_않으면_예외가_발생한다() {
+//        // given
+//        optionGroupRepository.save(optionGroupFixture.선택지_그룹(savedQuestion.getId()));
+//        CheckboxAnswer checkboxAnswer = new CheckboxAnswer(savedQuestion.getId(), null);
+//
+//        // when, then
+//        assertThatCode(() -> checkBoxAnswerValidator.validate(checkboxAnswer))


여긴 왜 주석하셨나요?

하핫.. 지웠습니다😊

nayonsoso · 2024-09-03T11:50:39Z

backend/src/main/java/reviewme/review/service/exception/QuestionNotAnsweredException.java

+public class QuestionNotAnsweredException extends BadRequestException {
+
+    public QuestionNotAnsweredException(long questionId) {
+        super("질문에 대한 답변을 작성하지 않았어요.");
+        log.warn("question must be answered - questionId: {}", questionId, this);
+    }
+}


이 예외가 세군데에서 던져지는데, 순서상 중복이 됩니다.
의도하신게 아니라면, ReviewValidator 에서 제거를 하고
TextAnswerValidator 과 CheckBoxAnswerValidator에만 두어도 될 것 같아요!

ReviewMapper에서 하던 null 검증을 객체 생성자에서 하는 것으로 변경하면서 객체 내부에서 던져지게 됩니다.

Validator에서는 isEmpty()에 대한 검증을 하는데요. 다른 예외를 사용하지 않아도 될 것 같은데.. null 검증과 isEmpty 검증을 객체 생성자에서 한번에 하는게 나을까요?

아하, 지금은

TextAnswer 에서 내용물이 null 일 때

CheckboxAnswer 에서 선택된 옵션 아이디 리스트가가 null 일 때

CheckbosAnswerValidator 에서 선택된 옵션 아이디 리스트가 empty 일 때 발생하는군요~

코드를 봤는데, CheckboxAnswer의 selectedOptionIds가 null 인 것은 당연히 안되고, empty 인 답변도 DB에 저장되면 안되니 엔티티 단에서 생성자에서 한번에 하는게 깔끔한 것 같아요~ 그러고 만약 그렇게 된다면, 이 예외 패키지를 service 아래의 ewxception 이 아니라 domain 하위의 exception 패키지로 바꿔주세요!

…nIds)

nayonsoso

아래에 적은 CheckboxAnswers / TextAnswers 관련 내용 외에는 더 고칠 곳 없을 것 같습니다~
사실 안쓰는 예외가 있긴것 같긴 한데 다음 리팩터링(예외 클래스 점검)에서 고치면 될 것 같아요!
우선 서비스 리팩토링은 잘 된듯 하여 approve 드립니다.
수고하셨습니다🤩

nayonsoso · 2024-09-05T06:36:27Z

backend/src/main/java/reviewme/review/service/module/ReviewDetailMapper.java

+        TextAnswers textAnswers = new TextAnswers(review.getTextAnswers());
+        TextAnswer textAnswer = textAnswers.getAnswerByQuestionId(question.getId());


TextAnswers가 하는 역할이 questionId 가 주어졌을 때 request 에서 이에 해당하는 답변이 있는지 검증하는 것이었잖아요?

이걸 TextAnswerValidator 가 담당하고 있으니,

조회할 때는 없어도 될 것 같아서

TextAnswers 와 CheckboxAnswers 를 사용할 이유가 크게 없다면
(실제로 지금 CheckboxAnswers는 테스트 코드에서만 사용되고 있습니다)
삭제해도 될 것 같습니다.
다만 지금 바꾸기에는 더 라인 변경이 많아질 것 같아서
다음 리팩터링에서 잊지 않게 이슈로 만들어주었습니다.

#571

(테드와 오프라인으로 대화를 나눈 후)
TextAnswers / CheckboxAnswers 가 'DB에 쿼리를 덜 날리게 하는 기능도 한다'는 이야기가 나왔습니다.
정말 그 기능이 유용하게 쓰이고 있는지? 반드시 저 객체들이 필요한지에 대해서 여전히 의문이 들었지만,
무튼 다음 리팩터링으로 미루기로 했습니다.

skylar1220

추가로 남긴 코멘트에 대해 대화로 해결방법까지 합의가 되어 어프루브합니다!
(테드가 잘 반영해주겠지~~)

skylar1220 · 2024-09-05T10:22:49Z

backend/src/main/java/reviewme/review/service/module/ReviewDetailMapper.java

+        List<Section> sections = sectionRepository.findAllByTemplateId(templateId);
+        List<SectionAnswerResponse> sectionResponses = sections.stream()
+                .map(section -> mapToSectionResponse(review, reviewGroup, section))
+                .filter(SectionAnswerResponse::hasQuestions)


filter의 역할을 정리하면 이렇게인데.
'선택하지 않은 섹션도 섹션 응답 객체로 매핑은 함. 하지만, 섹션 응답 객체 안에 응답된 질문이 없을테니, 그걸 거름'

그럼 해당 맥락으로는 hasQuestions보다는 hasAnswer, hasAnsweredQuestion 같은 메서드명이 더 잘 이해될 것 같습니다!

donghoony

몇가지 수정사항만 제안하고 어프룹합니다 최고다~~~

donghoony · 2024-09-06T01:30:16Z

backend/src/main/java/reviewme/review/service/ReviewRegisterService.java

-    private final CheckBoxAnswerValidator checkBoxAnswerValidator;
+



개행 두 줄이네요 👀

donghoony · 2024-09-06T01:30:38Z

backend/src/main/java/reviewme/review/service/module/ReviewMapper.java

+    private final AnswerMapper answerMapper;
+


이쪽은 맥락을 나누기 위해 개행했나요? 필드가 많지 않아서 그냥 붙여도 된다는 의견입니다.

donghoony · 2024-09-06T01:32:12Z

backend/src/main/java/reviewme/review/service/module/ReviewValidator.java

+        validateAnswer(review);
        validateAllAnswersContainedInTemplate(review);
        validateAllRequiredQuestionsAnswered(review);


이제 봤네요. 리뷰 전체를 넘기기보다는 필요한 파라미터만 넘겨주는 건 어떨까요?

donghoony · 2024-09-06T01:33:00Z

backend/src/main/java/reviewme/review/service/module/ReviewDetailMapper.java

@@ -53,7 +53,7 @@ public ReviewDetailResponse mapToReviewDetailResponse(Review review, ReviewGroup
    private SectionAnswerResponse mapToSectionResponse(Review review, ReviewGroup reviewGroup, Section section) {
        List<QuestionAnswerResponse> questionResponses = questionRepository.findAllBySectionId(section.getId())
                .stream()
-                .filter(question -> review.getAllQuestionIdsFromAnswers().contains(question.getId()))
+                .filter(question -> review.getAnsweredQuestionIds().contains(question.getId()))


객체에게 물어볼 수 있지 않을까? 라는 생각입니다.
review.hasAnsweredQuestionId()

커밋 따라가면서 리뷰했는데 뒤쪽에서 반영됐네요 👍🏻👍🏻👍🏻👍🏻👍🏻👍🏻👍🏻

…Exception을 domain 하위의 exception 패키지로 변경

…factor/384-2-service-refactor # Conflicts: # backend/src/test/java/reviewme/review/service/CreateReviewServiceTest.java # backend/src/test/java/reviewme/review/service/ReviewDetailLookupServiceTest.java # backend/src/test/java/reviewme/review/service/ReviewListLookupServiceTest.java # backend/src/test/java/reviewme/review/service/module/CheckBoxAnswerValidatorTest.java

Kimprodp added 26 commits August 28, 2024 23:38

feat: 리뷰를 검증하는 서비스 구현

1e6204b

refactor: 리뷰 등록 서비스가 리뷰를 생성하고 검증 서비스를 통해서 검증하도록 변경

d62a842

- 서비스명 변경 : CreateReviewService → ReviewRegisterService

refactor: 답변 검증 서비스가 요청이 아닌 객체를 검증하도록 변경

de03539

feat: 리뷰에서 제공하는 기능 추가

2fc94e1

refactor: dto명 변경 및 사용하지 않는 메서드 삭제

7561c72

refactor: 예외명 및 메세지 변경

d3842d2

refactor: 리뷰 목록 조회 서비스명 변경 및 응답 객체 mapper를 활용하도록 변경

3cfe514

feat: 리뷰 목록 응답 객체 매핑 서비스 추가

daecbdc

refactor: dto명 변경으로 인한 수정

9cc53a9

refactor: ReviewMapper를 통해 리뷰를 생성하도록 변경

b60807f

refactor: 리뷰 상세 응답 mapper를 활용하도록 변경 및 일부 로직 리팩터링

ebc281c

refactor: 템플릿을 찾는 로직을 mapper에서 하도록 변경

e42293c

refactor: 리팩터링에 따른 컨트롤러 변경사항

357e9ef

Merge remote-tracking branch 'refs/remotes/origin/develop' into be/re…

bb153e5

…factor/384-2-service-refactor

test: 변경사항 반영

646d8b7

refactor: 질문이 없는 섹션은 응답 생성하지 않도록 변경

20d6ffc

refactor: 답변이 null인 경우에 대한 예외 추가

bd4b455

refactor: CheckBoxAnswerValidator null 검증 제외 및 질문 유효 검증 추가

fd2f730

refactor: TextAnswerValidator 질문 유효 검증 추가

14e1819

test: Validator 테스트 작성

76a01c7

test: Mapper 테스트 작성

08e5707

test: ReviewValidator 테스트 작성

7e15fa3

test: ReviewRegisterService 테스트 간소화

8726e0c

test: 리팩터링 사항 반영

cfe4fd9

test: Review 테스트 작성

c351eb2

chore: secret 변경사항 반영

4ff1bb4

Kimprodp added ✨ BE 🛠️ 리팩터링 🫠 힘들다... 💪 아자아자!! labels Aug 29, 2024

donghoony requested changes Aug 29, 2024

View reviewed changes

skylar1220 requested changes Sep 2, 2024

View reviewed changes

nayonsoso requested changes Sep 3, 2024

View reviewed changes

Kimprodp added 13 commits September 4, 2024 23:14

refactor: 답변 생성 시, null 이 들어올 수 없도록 변경

f494164

refactor: ReviewMapper 에서 답변에 대한 검증 없이 엔티티 매핑만 하도록 변경

b0e20bc

refactor: ReviewValidator 에서 답변에 대한 검증도 진행하도록 변경

0d63f18

refactor: 메서드명 변경 (getAllQuestionIdsFromAnswers -> getAnsweredQuestio…

d93f943

…nIds)

refactor: @transactional 어노테이션을 클래스 범위에만 적용하도록 변경

6363b82

refactor: 메서드 분리 변경

0062fb4

refactor: 메서드 시그니쳐 변경

1359b10

style : 개행 수정

8caa2b0

refactor: 존재하는 메서드 활용하도록 변경

b90aa47

refactor: 리뷰에 특적 질문에 대한 답변이 있는지 확인하는 기능 추가

1f18e77

style: 개행 수정

0071013

refactor: 객체 메서드를 활용하도록 변경

7022bcb

test: AnswerMapperTest 작성

c280809

nayonsoso approved these changes Sep 5, 2024

View reviewed changes

This was referenced Sep 5, 2024

[BE] 불필요하게 DB 를 조회하는 로직을 개선한다. #571

Closed

[BE] 요청에 대한 검증은 spring bean validation 을 사용해서 dto 에서 수행하게 한다. #572

Closed

skylar1220 approved these changes Sep 6, 2024

View reviewed changes

donghoony approved these changes Sep 6, 2024

View reviewed changes

Kimprodp added 6 commits September 7, 2024 10:41

refactor: 답변 검증 시, 답변이 빈 경우에 대한 검증도 객체 생성자에서 진행 및 QuestionNotAnswered…

d9b4519

…Exception을 domain 하위의 exception 패키지로 변경

refactor: 메서드명 변경 (hasQuestions -> hasAnsweredQuestion)

caa0716

style: 개행 변경

eca9491

refactor: ReviewValidator 파라미터를 나눠보 보낼 수 있는 부분 적용

591f8cb

test: develop 브랜치 머치 후, 미변경 사항 적용

610b785

Kimprodp merged commit ae477fc into develop Sep 7, 2024
2 checks passed

donghoony deleted the be/refactor/384-2-service-refactor branch September 26, 2024 02:46

	if (textAnswer.getContent() == null) {
	if (textAnswer.hasNoContent()) {

		private QuestionAnswerResponse mapToTextQuestionResponse(Review review, ReviewGroup reviewGroup,
		Question question) {

		ReviewGroup reviewGroup = findReviewGroupByRequestCodeOrThrow(reviewRequestCode);
		validateGroupAccessCode(groupAccessCode, reviewGroup);

		Question question = questionRepository.findById(answerRequest.questionId())
		.orElseThrow(() -> new SubmittedQuestionNotFoundException(answerRequest.questionId()));

		TextAnswers textAnswers = new TextAnswers(review.getTextAnswers());
		TextAnswer textAnswer = textAnswers.getAnswerByQuestionId(question.getId());

		private final CheckBoxAnswerValidator checkBoxAnswerValidator;

[BE] refactor: 서비스 리팩터링 #540

[BE] refactor: 서비스 리팩터링 #540

Conversation

Kimprodp commented Aug 29, 2024 • edited Loading

🚀 어떤 기능을 구현했나요 ?

🔥 어떻게 해결했나요 ?

📝 어떤 부분에 집중해서 리뷰해야 할까요?

📚 참고 자료, 할 말

github-actions bot commented Aug 29, 2024 • edited Loading

Test Results

donghoony left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skylar1220 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nayonsoso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nayonsoso Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

nayonsoso left a comment • edited Loading

Choose a reason for hiding this comment

nayonsoso Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skylar1220 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donghoony left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kimprodp commented Aug 29, 2024 •

edited

Loading

github-actions bot commented Aug 29, 2024 •

edited

Loading

skylar1220 left a comment •

edited

Loading

nayonsoso Sep 5, 2024 •

edited

Loading

nayonsoso left a comment •

edited

Loading

nayonsoso Sep 5, 2024 •

edited

Loading