[BE] 서비스 중복 로직 제거 및 캡슐화 #724

woosung1223 · 2023-11-08T12:10:17Z

구현 기능 및 변경 사항

서비스 로직 개선, 중복 제거
가독성 개선
사용하지 않는 메소드 정리

스크린샷(선택)

jaehee329

id 기반으로 처리할 수 있는 검증 로직들을 다 넣어주셨네요 좋습니다 👍
저희끼리도 컨벤션이 완벽하게 일치하지 않는 부분들은 한 번에 얘기해보죠 😂

jaehee329 · 2023-11-09T01:13:27Z

backend/src/main/java/harustudy/backend/polling/service/PollingService.java

+                .anyMatch(Participant::getIsHost);
+    }
+
+    public ProgressResponse pollInProgress(Long studyId) {


혹시 InProgress라고 명명한 이유가 있을까요?

이 클래스에서 메서드 명인 pollWaiting과 pollProgress가 어느 상황에 쓰이는지를 표현하고
무슨 일을 하는지 표현하지는 않는 것 같아서 이 기회에 둘 다 바꾸면 어떨까 싶어서요

당장은 전자는 step과 참여자를, 후자는 step을 가져온다는 의미를 담으면 어떨까 싶은데요..

오 좋은거 같습니다!

기존이 Waiting으로 되어있길래 맞췄는데 차라리 무엇을 반환하는지 명확히 하는게 낫겠네요 👍

pollForWaitingStep, pollForProgressStep 정도는 어떠신가욤? 무엇을 반환하는지를 메서드 명에 반영하려면 Dto 네이밍도 같이 수정되야 하지 않을까 싶은 생각이 드는데 더 적절한 네이밍 있으면 얘기 나눠보시죠!

그런데 지금와서 생각해보니 pollWaiting도 결국 뷰에 의존적인 메소드 네이밍인데, 하나만 pollStep으로 맞추는게 일관성이 깨지는 느낌이 드는 것 같습니다. 차라리 둘 다 뷰에 의존적인 네이밍으로 두는 게 낫지 않을까요?

일관성을 깨뜨리기 시작하면 히이로 말처럼 DTO부터 다 개편해야 될 것 같습니다! 제 생각엔 여기 PR에서 하지 않고 추후 도메인 연관관계 재설정하면서 갈아엎어도 좋을 것 같은데 어떻게 생각하시나요?

레거시 개선 시간이 두렵네요 ㅋㅋㅋㅋㅋ
이정도 레거시는 쌓아둬도 될 것 같긴 합니다

jaehee329 · 2023-11-09T01:18:11Z

backend/src/main/java/harustudy/backend/study/repository/StudyRepository.java

+    @Query("select s from Study s join s.participants p " +
+            "where p.member.id = :memberId and s.createdDate between :startDate and :endDate")
+    List<Study> findByMemberIdAndCreatedDateSortedBy(Long memberId, LocalDateTime startDate,
+                                                     LocalDateTime endDate, Sort sort);
+
+    @Query("select s from Study s join s.participants p " +
+            "where p.member.id = :memberId and s.createdDate between :startDate and :endDate")
+    Page<Study> findPageByMemberIdAndCreatedDate(Long memberId, LocalDateTime startDate,
+                                                 LocalDateTime endDate, Pageable pageable);


저희가 컨트롤러에서는 생략해도 괜찮은 @PathVariable과 같은 어노테이션들을 명시적으로 붙여주고 있는데요,
레포지토리에서는 JPQL을 쓰는 경우와 그렇지 않은 경우 모두 생략하는 것으로 하나요?
저는 둘 다 상관 없긴 합니다.. 😅

해당 부분을 제외하고는 다 @Param 이 붙어있지 않길래 지웠는데 이거는 팀 차원에서 논의해보면 좋을 것 같습니다! 👍

저도 상관 없는 쪽인지라 굳이 이유가 없다면 명시하지 않는 쪽으로 통일하는게 더 깔끔할 것 같네요 👍

aak2075

뚱뚱했던 서비스가 날씬해지고 있군요ㅋㅋㅋ 고생하셨습니다!
코멘트 하나 남겼으니 확인해 주세요~

aak2075 · 2023-11-10T00:06:06Z

backend/src/main/java/harustudy/backend/participant/domain/Participant.java

@@ -95,4 +97,16 @@ public void validateIsHost() {
            throw new ParticipantIsNotHostException();
        }
    }
+
+    public void validateIsBelongsTo(Study study) {
+        if (!this.study.getId().equals(study.getId())) {


(논란 투척)
직접 id 꺼내서 비교하기 보다 this.study.equals(study)로 비교하는건 어떤가요...?
엔티티의 equals를 어떻게 정의하냐가 논쟁이 많은 주제인것 같은데
좋은 글들도 많이 있더라구요
https://blog.yevgnenll.me/posts/jpa-entity-eqauls-and-hashcode-equality
영한님 강의에서는 비즈니스키로 equals를 재정의할것이 좋다고 하셨었는데, study에는 비즈니스키가 딱히 없는것 같기도 해서 pk로 equals 재정의 하고 id가 null이면 false를 반환하면 괜찮다고 생각합니다.
이렇게 했을때 가능하면 엔티티를 비교하거나 컬렉션을 사용할 때 영속상태의 엔티티로 사용해야 함을 인지해야겠죠.

좋은 아티클 추천 감사합니다 마코! 덕분에 잊고 지내던 동등성에 대해서 다시 한 번 생각할 수 있게 되었네요 ㅎㅎ

Study 엔티티에서 id를 제외하면 다른 필드를 가지고 Study 엔티티를 고유하게 판단할 수 있는 기준으로 잡을 수 있는 비즈니스 키를 도출하기 어렵겠다는 데는 같은 생각입니다. 같은 이유로 PK 이외의 필드들로 엔티티의 고유성을 부여하는 것도 어려우니, 선택지가 PK밖에 없을 것 같아요.

사실 Set과 같은 컬렉션을 사용할 때 해당 자료구조의 동작 원리가 깨질 수 있다는 건 비영속화된 엔티티를 영속화된 엔티티와 함께 사용하기 때문이라고 생각해요. 그리고 비영속화된 엔티티를 비즈니스 로직을 처리하는데 사용해야 된다고 한다면 VO로서 사용되어야 한다는 입장입니다. 엔티티와 VO로 나누어 생각한다면 이들을 함께 컬렉션에 담아 사용하지는 않을 것 같아요!

저희가 프로젝트 리팩토링의 방향을 좀 더 객체지향스러운 쪽으로 진행하고자 한다면 마코가 제시해주신 방향대로 구현하는게 맞을 것 같아요. 추후 저희가 컬렉션을 사용해야 하는 상황이 온다면 VO로 변환해서 해당 로직을 수행하도록 하면 될 것이고요.

몇달 전에 했었던 지하철 미션이 생각나네요 ㅎㅎ...
개인적인 생각들을 정리해봤는데 다른 크루들의 의견도 궁금합니다.

오 오랜만에 레벨1의 영광을 다시 느끼는 기분이군요..👍

말씀해주신대로 id 값을 기준으로 동등성을 비교하느냐 내부 필드를 기준으로 동등성을 비교하느냐는 객체의 성격에 따라 나뉠 거 같습니다!

제 기억으로는 코드 곳곳에 이런 동등성 비교가 id로 이루어졌던 것 같아 개편이 필요하긴 한 것 같습니다.

개인적으로는 조만간 도메인 격벽 분리 & 이벤트로 의존성 개선을 해야 할 것 같은데 해당 이슈 미리 파두고 한번에 개선하면 좋을 것 같은데 어떻게 생각하시나요?

만약 이번 PR에서 개선해야 한다면 추가적인 논의가 필요한 것 같은데, 이는 만나서 이야기해봐야 할 것 같네요!

위에 polling 관련 메서드 네이밍 관련한 이슈도 그렇고 따로 PR을 만들어 거기에서 해결해보는 방향이 좋겠네요!

일단 이슈 먼저 파뒀습니다!

aak2075 · 2023-11-10T00:54:15Z

backend/src/main/java/harustudy/backend/content/service/ContentService.java

-        validateStudyIsPlanning(study);
+        participant.validateIsBelongsTo(study);
+        participant.validateIsCreatedByMember(member);
+        study.validateIsPlanning();


훨씬 좋네요 굳굳

MoonJeWoong

점진적 리팩토링 좋네요 ㅎㅎ 사소한 부분들 리뷰 남겨봤는데 확인 부탁드려용!

MoonJeWoong · 2023-11-11T18:24:43Z

backend/src/main/java/harustudy/backend/participant/domain/Participant.java

@@ -95,4 +97,16 @@ public void validateIsHost() {
            throw new ParticipantIsNotHostException();
        }
    }
+
+    public void validateIsBelongsTo(Study study) {
+        if (!this.study.getId().equals(study.getId())) {


좋은 아티클 추천 감사합니다 마코! 덕분에 잊고 지내던 동등성에 대해서 다시 한 번 생각할 수 있게 되었네요 ㅎㅎ

Study 엔티티에서 id를 제외하면 다른 필드를 가지고 Study 엔티티를 고유하게 판단할 수 있는 기준으로 잡을 수 있는 비즈니스 키를 도출하기 어렵겠다는 데는 같은 생각입니다. 같은 이유로 PK 이외의 필드들로 엔티티의 고유성을 부여하는 것도 어려우니, 선택지가 PK밖에 없을 것 같아요.

사실 Set과 같은 컬렉션을 사용할 때 해당 자료구조의 동작 원리가 깨질 수 있다는 건 비영속화된 엔티티를 영속화된 엔티티와 함께 사용하기 때문이라고 생각해요. 그리고 비영속화된 엔티티를 비즈니스 로직을 처리하는데 사용해야 된다고 한다면 VO로서 사용되어야 한다는 입장입니다. 엔티티와 VO로 나누어 생각한다면 이들을 함께 컬렉션에 담아 사용하지는 않을 것 같아요!

저희가 프로젝트 리팩토링의 방향을 좀 더 객체지향스러운 쪽으로 진행하고자 한다면 마코가 제시해주신 방향대로 구현하는게 맞을 것 같아요. 추후 저희가 컬렉션을 사용해야 하는 상황이 온다면 VO로 변환해서 해당 로직을 수행하도록 하면 될 것이고요.

몇달 전에 했었던 지하철 미션이 생각나네요 ㅎㅎ...
개인적인 생각들을 정리해봤는데 다른 크루들의 의견도 궁금합니다.

MoonJeWoong · 2023-11-11T18:37:09Z

backend/src/main/java/harustudy/backend/participant/service/ParticipantService.java

-    public ParticipantsResponse tempFindParticipantWithFilter(
-            AuthMember authMember, Long studyId, Long memberId
-    ) {
+    public ParticipantsResponse tempFindParticipantWithFilter(AuthMember authMember, Long studyId, Long memberId) {


요거 temp 메서드 왜 생겼었는지 또 기억이 안나네요 ㅎㅎ...
그건 차치하고 당장 제거하지 않을 거면 아래 findParticipantWithFilter 메서드처럼 읽기 전용 트랜잭션 설정해주는 것은 어떤가욤?

오 여기 readonly가 빠졌었네요!

굿굿 바로 변경했습니다!

MoonJeWoong · 2023-11-11T19:11:29Z

backend/src/main/java/harustudy/backend/participant/service/ParticipantService.java

+        validateEverParticipated(authMember, study);
+        participant.validateIsCreatedByMember(member);
+        participant.validateIsBelongsTo(study);


제가 새벽에 리뷰해서 헷갈린 걸 수도 있는데 validateEverParticipated 검증 내용은 아래 validate 두줄로 검증되지 않나용? member가 authMember Id 로 조회해온 것이라서 119번째 라인 검증은 불필요할 것 같은데 어떻게 생각하시는지요?

생각해보니 그렇네요!

participant가 member에 의해 생성되었는지

study와 연관된 participant인지
를 검증하고 있으므로 study + member로 participant를 찾을 필요는 없을 것 같습니다!

불필요한 쿼리 제거 👍

MoonJeWoong · 2023-11-11T19:20:46Z

backend/src/main/java/harustudy/backend/study/service/StudyService.java

+        Study study = new Study(request.name(), request.totalCycle(), request.timePerCycle());
+        studyRepository.save(study);
+        participantCodeRepository.save(generateUniqueCode(study));

-        return savedStudy.getId();
+        return study.getId();


JPA에서는 save 메서드의 반환값을 받아서 사용하는 것이 persist와 merge 방식 모두 통일된 형식으로 사용할 수 있어서 좋다고 생각하는데 이렇게 변경해주신 이유가 따로 있으신지 궁금합니다!

해당 시점에서는 study, saveStudy가 같은 인스턴스이기 때문에 동일성 비교도 성립하는데, 하나의 메모리 주소값에 대한 이름을 두 개 가질 필요는 없다고 생각했습니다! (관리 비용이 조금이라도 증가하니까요)

또한 해당 메소드는 createStudy로 스터디를 생성하는 기능을 담당하고 있는데, merge가 발생할 일이 없지 않을까요?

만약 merge가 발생해야 하는 상황이라면 말씀해주신대로 savedStudy를 사용하는게 맞겠다는 생각이 듭니다!

그런데 merge가 절대 발생하지 않을 상황에서 컨벤션으로 savedStudy처럼 사용하는 것은 관리 포인트가 증가하는 것 같아서 저렇게 변경했습니다! (유연성은 필요한 시점에 부여하는게 좋다고 생각하는 입장입니다..!)

요 부분은 팀 컨벤션적인 부분이라서 정하고 지키기만 하면 될 것 같네요! 아직 저희 서비스 비즈니스 로직에서 준영속 엔티티를 관리하는 경우는 없다보니 나중에 필요해지는 순간에 다시 논의해보시죠 😄

MoonJeWoong · 2023-11-11T19:25:06Z

backend/src/main/java/harustudy/backend/study/repository/StudyRepository.java

+    @Query("select s from Study s join s.participants p " +
+            "where p.member.id = :memberId and s.createdDate between :startDate and :endDate")
+    List<Study> findByMemberIdAndCreatedDateSortedBy(Long memberId, LocalDateTime startDate,
+                                                     LocalDateTime endDate, Sort sort);
+
+    @Query("select s from Study s join s.participants p " +
+            "where p.member.id = :memberId and s.createdDate between :startDate and :endDate")
+    Page<Study> findPageByMemberIdAndCreatedDate(Long memberId, LocalDateTime startDate,
+                                                 LocalDateTime endDate, Pageable pageable);


저도 상관 없는 쪽인지라 굳이 이유가 없다면 명시하지 않는 쪽으로 통일하는게 더 깔끔할 것 같네요 👍

MoonJeWoong · 2023-11-11T19:34:42Z

backend/src/main/java/harustudy/backend/polling/service/PollingService.java

+                .anyMatch(Participant::getIsHost);
+    }
+
+    public ProgressResponse pollInProgress(Long studyId) {


pollForWaitingStep, pollForProgressStep 정도는 어떠신가욤? 무엇을 반환하는지를 메서드 명에 반영하려면 Dto 네이밍도 같이 수정되야 하지 않을까 싶은 생각이 드는데 더 적절한 네이밍 있으면 얘기 나눠보시죠!

MoonJeWoong

도메인 분리 내용 관련해서 이슈 파둬서 해당 PR은 merge 하겠습니다.

woosung1223 added 3 commits November 7, 2023 10:57

refactor: ContentService 중복 로직 제거 및 도메인 로직 캡슐화

560d1c5

refactor: 서비스 로직 개선 및 메소드명을 이해하기 쉽게 변경

0d32863

refactor: repository 스타일 일관화

d8983f9

woosung1223 added BE 백엔드 작업 refactor labels Nov 8, 2023

woosung1223 self-assigned this Nov 8, 2023

jaehee329 approved these changes Nov 9, 2023

View reviewed changes

aak2075 approved these changes Nov 10, 2023

View reviewed changes

MoonJeWoong requested changes Nov 11, 2023

View reviewed changes

woosung1223 added 2 commits November 12, 2023 13:03

chore: @Transactional readonly 설정

52770a7

refactor: 중복 검증 로직 제거

d24537a

MoonJeWoong approved these changes Nov 16, 2023

View reviewed changes

MoonJeWoong merged commit 9417229 into develop Nov 16, 2023
3 checks passed

MoonJeWoong deleted the be/feature/721-service-encapsulation branch November 16, 2023 08:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BE] 서비스 중복 로직 제거 및 캡슐화 #724

[BE] 서비스 중복 로직 제거 및 캡슐화 #724

woosung1223 commented Nov 8, 2023

jaehee329 left a comment

jaehee329 Nov 9, 2023

woosung1223 Nov 9, 2023

MoonJeWoong Nov 11, 2023

woosung1223 Nov 12, 2023

jaehee329 Nov 15, 2023

jaehee329 Nov 9, 2023

woosung1223 Nov 9, 2023

MoonJeWoong Nov 11, 2023

aak2075 left a comment

aak2075 Nov 10, 2023

MoonJeWoong Nov 11, 2023

woosung1223 Nov 12, 2023

MoonJeWoong Nov 12, 2023 •

edited

Loading

aak2075 Nov 10, 2023

MoonJeWoong left a comment

MoonJeWoong Nov 11, 2023

MoonJeWoong Nov 11, 2023

woosung1223 Nov 12, 2023

MoonJeWoong Nov 11, 2023

woosung1223 Nov 12, 2023

MoonJeWoong Nov 11, 2023

woosung1223 Nov 12, 2023

MoonJeWoong Nov 12, 2023

MoonJeWoong Nov 11, 2023

MoonJeWoong Nov 11, 2023

MoonJeWoong left a comment

[BE] 서비스 중복 로직 제거 및 캡슐화 #724

[BE] 서비스 중복 로직 제거 및 캡슐화 #724

Conversation

woosung1223 commented Nov 8, 2023

관련 이슈

구현 기능 및 변경 사항

스크린샷(선택)

jaehee329 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

aak2075 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

MoonJeWoong Nov 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoonJeWoong 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

MoonJeWoong left a comment

Choose a reason for hiding this comment

MoonJeWoong Nov 12, 2023 •

edited

Loading