-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
로드맵 서비스 로직 가독성 개선 #1613
로드맵 서비스 로직 가독성 개선 #1613
Changes from 6 commits
dc4fae9
11e87dc
bc41ccb
5faa0fb
746ee8c
4eac034
c730607
d94a493
e9cc339
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,5 @@ | ||
package wooteco.prolog.roadmap.application; | ||
|
||
import static wooteco.prolog.common.exception.BadRequestCode.ROADMAP_KEYWORD_NOT_FOUND_EXCEPTION; | ||
import static wooteco.prolog.common.exception.BadRequestCode.ROADMAP_SESSION_NOT_FOUND_EXCEPTION; | ||
|
||
import java.util.List; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.transaction.annotation.Transactional; | ||
import wooteco.prolog.common.exception.BadRequestException; | ||
|
@@ -15,6 +11,11 @@ | |
import wooteco.prolog.roadmap.domain.repository.KeywordRepository; | ||
import wooteco.prolog.session.domain.repository.SessionRepository; | ||
|
||
import java.util.List; | ||
|
||
import static wooteco.prolog.common.exception.BadRequestCode.ROADMAP_KEYWORD_NOT_FOUND_EXCEPTION; | ||
import static wooteco.prolog.common.exception.BadRequestCode.ROADMAP_SESSION_NOT_FOUND_EXCEPTION; | ||
|
||
@Transactional | ||
@Service | ||
public class KeywordService { | ||
|
@@ -23,7 +24,7 @@ public class KeywordService { | |
private final KeywordRepository keywordRepository; | ||
|
||
public KeywordService(final SessionRepository sessionRepository, | ||
final KeywordRepository keywordRepository) { | ||
final KeywordRepository keywordRepository) { | ||
this.sessionRepository = sessionRepository; | ||
this.keywordRepository = keywordRepository; | ||
} | ||
|
@@ -83,14 +84,14 @@ public KeywordsResponse findSessionIncludeRootKeywords(final Long sessionId) { | |
|
||
List<Keyword> keywords = keywordRepository.findBySessionIdAndParentIsNull(sessionId); | ||
|
||
return KeywordsResponse.createResponse(keywords); | ||
return KeywordsResponse.of(keywords); | ||
} | ||
|
||
@Transactional(readOnly = true) | ||
public KeywordsResponse newFindSessionIncludeRootKeywords() { | ||
List<Keyword> keywords = keywordRepository.newFindByParentIsNull(); | ||
|
||
return KeywordsResponse.createResponse(keywords); | ||
return KeywordsResponse.of(keywords); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 정팩메 쓸 때, 그냥 인자가 하나이면 from 쓰지 않나여?? 리스트나 가변인자 또한 하나의 자료구조인거지, List가 여러 개가 아니니까 저는 통용적으로 사용하는 from 사용하는게 좋아보입니다! |
||
} | ||
|
||
public void updateKeyword( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,122 +1,50 @@ | ||
package wooteco.prolog.roadmap.application; | ||
|
||
import java.util.Comparator; | ||
import java.util.HashSet; | ||
import java.util.stream.Collectors; | ||
import lombok.RequiredArgsConstructor; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.transaction.annotation.Transactional; | ||
import wooteco.prolog.common.exception.BadRequestException; | ||
import wooteco.prolog.roadmap.application.dto.KeywordResponse; | ||
import wooteco.prolog.roadmap.application.dto.KeywordsResponse; | ||
import wooteco.prolog.roadmap.application.dto.RecommendedPostResponse; | ||
import wooteco.prolog.roadmap.domain.Curriculum; | ||
import wooteco.prolog.roadmap.domain.EssayAnswer; | ||
import wooteco.prolog.roadmap.domain.Keyword; | ||
import wooteco.prolog.roadmap.domain.Quiz; | ||
import wooteco.prolog.roadmap.domain.repository.CurriculumRepository; | ||
import wooteco.prolog.roadmap.domain.repository.EssayAnswerRepository; | ||
import wooteco.prolog.roadmap.domain.repository.KeywordRepository; | ||
import wooteco.prolog.roadmap.domain.repository.QuizRepository; | ||
import wooteco.prolog.session.domain.Session; | ||
import wooteco.prolog.session.domain.repository.SessionRepository; | ||
import wooteco.prolog.roadmap.domain.repository.dto.KeywordIdAndDoneQuizCount; | ||
import wooteco.prolog.roadmap.domain.repository.dto.KeywordIdAndTotalQuizCount; | ||
|
||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Set; | ||
|
||
import static java.util.stream.Collectors.groupingBy; | ||
import static java.util.stream.Collectors.toList; | ||
import static java.util.stream.Collectors.toSet; | ||
import static wooteco.prolog.common.exception.BadRequestCode.CURRICULUM_NOT_FOUND_EXCEPTION; | ||
import static java.util.stream.Collectors.toMap; | ||
|
||
@RequiredArgsConstructor | ||
@Transactional | ||
@Transactional(readOnly = true) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. readOnly를 Class 레벨에 붙히면 CUD 가 실행되지 않을 수 있으니 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CUD를 실행시키지 않으려고 해주는거 아닌가여?? 요기 클래스에 있는 것들은 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
세 가지 방법 중 어떤 것을 선택하더라도 각각 어떻게 동작할 지만 염두에 두면 문제는 없을 것 같다고 생각해요. 저는 클래스 레벨에 readOnly를 명시하고, 읽기용 디비에는 select만 가능하도록 해두면 개발자가 빠뜨렸을 때 요란하게 예외가 발생하니 좋다고 생각됩니다. |
||
@Service | ||
public class RoadMapService { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 조회만 담당하는 서비스라면 이름에 Query를 붙여도 좋을 것 같네요... 개인적인 의견입니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 로드맵 서비스는 하나밖에 없고, 분리의 형태가 아니어도 커맨드, 쿼리로 나누는 것이 좋나요??
다른 서비스들 중에 커맨드, 쿼리로 나뉘어진 서비스가 없어서 6기가 봤을 때 더 혼란만 가중할 수도 있을 것 같구요! 추가적으로 어드민 api 관련으로 정리하고 나면 키워드 서비스로 이동할 것 같아요 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 말씀을 듣고 보니 지금 로드맵 서비스 수준에서 고민할 문제는 아닌 것 같네요...! |
||
|
||
private final CurriculumRepository curriculumRepository; | ||
private final SessionRepository sessionRepository; | ||
private final KeywordRepository keywordRepository; | ||
private final QuizRepository quizRepository; | ||
private final EssayAnswerRepository essayAnswerRepository; | ||
|
||
@Transactional(readOnly = true) | ||
public KeywordsResponse findAllKeywordsWithProgress(final Long curriculumId, final Long memberId) { | ||
final Curriculum curriculum = curriculumRepository.findById(curriculumId) | ||
.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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done보다는 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 좋아요!! |
||
|
||
final List<Keyword> keywordsInCurriculum = getKeywordsInCurriculum(curriculum); | ||
final KeywordsResponse keywordsResponse = KeywordsResponse.of(keywords); | ||
keywordsResponse.setProgress(totalQuizCounts, doneQuizCounts); | ||
|
||
final Map<Keyword, Set<Quiz>> quizzesInKeywords = quizRepository.findAll().stream() | ||
.collect(groupingBy(Quiz::getKeyword, toSet())); | ||
|
||
return createResponsesWithProgress(keywordsInCurriculum, quizzesInKeywords, getDoneQuizzes(memberId)); | ||
} | ||
|
||
private Set<Quiz> getDoneQuizzes(final Long memberId) { | ||
return essayAnswerRepository.findAllByMemberId(memberId).stream() | ||
.map(EssayAnswer::getQuiz) | ||
.collect(toSet()); | ||
} | ||
|
||
private List<Keyword> getKeywordsInCurriculum(final Curriculum curriculum) { | ||
final Set<Long> sessionIds = sessionRepository.findAllByCurriculumId(curriculum.getId()) | ||
.stream() | ||
.map(Session::getId) | ||
.collect(toSet()); | ||
|
||
return keywordRepository.findBySessionIdIn(sessionIds); | ||
} | ||
|
||
private KeywordsResponse createResponsesWithProgress(final List<Keyword> keywords, | ||
final Map<Keyword, Set<Quiz>> quizzesPerKeyword, | ||
final Set<Quiz> doneQuizzes) { | ||
final List<KeywordResponse> keywordResponses = keywords.stream() | ||
.filter(Keyword::isRoot) | ||
.map(keyword -> createResponseWithProgress(keyword, quizzesPerKeyword, doneQuizzes)) | ||
.sorted(Comparator.comparing(KeywordResponse::getKeywordId)) | ||
.collect(toList()); | ||
|
||
return new KeywordsResponse(keywordResponses); | ||
} | ||
|
||
private KeywordResponse createResponseWithProgress(final Keyword keyword, | ||
final Map<Keyword, Set<Quiz>> quizzesPerKeyword, | ||
final Set<Quiz> doneQuizzes) { | ||
final int totalQuizCount = quizzesPerKeyword.getOrDefault(keyword, new HashSet<>()).size(); | ||
final int doneQuizCount = getDoneQuizCount( | ||
quizzesPerKeyword.getOrDefault(keyword, new HashSet<>()), doneQuizzes); | ||
|
||
final List<RecommendedPostResponse> recommendedPostResponses = keyword.getRecommendedPosts().stream() | ||
.map(RecommendedPostResponse::from) | ||
.collect(toList()); | ||
|
||
return new KeywordResponse( | ||
keyword.getId(), | ||
keyword.getName(), | ||
keyword.getDescription(), | ||
keyword.getSeq(), | ||
keyword.getImportance(), | ||
totalQuizCount, | ||
doneQuizCount, | ||
keyword.getParentIdOrNull(), | ||
recommendedPostResponses, | ||
createChildrenWithProgress(keyword.getChildren(), quizzesPerKeyword, doneQuizzes) | ||
); | ||
return keywordsResponse; | ||
} | ||
|
||
private int getDoneQuizCount(final Set<Quiz> quizzes, final Set<Quiz> doneQuizzes) { | ||
quizzes.retainAll(doneQuizzes); | ||
return quizzes.size(); | ||
private Map<Long, Integer> getTotalQuizCounts() { | ||
return keywordRepository.findTotalQuizCount().stream() | ||
.collect( | ||
toMap( | ||
KeywordIdAndTotalQuizCount::getKeywordId, | ||
KeywordIdAndTotalQuizCount::getTotalQuizCount)); | ||
} | ||
|
||
private List<KeywordResponse> createChildrenWithProgress(final Set<Keyword> children, | ||
final Map<Keyword, Set<Quiz>> quizzesPerKeyword, | ||
final Set<Quiz> userAnswers) { | ||
return children.stream() | ||
.map(child -> createResponseWithProgress(child, quizzesPerKeyword, userAnswers)) | ||
.sorted(Comparator.comparing(KeywordResponse::getKeywordId)) | ||
.collect(Collectors.toList()); | ||
private Map<Long, Integer> getDoneQuizCounts(final Long memberId) { | ||
return keywordRepository.findDoneQuizCountByMemberId(memberId).stream() | ||
.collect( | ||
toMap( | ||
KeywordIdAndDoneQuizCount::getKeywordId, | ||
KeywordIdAndDoneQuizCount::getDoneQuizCount)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,35 +1,30 @@ | ||
package wooteco.prolog.roadmap.domain.repository; | ||
|
||
import org.springframework.data.jpa.repository.EntityGraph; | ||
import org.springframework.data.jpa.repository.JpaRepository; | ||
import org.springframework.data.jpa.repository.Query; | ||
import org.springframework.data.repository.query.Param; | ||
import wooteco.prolog.roadmap.domain.Keyword; | ||
import wooteco.prolog.roadmap.domain.repository.dto.KeywordIdAndDoneQuizCount; | ||
import wooteco.prolog.roadmap.domain.repository.dto.KeywordIdAndTotalQuizCount; | ||
|
||
import java.util.List; | ||
import java.util.Optional; | ||
import java.util.Set; | ||
|
||
import static org.springframework.data.jpa.repository.EntityGraph.EntityGraphType.FETCH; | ||
|
||
public interface KeywordRepository extends JpaRepository<Keyword, Long> { | ||
|
||
@EntityGraph(attributePaths = "recommendedPosts", type = FETCH) | ||
Optional<Keyword> findById(final Long id); | ||
@Query("SELECT k.id AS keywordId, COUNT (q.id) as totalQuizCount " + | ||
"FROM Keyword k " + | ||
"JOIN Quiz q ON q.keyword.id = k.id " + | ||
"GROUP BY k.id") | ||
List<KeywordIdAndTotalQuizCount> findTotalQuizCount(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이거 처음보는건데, JPQL에서 명시적으로 인터페이스에 관한 구현체를 만들어주는건가요??
어떻게 동작하는건지 처음 봐서 한번 여쭤봅니다!! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 원하는 컬럼만 디비에서 가져오고 싶을 때 dto 프로젝션을 사용합니다! 어떤 역할을 수행하는 객체로 프로젝션하고 싶다면 클래스 기반의 프로젝션, |
||
|
||
@EntityGraph(attributePaths = "recommendedPosts", type = FETCH) | ||
List<Keyword> findAll(); | ||
@Query("SELECT k.id AS keywordId, COUNT (e.id) AS doneQuizCount " + | ||
"FROM Keyword k " + | ||
"JOIN Quiz q ON k.id = q.keyword.id " + | ||
"JOIN EssayAnswer e ON e.quiz.id = q.id " + | ||
"WHERE e.member.id = :memberId " + | ||
"GROUP BY k.id ") | ||
List<KeywordIdAndDoneQuizCount> findDoneQuizCountByMemberId(@Param("memberId") Long memberId); | ||
|
||
@Query("SELECT k FROM Keyword k " | ||
+ "LEFT JOIN FETCH k.children c " | ||
+ "LEFT JOIN FETCH k.recommendedPosts " | ||
+ "LEFT JOIN FETCH k.parent p " | ||
+ "LEFT JOIN FETCH p.recommendedPosts " | ||
+ "LEFT JOIN FETCH c.recommendedPosts " | ||
+ "LEFT JOIN FETCH c.children lc " | ||
+ "LEFT JOIN FETCH lc.recommendedPosts " | ||
+ "LEFT JOIN FETCH lc.children " | ||
+ "WHERE k.id = :keywordId ORDER BY k.seq") | ||
Keyword findFetchByIdOrderBySeq(@Param("keywordId") Long keywordId); | ||
|
||
@Query("SELECT k FROM Keyword k " | ||
|
@@ -42,5 +37,8 @@ public interface KeywordRepository extends JpaRepository<Keyword, Long> { | |
+ "WHERE k.parent IS NULL") | ||
List<Keyword> newFindByParentIsNull(); | ||
|
||
List<Keyword> findBySessionIdIn(final Set<Long> sessionIds); | ||
@Query("SELECT k FROM Keyword k " + | ||
"JOIN Session s ON s.id = k.sessionId " + | ||
"WHERE s.curriculumId = :curriculumId ") | ||
List<Keyword> findAllByCurriculumId(Long curriculumId); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
정적 팩토리 메서드가 하나의 매개변수만 받는 경우 of보다는 from을 사용하는 것이 일반적이라고 합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List 타입이나 가변인자도 from으로 쓰나요??!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음 이건 관점에 따라 달라질 수도 있겠네요...
저의 경우 List타입이라도 매개변수의 개수가 1개라면 from을 사용했었습니다. 가변인자는 정적 팩토리 메서드에서 활용해본 적은 없지만 구현하게 된다면 of를 사용할 것 같아요
이 부분은 연어의 판단에 따르겠습니다!