-
Notifications
You must be signed in to change notification settings - Fork 3
[BE] refactor: 구조화된 템플릿 사용 #980
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
base: develop
Are you sure you want to change the base?
Conversation
중간에 워딩 하나 바꾼게 있어서 여러 패키지를 좀 건드렸어요. 참고 부탁합니다. (checkBox -> checkbox로 통일했습니다) |
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.
이해하면서 한 번 끊고 자세한 건 다음 코멘트 때 물어볼게요!
} | ||
} | ||
|
||
public List<Long> getSectionIds() { |
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.
이쪽도 N+1 주의해야 할 거예요
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.
지금 동작으로는 EAGER 로딩 시, 자동으로 JOIN 사용해서 N+1은 발생하지 않아요.
대신 Section을 로딩할 때 N+1이 발생했는데요. 이 부분은 FETCH JOIN을 사용하면 해결되긴 합니다.
별개로 이번 PR이 머지되고 이제 다른 곳에서 StructuredTemplate
을 사용하게 되면, 템플릿 자원들을 DB 호출하는 곳은 StructuredTemplateService
에서 구조화된 템플릿을 만들 때 밖에 없습니다.
그래서 템플릿, 섹션, 질문, 옵션그룹, 옵션아이템을 가져올 때 필요한 것들을 한번에 즉시 로딩 해도 상관없다 생각이 들어요. 이렇게 되면 직접 작성한 쿼리에는 모두 FETCH JOIN 붙이고 잘 동작되는지 확인이 필요합니다.
만약 보수적으로 접근하고 싶다면, LAZY 하고 @Fetch(FetchMode.JOIN)
과 FETCH JOIN을 병행하는 방법이 있습니다.
저는 둘 다 상관없긴 합니다. 우선은 코드 수정이 적은 즉시로딩에 FETCH JOIN을 붙이는 쪽으로 수정할게요, 의견 주세용
} | ||
} | ||
|
||
private void validateNoDuplicates(List<Long> sectionIds) { |
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.
긍정문을 활용해서 validateUniqueSectionIds
?
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.
둘 다 긍정문 아닌가요?? 헷갈리네..
메서드명이 조금 직관적이였으면 해서 이 부분은 다른 분들 의견 반영해서 나머지도 통일할게요~
@nayonsoso @skylar1220
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.
둘 다 긍정문 아닌가요?
validateNoDuplicates 는 "중복이 없는지 검증"이고
validateUniqueSectionIds 는 "값이 유일한지 검증"이므로
중복이 '없는지'가 부정의 의미를 담고있다는 말 같아요😊
저는 두 의견 다 일리가 있다고 생각되는데, 중복이 없는것을 검사하는게 더 의미를 강조한다고 느껴져서 validateNoDuplicates 에 한표요!
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.
저도 validateNoDuplicates가 행위와 일치해서 이해가 쉽네요~ 한표요!
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.
고럼 메서드명은 그대로 가겠슴다
import reviewme.template.domain.OptionGroup; | ||
import reviewme.template.domain.exception.OptionGroupNotExistException; | ||
|
||
@Getter |
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.
필요한가요 ? 다른 클래스에서는 없길래 질문합니당
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.
안쓰는데 뺀다는걸 빼먹었슴다 반완
this.template = template; | ||
this.sections = sections; | ||
this.questions = questions; | ||
this.optionGroups = null; | ||
this.optionItems = null; |
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.
생성자 체이닝해주세요! this
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.
반영했스니다!
|
||
public class StructuredTemplateCreator { | ||
|
||
public StructuredTemplate create(Template template, List<Section> sectionList, List<Question> questionList, |
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
접두어 대신 s
복수형으로 가시죠! (변수명이 타입에 의존함)
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.
Sections sections = new Sections(sectionList);
Questions questions = new Questions(questionList);
OptionGroups optionGroups = new OptionGroups(optionGroupList);
OptionItems optionItems = new OptionItems(optionItemList);
요 부분에서 네이밍이 겹쳐서 저렇게 해놓긴 했는데,, 아이디어 없을까요
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.
Sections, Questions와 같이 일급 컬렉션이 외부에서 사용되지 않는다면, 외부에서는 -s
로 끝나는 게 Collection이라 List로 넣는다는 게 자연스러울 것 같긴 해요
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.
요 부분 내부 로직이 변경되면서 이 코드가 사라졌습니다~
@Slf4j | ||
public class StructuredTemplateInvisibleSectionFoundException extends BadRequestException { | ||
|
||
private static final String ERROR_MESSAGE = "서버 내부에서 문제가 발생했어요. 서버에 문의해주세요."; |
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.
INTERNAL_SERVER_ERROR_MESSAGE
?
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.
반완!
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.
테테테드 왕건이 rc 드립니다~
@@ -53,10 +53,10 @@ public void setup() { | |||
long growthOptionId = optionItemRepository.save(new OptionItem("🌱성장 마인드셋 (예: 새로운 분야나 잘 모르는 분야에 도전하는 마음, 꾸준한 노력으로 프로젝트 이전보다 성장하는 모습)",categoryOptionGroupId,5, OptionType.CATEGORY )).getId(); | |||
|
|||
// 커뮤니케이션 능력 섹션 | |||
long checkBoxCommunicationQuestionId = questionRepository.save(new Question(true, QuestionType.CHECKBOX, "커뮤니케이션, 협업 능력에서 어떤 부분이 인상 깊었는지 선택해주세요.", null, 1)).getId(); | |||
long checkboxCommunicationQuestionId = questionRepository.save(new Question(true, QuestionType.CHECKBOX, "커뮤니케이션, 협업 능력에서 어떤 부분이 인상 깊었는지 선택해주세요.", null, 1)).getId(); |
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.
checkbox 통일 좋네요 👍
} | ||
} | ||
|
||
private void validateNoDuplicates(List<Long> sectionIds) { |
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.
둘 다 긍정문 아닌가요?
validateNoDuplicates 는 "중복이 없는지 검증"이고
validateUniqueSectionIds 는 "값이 유일한지 검증"이므로
중복이 '없는지'가 부정의 의미를 담고있다는 말 같아요😊
저는 두 의견 다 일리가 있다고 생각되는데, 중복이 없는것을 검사하는게 더 의미를 강조한다고 느껴져서 validateNoDuplicates 에 한표요!
|
||
public DuplicateQuestionIdException(List<Long> questionIds) { | ||
super("섹션에는 중복된 질문이 있을 수 없어요."); | ||
log.info("Duplicate question ID found during create Section - questionIds: {}", questionIds); |
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.
[사소]
문법적으로 during 뒤에는 v-ing 가 와야한다고 알고 있어요,
그래서 during create → while creating 으로 변경하면 좋겠네요!
그리고 "while + 현재분사(-ing)" 구조가 동작이 진행되는 동안 발생한 사건을 표현할 때 더 자연스럽게 사용된다고 하네요 ㅎㅎ
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.
문법 고수 산초 다른 예외도 다 반영했습니다~
StructuredTemplate(Template template, Sections sections, Questions questions, OptionGroups optionGroups, | ||
OptionItems optionItems) { | ||
this.template = template; | ||
this.sections = sections; | ||
this.questions = questions; | ||
this.optionGroups = optionGroups; | ||
this.optionItems = optionItems; | ||
} | ||
|
||
StructuredTemplate(Template template, Sections sections, Questions questions) { | ||
this(template, sections, questions, null, null); | ||
} | ||
|
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.
생성자 체이닝을 할 때는 기본 생성자(모든 파리미터를 가진 생성자)를 가장 아래에 쓴다고 알고 있어요, '함수는 호출 순서대로' 나열하는 것이 기본이기 때문에 그렇다고 하네요!
예를 들어 a 함수가 b 함수를 호출하면 a -> b 순으로 나열하는 것처럼요~
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.
요..건 가장 위에 어떤 걸 쓰는지가 나타나야 한다고 생각해요
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.
두 의견 다 공감이 되는 의견이라 고민되네요.. 커비 의견도 들려주십쇼 @skylar1220
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.
생성자는 논외로 가는 게 맞더고 생각합니다, 어떤 필드가 어떻게 채워지는지가 가장 위에 나와있어야 하지 않을까요?
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.
저도 기본 생성자가 가장 위에있는 것이 더 잘읽혀요! 기본이 아닌 생성자들이 위로 가게되면 쭉 읽으면서 '그래서 기본 생성자는 어디있지?'하고 찾아야할 것 같아서요!
public class StructuredTemplateCreator { | ||
|
||
public StructuredTemplate create(Template template, List<Section> sectionList, List<Question> questionList, | ||
List<OptionGroup> optionGroupList, List<OptionItem> optionItemList) { |
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.
StructuredTemplateCreator 에 대해 대왕 피드백이 있습니다🤴🏻
StructuredTemplateCreator를 빈으로 등록하고,
레포지토리들을 주입받아서 이 안에서 StructuredTemplate 을 만들었으면 합니다.
'템플릿 생성에 필요한 재료를 찾는 것이 StructuredTemplateService 의 역할일까?' 하면 아니라고 생각합니다.
테드가 적은 지난 디스크립션에 저도 공감을 했는데, 그 의도랑 현재 코드가 맞지 않는 것 같습니다.
제가 이해한 바로는, StructuredTemplate 를 조회를 위한 VO로 사용을 하되,
이 객체가 조회와 관련된 모든 일을 하기에는 책임이 무거워지므로
이를 분산하기 위해서 객체를 분리한다고 했었던 것 같아요.
그리고 그 의도로 탄생한 것이 StructuredTemplateService 맞죠?
또한 이 VO 를 생성하는 로직도 복잡하므로 이를 생성하는 책임은 StructuredTemplateCreator 로 분산했다고 생각했습니다.
이런 책임들을 생각한다면, StructuredTemplateCreator가 long templateId
를 인자로 받고,
StructuredTemplate 생성에 필요한 재료들을 찾아와서 직접 생성을 하는 것이 적절하다 생각합니다. StructuredTemplateService 에는 비지니스 로직이 남긴 조회가 들어가고요.
지금의 코드에서는 생성 책임이 분산되었다 생각합니다.
그리고 이것에 이어서, 저는 StructuredTemplateCreator 내부의 검증 중 대부분이 없어도 된다고 생각합니다.
만약 제가 위에 코멘트한대로 StructuredTemplateCreator 가 repository 에서 직접 엔티티를 조회해온다면
정합성이 보장이 된 데이터 들일 것입니다. 따라서 추가적인 정합성 검증은 거치지 않아도 된다고 생각합니다.
이것이 왕건이 RC가 될 것 같아 조심스러운데요... 여기에 대한 테드의 생각도 듣고 싶습니당~
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.
오우 놓치고 있던 부분 같아요. 의견 고맙습니다
StructuredTemplateCreator에서 repo들을 가지고 templateId에 대해 StructuredTemplate 을 만들어서 반환하는 것으로 했어요.
두번째 의견에 대해서 생각하다 보니, 현재는 템플릿 등록 로직이 없어서 템플릿의 구조적인 부분에 대한 검증은 진행하지 않고 있어요. 그래서 무조건 정합성에 맞다고는 볼 수 없다고 생각해요(템플릿-섹션-질문 까지는 괜찮으나 옵션그룹 부터는 id참조가 반대 방향이라 잘못된 구조가 만들어질 수 있어요). 그래서 StructuredTemplateCreator에서 이 부분을 보완할 수 있을까 했는데, 이 클래스의 책임은 조회를 위해서 저장된 자원들로 StructuredTemplate을 만드는 것이니 여기에 검증 로직을 넣는 것 보단,
추후 템플릿 등록 로직이 추가되면StructuredTemplate을 통해서 템플릿을 만들고, 구조적인 부분의 검증이 진행된 후에 자원을 저장하는 방식이 맞다고 생각했어요. 따라서 이 둘을 구분하기 위해서 클래스명을 변경했습니다. (StructuredTemplateCreator -> StructuredTemplateFinder)
- StructuredTemplateCreator : Component, 내부 repo를 가지고 템플릿 구조적인 부분을 검증 후 자원을 저장
- StructuredTemplateFinder : Component, 내부 repo를 가지고 단순히 템플릿 id에 해당하는 자원을 꺼내서 반환
이렇게 가면 좋을 듯 해서 네이밍을 변경했습니다. 덕분에 상당히 많은 코드가 줄었어요~ 👍
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.
현재는 템플릿 등록 로직이 없어서 템플릿의 구조적인 부분에 대한 검증은 진행하지 않고 있어요.
그래서 무조건 정합성에 맞다고는 볼 수 없다고 생각해요
저도 여기에는 동의합니다😅
그리고 StructuredTemplateCreator → StructuredTemplateFinder의 변경도 적절해보이네요!
public class Sections { | ||
|
||
private final List<Section> sections; | ||
|
||
public Sections(List<Section> sections) { | ||
validateSections(sections); | ||
this.sections = sections.stream() | ||
.distinct() | ||
.toList(); | ||
} |
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.
사실 이런 일급 컬렉션에 대해서 처음에는 '이렇게까지 할 필요가 있나?!' 싶었는데,
나중에 코드가 붙으면 매우 유용하게 사용될 것 같아 칭찬 스티커 하나 드립니다👍
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.
간단한 거 먼저 하고 마저 이어서 할게요!
|
||
private void validateNoDuplicates(List<Long> questionIds) { | ||
int originalSize = questionIds.size(); | ||
int deduplicatedSize = new HashSet<>(questionIds).size(); |
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.
중복을 제거한 후의 수라는 뜻의 변수명은 어떨까요?
예: distinctSize
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.
dedup이 그 의미입니당~
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.
아루의 코멘트 정답
} | ||
} | ||
|
||
private void validateNoDuplicates(List<Long> sectionIds) { |
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.
저도 validateNoDuplicates가 행위와 일치해서 이해가 쉽네요~ 한표요!
|
||
public OptionGroupNotExistException() { | ||
super("서버 내부에서 문제가 발생했어요. 서버에 문의해주세요."); | ||
log.info("Not exist OptionGroup during create OptionGroups"); |
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.
문법상 Not exist이 동사의 부정형을 나타내는 문법상 맞지 않아서 보완하면 좋을 것 같아요.
during은 명사(동명사x)와 함께 쓰는 걸로 알고있어서 while로 바꾸는 것도 함께 제안해요!
예: OptionGroup does not exist while creating OptionGroups
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.
메세지들을 변경했으나, 산초의 코멘트 반영으로 예외 클래스가 사용되지 않습니다요 😢
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.
이 예외는 최근 커밋에서 StructuredTemplateFinder가 추가되었고, 그 안의 OptionGroups를 생성하면서 쓰이는 예외네요~
현재는 템플릿 등록 로직이 없어서 템플릿의 구조적인 부분에 대한 검증은 진행하지 않고 있어요. 그래서 무조건 정합성에 맞다고는 볼 수 없다고 생각해요
위의 산초와의 코멘트에서 테드가 언급한 이 부분을 보고 repo에서 가져온 요소들에 대해서도 정합성 검증(없는 요소일 수 있음)을 하기위해 이 예외는 그대로 있겠구나 생각했는데 아닐까요?
logWarning(invisibleSectionIds, null); | ||
} | ||
|
||
private void logWarning(List<Long> invisibleSectionIds, List<Long> optionIds) { |
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.
메서드 분리를 해야하는 이유가 궁금해요~
이 로직이 각 생성자 안에 있어도 바로 읽힐 거라고 생각해서요!
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.
상동입니다..
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.
테드 어프로브 점 쪼금 더 궁금한 거 추가 질문 남겼어요!
@@ -0,0 +1,41 @@ | |||
package reviewme.template.domain.structured; |
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.
이 클래스가 structuredTemplateService의 조회용 하위서비스라고 느껴져요. 그래서 도메인이 아니라 서비스가 되는 것이 더 적당하다고 생각하는데 테드의 생각이 궁금합니다~
public long getTemplateId() { | ||
return template.getId(); | ||
} |
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.
추후 프로덕션 코드에서 쓰일 일이 있을까요?
현재는 테스트에서만 쓰이고 있어서요~ 다른곳의 get~Ids 메서드들에 대해서도 동일합니다!
🚀 어떤 기능을 구현했나요 ?
🔥 어떻게 해결했나요 ?
📝 어떤 부분에 집중해서 리뷰해야 할까요?
📚 참고 자료, 할 말