로드맵 서비스 로직 가독성 개선 #1613

nuyh99 · 2023-11-01T07:35:52Z

#️⃣연관된 이슈

로드맵 서비스 로직 개선 #1609

📝작업 내용

roadmap 패키지의 모든 Repository를 사용해서 자바 API를 통해 조립하던 기존 로직을 JPQL을 통해 개선했습니다.
- 커리큘럼 ID에 해당하는 모든 키워드를 조회하는 findAllByCurriculumId() 구현
- 키워드 ID 별로 총 퀴즈 개수를 조회하는 findTotalQuizCount() 구현
- 키워드 ID 별로 현재 멤버가 답변한 총 퀴즈 개수를 조회하는 findDoneQuizCountByMemberId() 구현

기존 요구 사항

세션 ID에 해당하는 키워드들을 한 번에 조회할 수 있다

바뀐 요구 사항

커리큘럼 ID에 해당하는 모든 세션의 모든 키워드들을 한 번에 조회할 수 있다
- 각 키워드별 총 퀴즈 개수도 같이 반환한다
- 로그인한 유저가 조회하면 각 키워드별 답변한 총 퀴즈 개수도 같이 반환한다
- 비로그인한 유저가 조회하면 각 키워드별 답변한 총 퀴즈 개수는 0개이다

#1609

amaran-th

훨씬 깔끔하고 이해하기 쉬워진 것 같네요.
고생하셨습니다!
간단하게 의견 남겨두었습니다. 확인 부탁드려요!

amaran-th · 2023-11-01T08:04:26Z

backend/src/main/java/wooteco/prolog/roadmap/application/KeywordService.java

@@ -83,14 +84,14 @@ public KeywordsResponse findSessionIncludeRootKeywords(final Long sessionId) {

        List<Keyword> keywords = keywordRepository.findBySessionIdAndParentIsNull(sessionId);

-        return KeywordsResponse.createResponse(keywords);
+        return KeywordsResponse.of(keywords);


정적 팩토리 메서드가 하나의 매개변수만 받는 경우 of보다는 from을 사용하는 것이 일반적이라고 합니다!

List 타입이나 가변인자도 from으로 쓰나요??!

음 이건 관점에 따라 달라질 수도 있겠네요...
저의 경우 List타입이라도 매개변수의 개수가 1개라면 from을 사용했었습니다. 가변인자는 정적 팩토리 메서드에서 활용해본 적은 없지만 구현하게 된다면 of를 사용할 것 같아요
이 부분은 연어의 판단에 따르겠습니다!

amaran-th · 2023-11-01T08:05:20Z

backend/src/main/java/wooteco/prolog/roadmap/application/RoadMapService.java


 @RequiredArgsConstructor
-@Transactional
+@Transactional(readOnly = true)
 @Service
 public class RoadMapService {


조회만 담당하는 서비스라면 이름에 Query를 붙여도 좋을 것 같네요... 개인적인 의견입니다.

로드맵 서비스는 하나밖에 없고, 분리의 형태가 아니어도 커맨드, 쿼리로 나누는 것이 좋나요??

KeywordService -> KeywordCommandService, KeywordQueryService 이건 좋다고 생각해요.
하지만 로드맵 서비스는 단 하나이고 인터페이스도 하나만 있어서 과하지 않나 싶습니다...

다른 서비스들 중에 커맨드, 쿼리로 나뉘어진 서비스가 없어서 6기가 봤을 때 더 혼란만 가중할 수도 있을 것 같구요!

추가적으로 어드민 api 관련으로 정리하고 나면 키워드 서비스로 이동할 것 같아요

말씀을 듣고 보니 지금 로드맵 서비스 수준에서 고민할 문제는 아닌 것 같네요...!
알겠습니다!

amaran-th · 2023-11-01T08:07:53Z

backend/src/main/java/wooteco/prolog/roadmap/application/RoadMapService.java

-            .orElseThrow(() -> new BadRequestException(CURRICULUM_NOT_FOUND_EXCEPTION));
+        final List<Keyword> keywords = keywordRepository.findAllByCurriculumId(curriculumId);
+        final Map<Long, Integer> totalQuizCounts = getTotalQuizCounts();
+        final Map<Long, Integer> doneQuizCounts = getDoneQuizCounts(memberId);


Done보다는 getAnsweredQuizCounts()와 같은 네이밍은 어떤가요?? '했다'보다는 '답변했다'는 의미가 드러나면 좋을 것 같아서요...!

좋아요!!

#1609

amaran-th

제가 남겼던 코멘트에 대해 의견 전달이 모두 이루어진 것 같아 approve하겠습니다!

BGuga

안녕하세요 연어 간단한 리뷰 남겼습니다!

BGuga · 2023-11-03T07:03:31Z

backend/src/main/java/wooteco/prolog/roadmap/application/RoadMapService.java


 @RequiredArgsConstructor
-@Transactional
+@Transactional(readOnly = true)


readOnly를 Class 레벨에 붙히면 CUD 가 실행되지 않을 수 있으니
Class 에는 Transactional 을 붙히고
method level 에 readOnly를 다는 것이 어떤가요??
기능의 효율성 보다는 작동이 더 중요하다 생각합니다

CUD를 실행시키지 않으려고 해주는거 아닌가여?? 요기 클래스에 있는 것들은 읽기 클래스다라고 명시해주는 것 같아서요

클래스 레벨에 rw 트랜잭션

클래스 레벨에 r 트랜잭션

메서드 레벨로만 rw 또는 r 트랜잭션

세 가지 방법 중 어떤 것을 선택하더라도 각각 어떻게 동작할 지만 염두에 두면 문제는 없을 것 같다고 생각해요.
푸우 말대로 했을 때는 메서드 레벨에 readOnly를 빼먹으면 조회 작업이 rw 디비로 가게 됩니다.
이는 발견하기 어려운 오동작이라고 생각되거든요!

저는 클래스 레벨에 readOnly를 명시하고, 읽기용 디비에는 select만 가능하도록 해두면 개발자가 빠뜨렸을 때 요란하게 예외가 발생하니 좋다고 생각됩니다.

BGuga · 2023-11-03T07:05:37Z

backend/src/main/java/wooteco/prolog/roadmap/application/RoadMapService.java

+        final KeywordsResponse keywordsResponse = KeywordsResponse.of(keywords);
+        keywordsResponse.setProgress(totalQuizCounts, answeredQuizCounts);


of 에 필요한 데이터를 다 넣으면 안되나요?

dto 내부에는 최대한 로직을 없애고 싶어서 setter만 만드려고 했는데 재귀로직은 빠질 수가 없네요ㅠ
말씀하신대로 그냥 진도율 관련 정보도 dto에게 넘겨줘서 처리하도록 했습니다!

BGuga · 2023-11-03T07:08:12Z

backend/src/main/java/wooteco/prolog/roadmap/application/dto/KeywordResponse.java

+        totalQuizCount = totalQuizCounts.getOrDefault(keywordId, 0);
+        doneQuizCount = answeredQuizCounts.getOrDefault(keywordId, 0);


퀴즈 갯수가 없으면 0 으로 반환하는 것은 제약사항에 대한 정의인 것 같은데
서비스에서 이런 것을 결정하는 것이 어떨까요?

비즈니스라기보다는 그냥 방어로직으로 작성한 것입니다!
그냥 null로 넣어도 0으로 들어가기 때문에 상관없습니다.

BGuga · 2023-11-03T07:10:31Z

backend/src/main/java/wooteco/prolog/roadmap/domain/repository/KeywordRepository.java

+        "JOIN EssayAnswer e ON e.quiz.id = q.id " +
+        "WHERE e.member.id = :memberId " +
+        "GROUP BY k.id ")
+    List<KeywordIdAndAnsweredQuizCount> findAnsweredQuizCountByMemberId(@Param("memberId") Long memberId);


직접 작성한 쿼리는 메서드 명에서 직접 native 쿼리인 것을 알 수 있으면 좋을 것 같다는 생각이 드네요!!

사용자 입장에서 native 쿼리인지 jpql인지 알 필요가 있는지 궁금해요!

java-saeng

고생하셨습니다잇
중간에 궁금한게 있어서 좀 여쭤봤서여 답변해주시면 감사하겠습니다.

아 그리고 브랜치 develop이 아니라 roadmap-develop에 해야하는거 아니에여??

java-saeng · 2023-11-03T07:37:36Z

backend/src/main/java/wooteco/prolog/roadmap/application/KeywordService.java

    }

    @Transactional(readOnly = true)
    public KeywordsResponse newFindSessionIncludeRootKeywords() {
        List<Keyword> keywords = keywordRepository.newFindByParentIsNull();

-        return KeywordsResponse.createResponse(keywords);
+        return KeywordsResponse.of(keywords);


정팩메 쓸 때, 그냥 인자가 하나이면 from 쓰지 않나여?? 리스트나 가변인자 또한 하나의 자료구조인거지, List가 여러 개가 아니니까 저는 통용적으로 사용하는 from 사용하는게 좋아보입니다!

java-saeng · 2023-11-03T07:38:27Z

backend/src/main/java/wooteco/prolog/roadmap/application/RoadMapService.java


 @RequiredArgsConstructor
-@Transactional
+@Transactional(readOnly = true)


CUD를 실행시키지 않으려고 해주는거 아닌가여?? 요기 클래스에 있는 것들은 읽기 클래스다라고 명시해주는 것 같아서요

java-saeng · 2023-11-03T07:43:28Z

backend/src/test/java/wooteco/prolog/roadmap/application/RoadMapServiceTest.java

+        final KeywordsResponse response = roadMapService.findAllKeywordsWithProgress(curriculumId, null);
+
+        //then
+        assertSoftly(softAssertions -> {


then절은 원하는 값과 실제 값을 비교하는거라고 생각합니다.
근데 이 코드 부분이 너무 길어서 뭘 검증하고 싶은건지 라는 생각이 초큼 듭니다,,
이건 개인적인 의견입니다!

커리큘럼 1, 세션 2, 키워드 4, 퀴즈 2, 답변 2
위의 엔티티들이 모여서 트리 형태의 키워드 dto를 리턴합니다.

따라서 값 검증을 하기 위해서는 길어질 수 밖에 없었습니다.

AssertJ에서 제공하는 체이닝을 중복만 제거해서 새로 이름만 붙여준 것입니다.
예를 들면 최상위 키워드는 2개여야 하고, keywordId는 ~, 총 퀴즈 수는 ~, 현재 멤버가 답변한 퀴즈 수는 ~

java-saeng · 2023-11-03T07:45:56Z

backend/src/main/java/wooteco/prolog/roadmap/domain/repository/KeywordRepository.java

+        "FROM Keyword k " +
+        "JOIN Quiz q ON q.keyword.id = k.id " +
+        "GROUP BY k.id")
+    List<KeywordIdAndTotalQuizCount> findTotalQuizCount();


이거 처음보는건데, JPQL에서 명시적으로 인터페이스에 관한 구현체를 만들어주는건가요??

KeywordIdAndTotalQuizCount, KeywordIdAndAnsweredQuizCount 얘네 둘다 DTO 패키지에 있는데 interface 더라구요

어떻게 동작하는건지 처음 봐서 한번 여쭤봅니다!!

원하는 컬럼만 디비에서 가져오고 싶을 때 dto 프로젝션을 사용합니다!
클래스로 만들 수도 있고, 인터페이스로 getter 네이밍만 맞춰주고 jpa가 생성하도록 할 수도 있습니다.

어떤 역할을 수행하는 객체로 프로젝션하고 싶다면 클래스 기반의 프로젝션,
지금은 단순히 dto로 사용하기 위해서 인터페이스 기반의 프로젝션을 사용했습니다!

#1609

nuyh99 · 2023-11-22T08:36:05Z

고생하셨습니다잇 중간에 궁금한게 있어서 좀 여쭤봤서여 답변해주시면 감사하겠습니다.

아 그리고 브랜치 develop이 아니라 roadmap-develop에 해야하는거 아니에여??

푸우라는 분이 이제는 바로 develop에 쏴도 될 것 같다고 하셔서 눈물을 훔치며 pr 날렸습니다...

sonarcloud · 2023-11-22T08:36:32Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

94.4% Coverage
0.0% Duplication

The version of Java (11.0.21) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

nuyh99 added 6 commits November 1, 2023 11:03

feat: 유저 ID로 답변 개수 조회 기능 구현

dc4fae9

#1609

test: 유저 ID로 답변 개수 조회 기능 테스트

11e87dc

#1609

test: 커리큘럼의 모든 키워드 조회 테스트

bc41ccb

#1609

refactor: 커리큘럼 ID의 모든 키워드 가져오는 로직 JPQL 사용해서 리팩토링

5faa0fb

#1609

refactor: Dto -> Map 변환 로직 Stream API 사용

746ee8c

#1609

refactor: 키워드 조회 시 추천 포스트 fetch join -> batch size 사용

4eac034

#1609

nuyh99 added BE 로드맵 로드맵 스쿼드 관련 이슈 labels Nov 1, 2023

nuyh99 self-assigned this Nov 1, 2023

amaran-th reviewed Nov 1, 2023

View reviewed changes

style: doneQuizCount -> answeredQuizCount로 용어 변경

c730607

#1609

amaran-th approved these changes Nov 1, 2023

View reviewed changes

BGuga requested changes Nov 3, 2023

View reviewed changes

java-saeng approved these changes Nov 3, 2023

View reviewed changes

nuyh99 added 2 commits November 22, 2023 16:33

style: 정적 팩토리 메서드 네이밍 변경

d94a493

#1609

refactor: 진도율 setter 제거 후 dto 내부에서 매핑하도록 변경

e9cc339

#1609

nuyh99 merged commit 1f8d48a into develop Nov 22, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

로드맵 서비스 로직 가독성 개선 #1613

로드맵 서비스 로직 가독성 개선 #1613

nuyh99 commented Nov 1, 2023

amaran-th left a comment

amaran-th Nov 1, 2023

nuyh99 Nov 1, 2023

amaran-th Nov 1, 2023

amaran-th Nov 1, 2023

nuyh99 Nov 1, 2023

amaran-th Nov 1, 2023

amaran-th Nov 1, 2023

nuyh99 Nov 1, 2023

amaran-th left a comment

BGuga left a comment

BGuga Nov 3, 2023

java-saeng Nov 3, 2023

nuyh99 Nov 22, 2023

BGuga Nov 3, 2023

nuyh99 Nov 22, 2023

BGuga Nov 3, 2023

nuyh99 Nov 22, 2023

BGuga Nov 3, 2023

nuyh99 Nov 22, 2023

java-saeng left a comment •

edited

Loading

java-saeng Nov 3, 2023

java-saeng Nov 3, 2023

java-saeng Nov 3, 2023

nuyh99 Nov 22, 2023

java-saeng Nov 3, 2023

nuyh99 Nov 22, 2023

nuyh99 commented Nov 22, 2023

sonarcloud bot commented Nov 22, 2023

		final KeywordsResponse keywordsResponse = KeywordsResponse.of(keywords);
		keywordsResponse.setProgress(totalQuizCounts, answeredQuizCounts);

		totalQuizCount = totalQuizCounts.getOrDefault(keywordId, 0);
		doneQuizCount = answeredQuizCounts.getOrDefault(keywordId, 0);

로드맵 서비스 로직 가독성 개선 #1613

로드맵 서비스 로직 가독성 개선 #1613

Conversation

nuyh99 commented Nov 1, 2023

#️⃣연관된 이슈

📝작업 내용

기존 요구 사항

바뀐 요구 사항

amaran-th 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

amaran-th left a comment

Choose a reason for hiding this comment

BGuga 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

java-saeng 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

nuyh99 commented Nov 22, 2023

sonarcloud bot commented Nov 22, 2023

java-saeng left a comment •

edited

Loading