[BE] 피드백 서비스 리팩터링(#619)

[github-actions]

📌 관련 이슈

• closed : [BE] 피드백 서비스 리팩터링 #619

✨ PR 세부 내용

소셜 피드백 서비스를 리팩터링했습니다!

기본적으로 개발 피드백 서비스와 형식을 똑같이 하고, Reader, Writer를 분리하였습니다.

[github-actions]

Test Results

 57 files   57 suites   9s ⏱️
178 tests 174 ✅ 4 💤 0 ❌
185 runs  181 ✅ 4 💤 0 ❌

Results for commit bef4146.

♻️ This comment has been updated with latest results.

[ashsty]

안녕하세요 뽀로로~
지난 개발 피드백 서비스 리팩터링과 같은 결로 잘 정리된 것 같네요! 👍

크게 로직적으로 손댈 부분은 없을 것 같아 간단한 코멘트만 남겨보았어요.
고생하셨습니다~~!

[ashsty]

[질문]

지금 여기서 수행하던 validateAlreadyExist 체크가 SocialFeedbackWriter의 create 메서드 내부로 들어가면서

MatchResultWriter의 completeSocialFeedback() 메서드는 이미 피드백을 작성한 상태(=이미 boolean 값 true)더라도
무조건 실행될 것 같은데요. (이미 true인 값을 다시 한 번 true로 할당)

MatchResult 객체의 값을 create에서 사용하고 있기 때문에 이런 순서가 된 것 같아요.
크게 상관 없는 상황일까요? (이미 true인 값을 다시 한 번 true로 할당하게 됨.)

애초에 피드백 작성한 이후에서 클라이언트에서 다시 한 번 작성 메서드를 호출할 확률이 낮을 테니
크게 상관 없을 거 같긴 해요.

[jcoding-play]

MatchResult 객체의 값을 create에서 사용하고 있기 때문에 이런 순서가 된 것 같아요.

맞습니당.

그리고 저도 사실 애쉬가 생각하는 것처럼 validateAlreadyExist가 제일 먼저 호출되는게 흐름 상 맞다고 생각해요! 😀
다음처럼 약간의 변형을 주어 validateAlreadyExist가 먼저 호출되도록 검증할 수는 있겠습니다.

public MatchResult completeSocialFeedback(long roomId, long deliverId, long receiverId) {
        MatchResult matchResult = matchResultRepository.findByRoomIdAndReviewerIdAndRevieweeId(roomId, receiverId, deliverId)
                .orElseThrow(() -> new CoreaException(ExceptionType.NOT_MATCHED_MEMBER));

       // 여기에 matchResult의 feedback이 이미 complete인지?? 

        matchResult.revieweeCompleteFeedback();
        return matchResult;
    }

근데 저러한 코드는 혹여나 MatchResult의 필드 값을 처음에 true로 지정하는 휴먼에러가 발생할 수 있다고 생각이 들었어요.

---

MatchResultWriter의 completeSocialFeedback() 메서드는 이미 피드백을 작성한 상태(=이미 boolean 값 true)더라도
무조건 실행될 것 같은데요. (이미 true인 값을 다시 한 번 true로 할당)

그래서 만약 이미 피드백을 작성한 상태여서 true이더라도, 애쉬 말대로 completeSocialFeedback 메서드는 그대로 통과하겠지만
Feedback 엔티티에 이미 저장된 값이 있는지 확인하기 때문에 중복으로 생성될 문제는 없다 생각이 됩니다.

이미 true인 값을 다시 한 번 true로 할당하게 됨.

이 부분도 딱히 문제가 있을부분은 없을거라 생각해요~ 😀

[ashsty]

[질문] 오잉 저는 Input과 request, Output과 response의 형태가 달라지는 경우에만 분리하는 것으로 생각했는데 그게 아니었군요?
(파라미터라던가...)

이제 service에서 reader/writer에 넘겨주는 dto는 그 형태가 완벽하게 같더라도 무조건 input/output dto를 새로 만들어서 넘겨주어야 하는 건가요? ㅇ.ㅇ

[jcoding-play]

이제 service에서 reader/writer에 넘겨주는 dto는 그 형태가 완벽하게 같더라도 무조건 input/output dto를 새로 만들어서 넘겨주어야 하는 건가요?

넵 제가 생각한건 애쉬의 말이 맞습니다!
컨트롤러단에서 전달/응답받는 dto를 도메인 계층까지 내려주는게 어색하다 느껴졌어요!

근데 사실 좀 귀찮긴하죠..ㅎㅎ
저번에 제가 올린 pr에서 이 부분에 대해 얘기 나눠보고 싶었는데, 만나서 같이 한번 얘기 나눠보면 좋을 것 같아요!! 👍👍

[hjk0761]

분리 잘 된 것 같아용
구현하느라 고생많았어요~!

[youngsu5582]

코드적으로는 건드릴 부분 없고 두 가지 부분에 대해서만 의견을 남깁니다.

모든 Reader/Writer 가 update , create 구문이 있을 때 Mapper 를 만들어야 하는가?

에 대해서, 다소 부정적인 의견도 듭니다.
어차피, 재사용이 없고 중간 변환 단계만 하는데 필요한가? 라는 생각 떄문인데요.
방장이 참여를 하는 것과 사용자가 참여를 한다.
이렇게 두 가지가 있을 때 공통된 입력으로 변환하는건 괜찮은거 같으나 1:1 인데 변환은 굳이 인거 같습니다.

Reader / Writer 가 도메인을 가지게 작성을 해야하는가

현재, 코드에서 roomId,reviewerId 와 같이 단순히 변수를 넣어서 하는 로직이 있습니다.

• 리더와 라이터가 도메인을 가지는게 바람직한가?
• 재사용을 용이하게 하려면 ID 와 도메인중 뭐를 가져야 하는가

이에 대해서는 대면으로 간단하게 얘기해도 좋을거 같습니당.

[youngsu5582]

FeedbackMapper 가 꼭 static 일 필요가 있을까? 생각이 들긴 해요.
static 이라는건 전역적 으로도 사용이 가능하다는 의미도 같이 포함이 되는데
Feedback 이라는 부분에 한정되어 있는 도메인인 만큼 Component 로 관리해도 괜찮지 않을까 생각이 드는데 어떤가용

[jcoding-play]

좋습니다~
static 제거하고 필드로 추가할게요!~ 👍

[jcoding-play]

모든 Reader/Writer 가 update , create 구문이 있을 때 Mapper 를 만들어야 하는가?
에 대해서, 다소 부정적인 의견도 듭니다.
어차피, 재사용이 없고 중간 변환 단계만 하는데 필요한가? 라는 생각 떄문인데요.
방장이 참여를 하는 것과 사용자가 참여를 한다.
이렇게 두 가지가 있을 때 공통된 입력으로 변환하는건 괜찮은거 같으나 1:1 인데 변환은 굳이 인거 같습니다.

이 부분은 약간 트레이드 오프인듯 해요~!
조이썬 말대로 재사용이 없다면 변환 단계가 필요 없겠지만, 추후 비즈니스 요구 사항이 추가되어 도메인 필드가 추가된다면 도메인 계층의 dto만 수정해도 되기 때문에 장점도 있겠죠. 그리고 클라이언트가 요청/응답받는 dto를 도메인 계층까지 내리는게 어색하기도 해서 다음과 같은 변환 과정을 두었는데, 이 부분은 사실 저도 상당히 귀찮았기 때문에 저번에 pr에 이에 대한 의견을 물어본것이구요~! 이 부분에 대해서 같이 얘기하고 맞쳐보면 하네요~! 일단은 이번 pr에서는 유지해봅니당~ 😀