[BE] 쓴 피드백은 피드백 모아보기에서 바로 볼 수 있도록 변경 + 리팩터링(#613) #617

github-actions · 2024-10-17T03:08:52Z

📌 관련 이슈

closed : [BE] 쓴 피드백은 피드백 모아보기에서 바로 볼 수 있도록 변경 + 리팩터링 #613

✨ PR 세부 내용

쓴 피드백은 피드백 모아보기에서 바로 볼 수 있도록 수정하였습니다.

그리고 피드백 서비스 쪽에 대한 리팩터링 범위가 그리 크지 않을줄 알고 같이 작업을 했었는데,
생각보다 범위가 커서 일단 현재 작업한 부분까지만 PR 제출하고 나머지 피드백 서비스쪽 리팩터링 이어서 해볼게요.

현재 파일 변경 범위가 넓어 "쓴 피드백은 피드백 모아보기에서 바로 보기 기능"에 대한 변경 사항만 보고 싶으시다면
쓴 피드백 조회 기능 수정 여기를 통해 봐주세요.

P2: DTO 관리

Reader와 Writer의 역할을 분리하면서 RequestDto와 ResponseDto를 어디까지 전달할지 고민했고,
결국 Service 레이어를 넘어가지 않도록 하는 게 좋겠다고 판단했습니다.

그래서 어제 조이썬과 논의한 결과, Service와 Domain 간의 데이터 전송을 위해
RequestDto는 InputDto로, ResponseDto는 OutputDto로 변환하기로 했습니다.

그래서 각 DTO를 변환하는 작업이 필요했는데, 제가 구현한 방식은, 각 레이어의 DTO가 다른 레이어의 DTO에 의존하지 않도록
Mapper 클래스를 만들어 변환 작업을 관리하도록 했습니다.

이런 구조를 통해 특정 레이어의 DTO가 다른 레이어의 DTO에 의존하지 않도록 분리하려는 것이 목적이였습니다. 만약 서로 다른 레이어의 DTO가 의존하게 된다면, 작업은 편리해질 수 있지만, 레이어 간 의존성이 생겨 복잡도가 높아질 수 있다고 생각했습니다.

하지만 애기 나눠보고 싶은 점은 이런 방식이 유지보수와 확장성 측면에서 정말 최선인지 궁금합니다.
혹은 DTO 간 어느 정도의 의존성을 허용하여 개발 편의성을 높이는 방안도 고려할 수 있을 것 같은데, 다들 어떻게 생각하시나요?
Mapper 클래스를 이용한 관리 방식이 좋은지, 아니면 다른 대안이 있을지 자유롭게 의견을 나눠주시면 좋겠습니다!

github-actions · 2024-10-17T04:27:33Z

Test Results

55 files 55 suites 8s ⏱️
167 tests 163 ✅ 4 💤 0 ❌
174 runs 170 ✅ 4 💤 0 ❌

Results for commit 34540ff.

♻️ This comment has been updated with latest results.

hjk0761

로직 커밋 바로가기 해줘서 고마워요 ㅎ.ㅎ

File Change 는 많았지만 reader, writer 분리와 dto 네이밍 수정때문인지라 잘 되었는지 확인을 주로 했습니다.
고생 많았어요!

질문만 코멘트로 달고 로직에 문제 없는 것 같아 approve 드립니당!

아래는 P2 에 대한 생각과 추가 질문입니다.

질문

서비스-도메인 계층에서의 DTO 네이밍을 바꾼건 좋은 관점인 것 같아요.
Input, Output 이라는 네이밍을 사용하게 된 이유가 궁금해요.
콘솔 혹은 DB와 소통할 거라고 오해할 수 있는 네이밍으로도 느껴지긴 하네요.

DTO 변환 책임에 대해서

결국 Mapper 라는, 레이어와 레이어를 이어주는 친구가 생겨야 하는건 어쩔수 없겠지만
Mapper 로 DTO 간 의존성을 끊는 것이 좋은 것 같아요.

DTO 끼리 의존성이 생겼을 때, 기능이 확장됨에 따라 DTO 가 꼭 일대일로만 의존성이 생기지 않을 것 같아 걱정이 되는 것 같아요.
한 DTO 에서 여러 DTO 로 변환하게 되면 유지보수 관점에서 더 불편해 질거라는 생각이 들어요.

그렇다고 Service 계층에서 직접 DTO 를 변환하는건,,,

저는 Mapper 사용이 위 방법중엔 괜찮은 선택인 것 같습니다.
Mapper 를 분석하면서 DTO 의 변환을 통해 로직을 유추할 수 있는 것도 장점이 될 수 있을 것 같고요.

hjk0761 · 2024-10-17T10:59:44Z

backend/src/main/java/corea/feedback/service/UserFeedbackService.java

-    private UserFeedbackResponse getUserFeedbackResponse(Map<Long, List<FeedbackResponse>> developFeedback, Map<Long, List<FeedbackResponse>> socialFeedback) {
-        List<Room> rooms = roomRepository.findAllById(
-                extractDistinctKeyStreams(developFeedback, socialFeedback).toList());
+    private UserFeedbackResponse getUserFeedbackResponse(Map<Long, List<FeedbackResponse>> developFeedback, Map<Long, List<FeedbackResponse>> socialFeedback, Predicate<Room> predicate) {


predicate 가 이름을 가지면 좋겠는데, 적절한 이름이 떠오르지 않네요..

hjk0761 · 2024-10-17T11:01:03Z

backend/src/main/java/corea/feedback/service/UserFeedbackService.java

-                extractDistinctKeyStreams(developFeedback, socialFeedback).toList());
+    private UserFeedbackResponse getUserFeedbackResponse(Map<Long, List<FeedbackResponse>> developFeedback, Map<Long, List<FeedbackResponse>> socialFeedback, Predicate<Room> predicate) {
+        List<Long> roomIds = extractDistinctKeyStreams(developFeedback, socialFeedback).toList();
+        List<Room> rooms = roomRepository.findAllById(roomIds);


findAllByIds 와 findAllById 가 동작이 같은가요? 궁금해서 질문 남깁니다.

JPA에서는 findAllById가 표준 메서드로 제공되지만, findAllByIds는 없습니다.

findAllById는 기본적으로 여러 개의 ID를 인자로 받아서 해당 ID들에 해당하는 엔티티들을 모두 조회합니다.

기본적으로 제공되는 것은 findAllById이며, 두 메서드의 동작은 같지 않습니다~

findAllByIds를 사용하려면 직접 메서드를 정의해야겠네요!

hjk0761 · 2024-10-17T11:03:23Z

backend/src/test/java/corea/feedback/controller/UserFeedbackControllerTest.java


        assertThat(response.feedbacks()).hasSize(2);
        assertThat(response.feedbacks().get(0).developFeedback()).hasSize(1);
        assertThat(response.feedbacks().get(0).socialFeedback()).hasSize(1);
        assertThat(response.feedbacks().get(1).developFeedback()).hasSize(1);
    }
+
+    @Test
+    @DisplayName("자신이 받은 피드백을 받을 때는 닫혀있는 방에서만 받는다.")


"자신이 받은 피드백은 닫혀있는 방에서 작성된 피드백만 확인할 수 있다" 와 같은 수정은 어떠신가요?

수정 완😀

youngsu5582

Reader와 Writer의 역할을 분리하면서 RequestDto와 ResponseDto를 어디까지 전달할지 고민했고,
결국 Service 레이어를 넘어가지 않도록 하는 게 좋겠다고 판단했습니다.
...

해당 내용은 느낀점으론
DTO 를 통해 바로 만들 수 있는 도메인 이 있는게 아닌 이상 어쩔수 없는거 같아요.
( Entity - Domain 이 분리 되어야 한다는 뜻 )

지금 처럼 Service 단 DTO 가 생길때만 이렇게 해도 괜찮을 거 같아요.

코드 짠다구 고생했슴당 🙂

youngsu5582 · 2024-10-18T09:25:38Z

backend/src/main/java/corea/feedback/domain/DevelopFeedbackReader.java

+    }
+
+    public DevelopFeedback findDevelopFeedback(long roomId, long deliverId, String username) {
+        return developFeedbackRepository.findByRoomIdAndDeliverIdAndReceiverUsername(roomId, deliverId, username)


나중에 근본적으로 HTTP API 가 username 이 아닌 receiverId 를 받는것도 생각해봐야 겠네요
( 인덱스 지정 및 명확하게 하려면 )

username 이 아닌 receiverId 를 받는것도 생각해봐야 겠네요
( 인덱스 지정 및 명확하게 하려면 )

👍 동의요~!

근데 memberId를 외부로 노출시키지 않기 위해 username으로 받은 설계를 했던 것 같은데, 그럼 이런 느낌으로 되는게 맞겠죠?

public class DevelopFeedbackService { public DevelopFeedbackResponse findDevelopFeedback(long roomId, long deliverId, String username) { // 여기서 username을 통해 memberId를 찾는다. long receiverId = ...; DevelopFeedback developFeedback = developFeedbackReader.findDevelopFeedback(roomId, deliverId, receiverId); return DevelopFeedbackResponse.from(developFeedback); }

뽀로로 의견이 좋은 것 같습니다용 👍

youngsu5582 · 2024-10-18T11:46:24Z

backend/src/main/java/corea/feedback/service/UserFeedbackService.java

-                .stream()
-                .map(FeedbackResponse::fromDeliver)
-                .collect(Collectors.groupingBy(FeedbackResponse::roomId));
+        return getUserFeedbackResponse(developFeedbackOutput, socialFeedbackOutput, room -> true);


해당 부분은 뭔가 뭔가긴 한대 어쩔수 없는듯요 🥲

Reader 클래스에서 FeedbackResponse를 반환하도록 한다면 좀 편해지긴 할건데... 저 부분에선 그냥 Response 반환하자라는 유혹이 넘치더라고요ㅋㅋㅋ

방법은 더 고민해봅시당~ 👍

Room::isNotOpened는 어떠신가영?

아래 Room::isClosed 조건이랑도 매치되는 것 같구 🤔
피드백 단계면 isNotOpened 반환값도 어차피 무조건 true일 것 같아서요.

물론 FAIL status도 통과되기는 하겠지만,
지금 로직도 OPEN이나 FAIL 단계 모두 포함하는 건 마찬가지라고 생각해용.

물론 정 FAIL인 경우가 포함되는 게 거슬린다면 isProgressOrClosed 같은 boolean 메서드를 RoomStatus에 하나 추가해도 될 것 같습니다!

Room::isNotOpened는 어떠신가영?

생각지 못한 부분인데 좋네요~
수정 완료했습니다! 👍

jcoding-play · 2024-10-19T08:34:36Z

질문
서비스-도메인 계층에서의 DTO 네이밍을 바꾼건 좋은 관점인 것 같아요.
Input, Output 이라는 네이밍을 사용하게 된 이유가 궁금해요.
콘솔 혹은 DB와 소통할 거라고 오해할 수 있는 네이밍으로도 느껴지긴 하네요.

이전에 조이썬이 Input, Output이란 네이밍으로 사용해본 경험이 있다고 했고, 저도 좋은 것 같아 사용하게 되었습니다.

네이밍은 서로 맞춰보면 되고, 바꾸고 쉬우니 더 나은 네이밍이 있다면 말해주시면 됩니당~

ashsty

안녕하세요 뽀로로~~ 리뷰가 늦어져서 죄송합니다! 😂

역시 깔끔하게 잘 작성된 것 같네요! 👍👍
수정된 점 확인했고, 간단한 제안 코멘트 몇 가지만 남겨두었습니다.

P2

저도 좋은 것 같습니다!

같은 DTO를 여기저기서 돌려쓰다보니 필요 없는 파라미터까지 전달되는 경우도 많고,
정확한 용례를 판단하기 힘들다고 느꼈던 경우도 많았거든요.

그래서 저도 RoomResponse쪽 칼질을 진행하고 있었는데
뽀로로와 같은 방식으로 mapper 한 번 활용해보겠습니다. 짱~!

ashsty · 2024-10-19T08:34:16Z

backend/src/main/java/corea/feedback/domain/DevelopFeedbackReader.java

+    }
+
+    public DevelopFeedback findDevelopFeedback(long roomId, long deliverId, String username) {
+        return developFeedbackRepository.findByRoomIdAndDeliverIdAndReceiverUsername(roomId, deliverId, username)


뽀로로 의견이 좋은 것 같습니다용 👍

ashsty · 2024-10-19T08:35:17Z

backend/src/main/java/corea/feedback/domain/DevelopFeedbackWriter.java

+
+    private void validateUpdateAuthority(DevelopFeedback developFeedback, long deliverId) {
+        if (developFeedback.isNotMatchingDeliver(deliverId)) {
+            throw new CoreaException(ExceptionType.FEEDBACK_UPDATE_AUTHORIZATION_ERROR);


세부 예외 처리 👍

ashsty · 2024-10-19T08:47:39Z

backend/src/main/java/corea/feedback/service/UserFeedbackService.java

-                .stream()
-                .map(FeedbackResponse::fromDeliver)
-                .collect(Collectors.groupingBy(FeedbackResponse::roomId));
+        return getUserFeedbackResponse(developFeedbackOutput, socialFeedbackOutput, room -> true);


Room::isNotOpened는 어떠신가영?

아래 Room::isClosed 조건이랑도 매치되는 것 같구 🤔
피드백 단계면 isNotOpened 반환값도 어차피 무조건 true일 것 같아서요.

물론 FAIL status도 통과되기는 하겠지만,
지금 로직도 OPEN이나 FAIL 단계 모두 포함하는 건 마찬가지라고 생각해용.

물론 정 FAIL인 경우가 포함되는 게 거슬린다면 isProgressOrClosed 같은 boolean 메서드를 RoomStatus에 하나 추가해도 될 것 같습니다!

ashsty · 2024-10-19T08:57:36Z

backend/src/test/java/corea/feedback/controller/UserFeedbackControllerTest.java

만약 위에서 room -> true를 isXXX로 바꾼다면 여기 "자신이 작성한 피드백을 받는다"

테스트 메서드 로직도 바뀌어야 합니다!

만약 비즈니스 로직을 바꾸지 않는다고 해도 현재는 OPEN 상태인 방을 테스트하고 있는 것 같아서,
CLOSED나 PROGRESS인 방을 기준으로 테스트하도록 바꾸어보아도 좋겠네요!

같이 수정완료했습니다~ 👍

ashsty · 2024-10-19T08:58:27Z

backend/src/test/java/corea/feedback/service/UserFeedbackServiceTest.java

@@ -43,7 +43,7 @@ class UserFeedbackServiceTest {
    private SocialFeedbackRepository socialFeedbackRepository;

    @Test
-    @DisplayName("닫힌 방마다 작성한 피드백들을 구분해서 가져온다.")
+    @DisplayName("작성한 피드백들을 가져온다.")


여기도 마찬가지로 room3의 RoomStatus 확인해주시면 좋을 것 같아요! 👍

같이 수정완료했습니다~ 👍

jcoding-play added 7 commits October 16, 2024 19:32

refactor: 작성한 피드백은 바로 피드백 모아보기에서 보도록 수정

62fb486

refactor: 피드백 가져오는 역할 클래스 분리

1fcc4bc

refactor: 개발 피드백 작성 기능 수정

a8e7a3c

refactor: 개발 피드백 수정 기능 변경

efe474f

refactor: 개발 피드백 조회 기능 수정

1fbf93b

refactor: 생성, 수정 dto 분리

8ef4741

refactor: 피드백을 조회하는 기능 수정

8010cff

github-actions bot added BE 백엔드 개발 관련 작업 기능 기능 구현 작업 labels Oct 17, 2024

Merge branch 'develop' into feat/#613

32cc97d

hjk0761 assigned jcoding-play Oct 17, 2024

Merge branch 'develop' into feat/#613

55fac74

jcoding-play requested review from ashsty, youngsu5582 and hjk0761 October 17, 2024 06:20

hjk0761 approved these changes Oct 17, 2024

View reviewed changes

youngsu5582 approved these changes Oct 18, 2024

View reviewed changes

jcoding-play added 2 commits October 19, 2024 17:16

refactor: 피드백 반영

6db2d68

Merge branch 'develop' into feat/#613

bcbec54

jcoding-play force-pushed the feat/#613 branch from fcd4592 to bcbec54 Compare October 19, 2024 08:27

ashsty approved these changes Oct 19, 2024

View reviewed changes

refactor: 피드백 반영

34540ff

jcoding-play merged commit 787e2e8 into develop Oct 19, 2024
4 checks passed

jcoding-play deleted the feat/#613 branch October 19, 2024 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BE] 쓴 피드백은 피드백 모아보기에서 바로 볼 수 있도록 변경 + 리팩터링(#613) #617

[BE] 쓴 피드백은 피드백 모아보기에서 바로 볼 수 있도록 변경 + 리팩터링(#613) #617

github-actions bot commented Oct 17, 2024 •

edited by jcoding-play

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading

hjk0761 left a comment

hjk0761 Oct 17, 2024

hjk0761 Oct 17, 2024

jcoding-play Oct 19, 2024

hjk0761 Oct 17, 2024

jcoding-play Oct 19, 2024

youngsu5582 left a comment

youngsu5582 Oct 18, 2024

jcoding-play Oct 19, 2024

ashsty Oct 19, 2024

youngsu5582 Oct 18, 2024

jcoding-play Oct 19, 2024

ashsty Oct 19, 2024 •

edited

Loading

jcoding-play Oct 19, 2024

jcoding-play commented Oct 19, 2024

ashsty left a comment

ashsty Oct 19, 2024

ashsty Oct 19, 2024

ashsty Oct 19, 2024 •

edited

Loading

ashsty Oct 19, 2024 •

edited

Loading

jcoding-play Oct 19, 2024

ashsty Oct 19, 2024

jcoding-play Oct 19, 2024

[BE] 쓴 피드백은 피드백 모아보기에서 바로 볼 수 있도록 변경 + 리팩터링(#613) #617

[BE] 쓴 피드백은 피드백 모아보기에서 바로 볼 수 있도록 변경 + 리팩터링(#613) #617

Conversation

github-actions bot commented Oct 17, 2024 • edited by jcoding-play Loading

📌 관련 이슈

✨ PR 세부 내용

P2: DTO 관리

github-actions bot commented Oct 17, 2024 • edited Loading

Test Results

hjk0761 left a comment

Choose a reason for hiding this comment

질문

DTO 변환 책임에 대해서

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

youngsu5582 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

ashsty Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcoding-play commented Oct 19, 2024

ashsty left a comment

Choose a reason for hiding this comment

P2

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashsty Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

ashsty Oct 19, 2024 • 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

github-actions bot commented Oct 17, 2024 •

edited by jcoding-play

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading

ashsty Oct 19, 2024 •

edited

Loading

ashsty Oct 19, 2024 •

edited

Loading

ashsty Oct 19, 2024 •

edited

Loading