Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 9 commits into from
Nov 22, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dc4fae9
feat: 유저 ID로 답변 개수 조회 기능 구현
nuyh99 Nov 1, 2023
11e87dc
test: 유저 ID로 답변 개수 조회 기능 테스트
nuyh99 Nov 1, 2023
bc41ccb
test: 커리큘럼의 모든 키워드 조회 테스트
nuyh99 Nov 1, 2023
5faa0fb
refactor: 커리큘럼 ID의 모든 키워드 가져오는 로직 JPQL 사용해서 리팩토링
nuyh99 Nov 1, 2023
746ee8c
refactor: Dto -> Map 변환 로직 Stream API 사용
nuyh99 Nov 1, 2023
4eac034
refactor: 키워드 조회 시 추천 포스트 fetch join -> batch size 사용
nuyh99 Nov 1, 2023
c730607
style: doneQuizCount -> answeredQuizCount로 용어 변경
nuyh99 Nov 1, 2023
d94a493
style: 정적 팩토리 메서드 네이밍 변경
nuyh99 Nov 22, 2023
e9cc339
refactor: 진도율 setter 제거 후 dto 내부에서 매핑하도록 변경
nuyh99 Nov 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions backend/src/main/java/wooteco/prolog/roadmap/application/KeywordService.java
View file
Open in desktop
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;
Expand All @@ -15,6 +11,12 @@
import wooteco.prolog.roadmap.domain.repository.KeywordRepository;
import wooteco.prolog.session.domain.repository.SessionRepository;

import java.util.List;

import static java.util.Collections.emptyMap;
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 {
Expand All @@ -23,7 +25,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;
}
Expand Down Expand Up @@ -65,7 +67,7 @@ public KeywordResponse findKeywordWithAllChild(final Long sessionId, final Long

Keyword keyword = keywordRepository.findFetchByIdOrderBySeq(keywordId);

return KeywordResponse.createWithAllChildResponse(keyword);
return KeywordResponse.createWithAllChildResponse(keyword, emptyMap(), emptyMap());
}

@Transactional(readOnly = true)
Expand All @@ -74,7 +76,7 @@ public KeywordResponse newFindKeywordWithAllChild(final Long keywordId) {

Keyword keyword = keywordRepository.findFetchByIdOrderBySeq(keywordId);

return KeywordResponse.createWithAllChildResponse(keyword);
return KeywordResponse.createWithAllChildResponse(keyword, emptyMap(), emptyMap());
}

@Transactional(readOnly = true)
Expand All @@ -83,14 +85,14 @@ public KeywordsResponse findSessionIncludeRootKeywords(final Long sessionId) {

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

return KeywordsResponse.createResponse(keywords);
return KeywordsResponse.of(keywords, emptyMap(), emptyMap());
}

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

return KeywordsResponse.createResponse(keywords);
return KeywordsResponse.of(keywords, emptyMap(), emptyMap());
}

public void updateKeyword(
Expand Down
115 changes: 20 additions & 95 deletions backend/src/main/java/wooteco/prolog/roadmap/application/RoadMapService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,122 +1,47 @@
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.KeywordIdAndAnsweredQuizCount;
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 클래스 레벨에 rw 트랜잭션
  2. 클래스 레벨에 r 트랜잭션
  3. 메서드 레벨로만 rw 또는 r 트랜잭션

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

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

@Service
public class RoadMapService {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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> answeredQuizCounts = getAnsweredQuizCounts(memberId);

final List<Keyword> keywordsInCurriculum = getKeywordsInCurriculum(curriculum);

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.of(keywords, totalQuizCounts, answeredQuizCounts);
}

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> getAnsweredQuizCounts(final Long memberId) {
return keywordRepository.findAnsweredQuizCountByMemberId(memberId).stream()
.collect(
toMap(
KeywordIdAndAnsweredQuizCount::getKeywordId,
KeywordIdAndAnsweredQuizCount::getAnsweredQuizCount));
}
}
46 changes: 24 additions & 22 deletions backend/src/main/java/wooteco/prolog/roadmap/application/dto/KeywordResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
package wooteco.prolog.roadmap.application.dto;

import java.util.ArrayList;
import lombok.AccessLevel;
import lombok.Getter;
import lombok.NoArgsConstructor;
import wooteco.prolog.roadmap.domain.Keyword;

import java.util.HashSet;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

Expand All @@ -21,14 +21,14 @@ public class KeywordResponse {
private int order;
private int importance;
private int totalQuizCount;
private int doneQuizCount;
private int answeredQuizCount;
private Long parentKeywordId;
private List<RecommendedPostResponse> recommendedPosts;
private List<KeywordResponse> childrenKeywords;

public KeywordResponse(final Long keywordId, final String name, final String description,
final int order, final int importance, final int totalQuizCount,
final int doneQuizCount, final Long parentKeywordId,
final int answeredQuizCount, final Long parentKeywordId,
final List<RecommendedPostResponse> recommendedPosts,
final List<KeywordResponse> childrenKeywords) {
this.keywordId = keywordId;
Expand All @@ -37,7 +37,7 @@ public KeywordResponse(final Long keywordId, final String name, final String des
this.order = order;
this.importance = importance;
this.totalQuizCount = totalQuizCount;
this.doneQuizCount = doneQuizCount;
this.answeredQuizCount = answeredQuizCount;
this.parentKeywordId = parentKeywordId;
this.recommendedPosts = recommendedPosts;
this.childrenKeywords = childrenKeywords;
Expand All @@ -53,34 +53,36 @@ public static KeywordResponse createResponse(final Keyword keyword) {
0, 0,
keyword.getParentIdOrNull(),
createRecommendedPostResponses(keyword),
null);
Collections.emptyList());
}

private static List<RecommendedPostResponse> createRecommendedPostResponses(final Keyword keyword) {
return keyword.getRecommendedPosts().stream()
.map(RecommendedPostResponse::from)
.collect(Collectors.toList());
}

public static KeywordResponse createWithAllChildResponse(final Keyword keyword) {
public static KeywordResponse createWithAllChildResponse(final Keyword keyword,
final Map<Long, Integer> totalQuizCounts,
final Map<Long, Integer> answeredQuizCounts) {
return new KeywordResponse(
keyword.getId(),
keyword.getName(),
keyword.getDescription(),
keyword.getSeq(),
keyword.getImportance(),
0,
0,
totalQuizCounts.getOrDefault(keyword.getId(), 0),
answeredQuizCounts.getOrDefault(keyword.getId(), 0),
keyword.getParentIdOrNull(),
createRecommendedPostResponses(keyword),
createChildren(keyword.getChildren()));
createChildren(keyword.getChildren(), totalQuizCounts, answeredQuizCounts));
}

private static List<KeywordResponse> createChildren(final Set<Keyword> children) {
List<KeywordResponse> keywords = new ArrayList<>();
for (Keyword keyword : children) {
keywords.add(createWithAllChildResponse(keyword));
}
return keywords;
private static List<RecommendedPostResponse> createRecommendedPostResponses(final Keyword keyword) {
return keyword.getRecommendedPosts().stream()
.map(RecommendedPostResponse::from)
.collect(Collectors.toList());
}

private static List<KeywordResponse> createChildren(final Set<Keyword> children,
final Map<Long, Integer> totalQuizCounts,
final Map<Long, Integer> answeredQuizCounts) {
return children.stream()
.map(keyword -> createWithAllChildResponse(keyword, totalQuizCounts, answeredQuizCounts))
.collect(Collectors.toList());
}
}
22 changes: 10 additions & 12 deletions backend/src/main/java/wooteco/prolog/roadmap/application/dto/KeywordsResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
package wooteco.prolog.roadmap.application.dto;

import java.util.List;
import java.util.stream.Collectors;
import lombok.AccessLevel;
import lombok.Getter;
import lombok.NoArgsConstructor;
import wooteco.prolog.roadmap.domain.Keyword;

import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

@NoArgsConstructor(access = AccessLevel.PROTECTED)
@Getter
public class KeywordsResponse {
Expand All @@ -17,18 +19,14 @@ public KeywordsResponse(final List<KeywordResponse> data) {
this.data = data;
}

public static KeywordsResponse createResponse(final List<Keyword> keywords) {
List<KeywordResponse> keywordsResponse = keywords.stream()
.map(KeywordResponse::createResponse)
.collect(Collectors.toList());
return new KeywordsResponse(keywordsResponse);
}

public static KeywordsResponse createResponseWithChildren(final List<Keyword> keywords) {
List<KeywordResponse> keywordsResponse = keywords.stream()
public static KeywordsResponse of(final List<Keyword> keywords,
final Map<Long, Integer> totalQuizCounts,
final Map<Long, Integer> answeredQuizCounts) {
final List<KeywordResponse> keywordsResponse = keywords.stream()
.filter(Keyword::isRoot)
.map(KeywordResponse::createWithAllChildResponse)
.map(rootKeyword -> KeywordResponse.createWithAllChildResponse(rootKeyword, totalQuizCounts, answeredQuizCounts))
.collect(Collectors.toList());

return new KeywordsResponse(keywordsResponse);
}
}
17 changes: 13 additions & 4 deletions backend/src/main/java/wooteco/prolog/roadmap/domain/Keyword.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,16 @@
import org.hibernate.annotations.BatchSize;
import wooteco.prolog.common.exception.BadRequestException;

import javax.persistence.*;
import javax.persistence.CascadeType;
import javax.persistence.Column;
import javax.persistence.Entity;
import javax.persistence.FetchType;
import javax.persistence.GeneratedValue;
import javax.persistence.GenerationType;
import javax.persistence.Id;
import javax.persistence.JoinColumn;
import javax.persistence.ManyToOne;
import javax.persistence.OneToMany;
import java.util.HashSet;
import java.util.Objects;
import java.util.Set;
Expand Down Expand Up @@ -38,6 +47,7 @@ public class Keyword {
@Column(name = "session_id", nullable = false)
private Long sessionId;

@BatchSize(size = 1000)
@OneToMany(mappedBy = "keyword", cascade = CascadeType.ALL, orphanRemoval = true)
private Set<RecommendedPost> recommendedPosts = new HashSet<>();

Expand All @@ -50,8 +60,7 @@ public class Keyword {
private Set<Keyword> children = new HashSet<>();

public Keyword(final Long id, final String name, final String description, final int seq,
final int importance,
final Long sessionId, final Keyword parent, final Set<Keyword> children) {
final int importance, final Long sessionId, final Keyword parent, final Set<Keyword> children) {
validateSeq(seq);
this.id = id;
this.name = name;
Expand All @@ -69,7 +78,7 @@ public static Keyword createKeyword(final String name,
final int importance,
final Long sessionId,
final Keyword parent) {
return new Keyword(null, name, description, seq, importance, sessionId, parent, null);
return new Keyword(null, name, description, seq, importance, sessionId, parent, new HashSet<>());
}

public void update(final String name, final String description, final int seq,
Expand Down
Loading
Loading