[Refactor] - 여행기 Service 코드 리팩터링 #504

Libienz · 2024-10-05T09:23:52Z

✅ 작업 내용

DTO 생성 체인이 양방향 연관관계를 적극 이용하도록 개선
여행기 생성 로직 양방향 연관관계 이용, 개선
여행기 상세 조회 로직 양방향 연관관계 이용, 개선
여행기 삭제 로직 양방향 연관관계 이용, 개선
사용되지 않는 서비스 클래스, 메서드 삭제
ImagePermanentSaver 컴포넌트 구현
하위 서비스가 도메인을 다루도록 수정 및 뷰관련 로직을 Facade로 응집

🙈 참고 사항

이미지 key Setter 열림
컬렉션 lazy 로딩 BatchSize 적용 안됨
양방향 연관관계 편의 메서드를 위한 Setter열림

eunjungL

수고하셨습니다 리비~
PR의 어마무시한 볼륨만큼 리비의 노고가 느껴집니다...

한 가지 수정됐으면 하는 부분이랑 질문, 의견이 있어서 일단 RC로 드립니다.
확인 부탁드려요!

eunjungL · 2024-10-05T13:25:08Z

backend/src/main/java/kr/touroot/travelogue/dto/request/TravelogueRequest.java

+        return travelogueDays;
+    }
+
+    private void addTravelogueDays(Travelogue travelogue) {


호출하는 public method 바로 밑으로 옮겨주세요~!

반영했습니다!

eunjungL · 2024-10-05T13:25:33Z

backend/src/main/java/kr/touroot/travelogue/dto/request/TravelogueRequest.java

@@ -31,4 +33,29 @@ public record TravelogueRequest(
    public Travelogue toTravelogueOf(Member author, String url) {


리팩토링 하면서 사용하지 않는 코드가 된 것 같네요!
지워주시면 좋을 것 같슴다

옷.. 꼼꼼한 리뷰 감사합니다

eunjungL · 2024-10-05T13:28:03Z

backend/src/main/java/kr/touroot/travelogue/service/TravelogueFacadeService.java

        travelogueTagService.createTravelogueTags(travelogue, request.tags());
-        travelogueLikeService.findLikeByTravelogueAndLiker(travelogue, author);


eunjungL · 2024-10-05T13:32:06Z

backend/src/main/java/kr/touroot/travelogue/service/TravelogueFacadeService.java

    }

    @Transactional
-    public TravelogueResponse updateTravelogue(Long id, MemberAuth member, TravelogueRequest request) {
+    public void updateTravelogue(Long id, MemberAuth member, TravelogueRequest updateRequest) {


질문)
구두로만 얘기해서 정확히 기억은 안나는데, void 형을 반환하면 테스트가 힘들어진다는 이유가 있어 Controller만 void형을 반환하고 나머지는 반환 값을 갖기로 얘기했던 것 같아요.
(혹시 뒤에 다른 논의가 있었는데 제가 컨텍스트를 놓친거라면 미리 죄송합니다 😅)

실제로 테스트가 doesNotThrowAnyException()으로 변경됐는데 예외가 발생하지 않았다라는 뜻이 업데이트에 성공했다라는 뜻으로 연결되기엔 어색하다고 느껴집니다.
개인적으로 service에서 테스트가 이뤄지고 있어도 facade 단에서의 정확한 테스트 역시 필요하다고 생각해요.

이에 대해 리비의 의견이 궁금하네요!

update는 바뀐 내용을 조회할 수 있도록 서비스 메서드가 상세조회 response를 반환한다

단 updateResponse를 새로 생성하지는 않음! 기존 상세 조회 response이용

후에 명세가 달라지게 되면 그 때 분리

delete는 void를 반환!

리건과 이야기 해보니 저번에 이렇게 결론이 났었더군요~
제가 놓친 부분입니다.

반영해서 테스트 의미 있게 수정해봤어요!

eunjungL · 2024-10-05T13:34:17Z

backend/src/main/java/kr/touroot/travelogue/service/TravelogueImagePermanentSaver.java

+
+@RequiredArgsConstructor
+@Component
+public class TravelogueImagePermanentSaver {


의견)
패키지는 service 패키지 밑인데 이름은 Saver라 뭔가 유틸 클래스의 느낌이 나네요.
어노테이션도 @Component로 주신걸 보니 서비스의 역할이 아니라고 느끼셨던 것 같아요.
패키지와 네이밍이 일치하지 않는 것 같은데 둘 중 하나는 맞춰주는게 어떨까 슬쩍 제안드립니다~

도메인을 다루고 도메인 관련 로직을 처리한다는 점에서 서비스로 해석해보았어요!

Libienz · 2024-10-07T08:00:29Z

클로버가 말씀해주신 부분과 작성한 연관관계 편의 메서드의 테스트가 추가되었습니다!

추가로 TravelogueTag 정보를 업데이트 하는 기능을 TravelogueTagService안으로 응집하였어요.
이 부분을 다들 어떻게 생각하시는지 들어보고 싶네요 저는 어색한가 싶다가도 괜찮은 것 같기도 하고..
나중에 수정로직 관련 토의가 마쳐지면 고정해야 할까요 흠... 🤔

eunjungL

리뷰 반영된거 확인했습니다~
수고하셨습니다 리비 👍

eunjungL · 2024-10-08T01:10:59Z

backend/src/main/java/kr/touroot/travelogue/dto/response/TravelogueResponse.java

+                .build();
+    }
+
+    private static TravelogueResponseBuilder baseBuilder(


nak-honest

정말 많은 코드를 리팩토링해주셔서 감사합니다 리비씨! 🙏
덕분에 코드의 가독성과 유지보수성이 많이 향상된 것 같아요. 💯
테스트 코드 수정도 상당히 번거롭고 시간이 많이 드는 작업인데, 꼼꼼하게 챙겨주셔서 너무 고생 많으셨습니다. 👏
이번 PR 덕분에 코드 퀄리티가 한층 더 좋아졌습니다!

구두로 함께 코드를 보며 이야기를 나누었기 때문에 리뷰는 많지 않습니다!
따라서 바로 approve 드립니다!

nak-honest · 2024-10-08T10:46:55Z

backend/src/main/java/kr/touroot/travelogue/domain/Travelogue.java

@@ -48,6 +48,7 @@ public class Travelogue extends BaseEntity {
    @Column(nullable = false, length = 20)
    private String title;

+    @Setter


세터를 사용해야 한다는 맥락은 이해 했는데요!!

다만 setXxx 라는 범용적인 이름보다는,
구체적인 이름을 줌으로써 사용 범위를 최대한 줄여보도록 하는 건 어떨지 조심스럽게 남겨봅니다!

낙낙의 의견에 동의합니다. setXxx보단 구체적으로 changeThumbnailWith(String newThumbnail) 같은 방식이 더 좋을 것 같습니다.

구체적인 이름이라고 해봤자 update밖에 생각나지 않네요~ 현재 다른 update메서드들이 이미 존재하기도 하고요..! change도 사실 update와 똑같은 이름이라고 생각합니다.

다만 사용처를 제한하는 맥락은 있을 것 같아서 전부 update로 바꾸어보았어요

더 좋은 의견 있다면 다음 리뷰에 말씀 부탁드립니다!

nak-honest · 2024-10-08T11:17:00Z

backend/src/main/java/kr/touroot/travelogue/service/TravelogueFacadeService.java

    @Transactional(readOnly = true)
    public TravelogueResponse findTravelogueById(Long id) {
        Travelogue travelogue = travelogueService.getTravelogueById(id);
-        return getTravelogueResponse(travelogue);
+        List<TravelogueTag> travelogueTags = travelogueTagService.readTagByTravelogue(travelogue);
+
+        return TravelogueResponse.createResponseForGuest(travelogue, travelogueTags);
    }

    @Transactional(readOnly = true)
    public TravelogueResponse findTravelogueById(Long id, MemberAuth member) {
+        Member accessor = memberService.getMemberById(member.memberId());
        Travelogue travelogue = travelogueService.getTravelogueById(id);


오버로딩이 장점이 많은 것은 사실이지만,
여기서는 오버로딩보단 서로 다른 이름을 주면 어떨까 싶네요!

TravelogueResponse.createResponseForGuest 로 따로 이름을 준 것처럼 서비스단에서도 게스트용은 다른 이름을 주는 것이 어떨지 여쭈어 봅니닷!

반영했습니다~

slimsha2dy

이 정도의 코드량이라니... 정말 수고 많으셨습니다 리비..
이번 pr에서의 목표는 충분히 완료한 것 같아서 approve 드리겠습니다.
저희끼리 얘기 나왔던 차후 개선점에 대해 리뷰 남겨봤습니다.
수고하셨습니다!

slimsha2dy · 2024-10-08T11:49:43Z

backend/src/main/java/kr/touroot/travelogue/domain/Travelogue.java

+    public void updateDays(List<TravelogueDay> travelogueDays) {
+        this.travelogueDays.clear();
+        travelogueDays.forEach(this::addDay);
+    }


이번 pr에서 할 일은 아닌 것 같긴 한데 양방향 연관관계를 활용해서 clear 없이 update를 수행하도록 바꾸면 더 좋겠네요.
update 로직 개선으로 추후 진행해봐도 좋을 것 같습니다!

이슈 파놓았습니다~ #514

변경 감지로 진행되면 깔끔할 것 같아요 다만 프론트랑 명세는 맞추어 봐야겠네요!

slimsha2dy · 2024-10-08T11:53:15Z

backend/src/main/java/kr/touroot/travelogue/service/TravelogueService.java

-        String url = s3Provider.copyImageToPermanentStorage(request.thumbnail());
-        Travelogue travelogue = request.toTravelogueOf(author, url);
+    public Travelogue save(Travelogue travelogue) {
+        travelogueImagePerpetuationService.copyTravelogueImagesToPermanentStorage(travelogue);


저희 같이 얘기 나눠 봤던 것처럼 서비스에서 서비스를 참조하고 있는 형태에 대해 좀 더 생각해봐야 할 것 같습니다!
save라는 구현 레벨에서 해당 메서드를 호출하는 것은 전혀 문제가 없는 것 같지만 같은 계층이 서로 참조하는 부분이 어색한 것 같아서요.

알파카 의견에 동의합니다. 같은 추상화 레벨의 객체 간의 의존성은 저희가 처음 정한 컨벤션에서 많이 벗어난다고 생각합니다. (다른 서비스들과 상이한 맥락이라 오용의 가능성도 있구요!)

결론을 내렸습니다. 여행기 이미지들을 영구 저장소로 이동하는 로직을 Facade에 노출되도록 수정했어요.

개선이 어색했는지 좋았는지 다음 리뷰에서 알려주시면 감사하겠습니다~

hangillee

고생하셨습니다. 리비!
제가 남긴 코멘트 확인 부탁드립니다!

hangillee · 2024-10-08T14:02:59Z

backend/src/main/java/kr/touroot/travelogue/domain/Travelogue.java

@@ -48,6 +48,7 @@ public class Travelogue extends BaseEntity {
    @Column(nullable = false, length = 20)
    private String title;

+    @Setter


낙낙의 의견에 동의합니다. setXxx보단 구체적으로 changeThumbnailWith(String newThumbnail) 같은 방식이 더 좋을 것 같습니다.

hangillee · 2024-10-08T14:05:47Z

backend/src/main/java/kr/touroot/travelogue/service/TravelogueService.java

-        String url = s3Provider.copyImageToPermanentStorage(request.thumbnail());
-        Travelogue travelogue = request.toTravelogueOf(author, url);
+    public Travelogue save(Travelogue travelogue) {
+        travelogueImagePerpetuationService.copyTravelogueImagesToPermanentStorage(travelogue);


알파카 의견에 동의합니다. 같은 추상화 레벨의 객체 간의 의존성은 저희가 처음 정한 컨벤션에서 많이 벗어난다고 생각합니다. (다른 서비스들과 상이한 맥락이라 오용의 가능성도 있구요!)

Libienz · 2024-10-09T17:25:32Z

모든 코멘트 내용 반영되었습니다.

Setter 대신 다른 이름 사용, 메서드 사용처 명확화
서비스 계층화 의미 살리기 (이미지 영구화 서비스 사용 로직 파사드 내부로 응집)
오버로딩 대신 의미있는 메서드 시그니처 사용

반영 합리적이었는지 리뷰 부탁드립니다.

hangillee

문제 없어보입니다!
고생하셨어요 리비!

Libienz added 27 commits October 4, 2024 11:10

style: 메서드 순서 수정

f73fe03

feat: TravelogueDay의 주인 되는 여행기 속성 세터 오픈

459b5e7

feat: Travelogue 양방향 연관관계 편의메서드 작성

9e18de6

feat: TravelogueDay의 TraveloguePlace 추가 양방향 연관관계 편의메서드 작성

2e0bd21

feat: TraveloguePlace의 TraveloguePhoto 추가 양방향 연관관계 편의메서드 작성

f2afac0

refactor: 양방향 연관관계를 통해 Dto 매핑 과정에서 엔티티 연관관계를 맺도록 수정

e6358d6

refactor: 양방향 연관관계를 통해 Dto 응답 Dto 매핑 과정 단순화

4692659

refactor: 사용하지 않는 메서드 제거 개선

9bb8111

refactor: 반정규화된 엔티티로부터 likeCount를 조회하도록 DTO 매핑 로직 개선

f9c116d

feat: 여행기에 대한 특정 멤버의 좋아요가 존재하는지 확인하는 기능 구현

fdaefcd

refactor: TravelogueFacadeService 양방향 연관관계를 이용한 매핑 과정 추상화 적용

93b5cbe

refactor: TravelogueLikeService 사용하지 않는 메서드 제거 개선

aaebaec

refactor: 불필요한 서비스 호출 제거 개선

873294b

refactor: 필터 여부에 따른 분기를 facade가 아닌 하위 서비스로 이동 개선

89efae0

refactor: TravelogueLikeService 사용하지 않는 메서드 제거 개선

08b5dbf

refactor: TravelogueLikeService 파사드 하위 서비스는 뷰를 다루지 않도록 개선

8f42809

fix: 여행기 콘텐츠가 수정되지 않는 문제점 해결

c7d8eb2

refactor: 파사드 하위의 TravelogueTagService가 도메인을 다루도록 계층 분리 개선

6c30061

refactor: 사용하지 않는 서비스 클래스 제거 개선

8752c64

refactor: 과한 메서드 분리 제거 개선

1ebae52

feat: 여행기 속 사진 컨텐츠를 영구 저장소로 옮기는 서비스 구현

4119f50

refactor: 사용하지 않는 메서드 제거 개선

1b3a1c1

refactor: S3 컴포넌트를 Facade가 아닌 하위 서비스가 사용하도록 추상화

5f9dbb8

test: 이미지 복사 로직 테스트 모킹 단순화 개선

09c510c

chore: develop branch merge

6dc2714

test: 특정 여행기에 특정 멤버가 좋아요했는지 확인할 수 있는 기능 테스트 작성

ad3191d

test: 여행기 저장 서비스 메서드 테스트 작성

cce9109

Libienz added BE REFACTOR ♻️ labels Oct 5, 2024

Libienz added this to the sprint 6 milestone Oct 5, 2024

eunjungL requested changes Oct 5, 2024

View reviewed changes

Libienz added 11 commits October 6, 2024 00:52

refactor: 사용하지 않는 메서드 제거 개선

70cb200

refactor: 메서드 배치 호출 순서에 맞게 수정

3286ac9

refactor: 사용하지 않는 메서드 제거 개선

0093013

refactor: Guest용 뷰 로직 DTO 내부로 응집 개선

b9dde68

refactor: TravelogueFacadeService의 여행기 업데이트 메서드가 변경 내용을 반환하도록 개선

b0c2dcc

test: 여행기 수정 테스트 doesNotThrow -> 변경 내용 확인 테스트로 개선

a7e5510

refactor: ImagePermanentSaver -> PerputationService로 이름 변경 및 서비스 컴포넌트화

72d9e83

refactor: memberService 메서드 이름 수정 (다른 서비스 메서드의 이름과 컨벤션 통일)

c37d8f1

test: 여행기 양방향 연관관계 편의 메서드 테스트 작성

daa2246

test: TravelogueDay 양방향 연관관계 편의 메서드 테스트 작성

842c40b

test: TraveloguePlace 양방향 연관관계 편의 메서드 테스트 작성

a57579b

Libienz requested a review from eunjungL October 7, 2024 08:01

eunjungL approved these changes Oct 8, 2024

View reviewed changes

nak-honest approved these changes Oct 8, 2024

View reviewed changes

slimsha2dy approved these changes Oct 8, 2024

View reviewed changes

hangillee requested changes Oct 8, 2024

View reviewed changes

Libienz and others added 4 commits October 10, 2024 02:03

refactor: Setter update로 이름 변경 (사용처 제한 명세 역할)

d079652

refactor: 오버로딩 제거 -> 메서드 시그니처에 명세 상세화

99f67be

refactor: 이미지 S3 영구 저장소 이동 로직을 Facade로 이동 (서비스 계층 분리 의미 명확하게)

6fc7d99

Merge branch 'develop/be' into feature/be/#496

720c5cf

Libienz requested a review from hangillee October 9, 2024 17:23

Libienz mentioned this pull request Oct 9, 2024

[Refactor] - 여행 계획 Service 코드 리팩터링 #507

Merged

Libienz and others added 2 commits October 10, 2024 02:33

Merge branch 'develop/be' into feature/be/#496

c27e99a

refactor: 메서드 명 변경 작업 미반영 -> 반영 조치

8b77665

hangillee approved these changes Oct 10, 2024

View reviewed changes

Libienz merged commit 6c85cf7 into develop/be Oct 10, 2024
3 checks passed

Libienz deleted the feature/be/#496 branch October 10, 2024 01:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Refactor] - 여행기 Service 코드 리팩터링 #504

[Refactor] - 여행기 Service 코드 리팩터링 #504

Libienz commented Oct 5, 2024 •

edited

Loading

eunjungL left a comment

eunjungL Oct 5, 2024

Libienz Oct 7, 2024

eunjungL Oct 5, 2024

Libienz Oct 7, 2024

eunjungL Oct 5, 2024

eunjungL Oct 5, 2024

Libienz Oct 7, 2024

eunjungL Oct 5, 2024

Libienz Oct 7, 2024

Libienz commented Oct 7, 2024 •

edited

Loading

eunjungL left a comment

eunjungL Oct 8, 2024

nak-honest left a comment

nak-honest Oct 8, 2024

hangillee Oct 8, 2024

Libienz Oct 9, 2024

nak-honest Oct 8, 2024

Libienz Oct 9, 2024

slimsha2dy left a comment

slimsha2dy Oct 8, 2024 •

edited

Loading

Libienz Oct 9, 2024 •

edited

Loading

slimsha2dy Oct 8, 2024 •

edited

Loading

hangillee Oct 8, 2024

Libienz Oct 9, 2024

hangillee left a comment

hangillee Oct 8, 2024

hangillee Oct 8, 2024

Libienz commented Oct 9, 2024

hangillee left a comment

		@@ -31,4 +33,29 @@ public record TravelogueRequest(
		public Travelogue toTravelogueOf(Member author, String url) {

		travelogueTagService.createTravelogueTags(travelogue, request.tags());
		travelogueLikeService.findLikeByTravelogueAndLiker(travelogue, author);

[Refactor] - 여행기 Service 코드 리팩터링 #504

[Refactor] - 여행기 Service 코드 리팩터링 #504

Conversation

Libienz commented Oct 5, 2024 • edited Loading

✅ 작업 내용

🙈 참고 사항

eunjungL 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

Libienz commented Oct 7, 2024 • edited Loading

eunjungL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nak-honest 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

slimsha2dy left a comment

Choose a reason for hiding this comment

slimsha2dy Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Libienz Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

slimsha2dy Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hangillee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Libienz commented Oct 9, 2024

hangillee left a comment

Choose a reason for hiding this comment

Libienz commented Oct 5, 2024 •

edited

Loading

Libienz commented Oct 7, 2024 •

edited

Loading

slimsha2dy Oct 8, 2024 •

edited

Loading

Libienz Oct 9, 2024 •

edited

Loading

slimsha2dy Oct 8, 2024 •

edited

Loading