Conversation
|
Caution Review failedThe pull request is closed. Walkthrough이 PR은 Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Seooooo24
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/command/CreateFormCommand.java (1)
46-56: 코드 중복을 줄일 수 있는 방법을 고려해보세요.
toEntityWithSection메서드가toEntity메서드와 거의 동일한 구현을 가지고 있습니다. 두 메서드의 유일한 차이점은 section 값의 출처입니다. 코드 중복을 줄이기 위해 다음과 같은 접근 방식을 고려해볼 수 있습니다:public FormField toEntity(Form savedForm) { return FormField.builder() .question(question) .fieldType(type) .options(options) .required(required) .fieldOrder(order) .section(section) .form(savedForm) .build(); } public FormField toEntityWithSection(Form savedForm, String section) { - return FormField.builder() - .question(question) - .fieldType(type) - .options(options) - .required(required) - .fieldOrder(order) - .section(section) - .form(savedForm) - .build(); + return FormField.builder() + .question(question) + .fieldType(type) + .options(options) + .required(required) + .fieldOrder(order) + .section(section) + .form(savedForm) + .build(); }또는 private 헬퍼 메서드를 사용하여 빌더 로직을 공통화할 수도 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java (2)
269-282: 다중 섹션 처리 로직이 복잡하니 가독성을 개선해 보세요.
toCreateFormFieldsForMultipleSections메서드에서 사용된 flatMap과 조건문을 포함하는 람다 표현식은 복잡성이 높습니다. 특히 "공통" 섹션을 다른 섹션들에 매핑하는 로직이 한 번에 이해하기 어렵습니다. 다음과 같이 메서드를 분리하여 가독성을 높일 수 있습니다:private List<FormField> toCreateFormFieldsForMultipleSections(Form savedForm, List<CreateFormFieldCommand> createFormFieldCommands) { return createFormFieldCommands.stream() - .flatMap(formFieldCommand -> { - if (formFieldCommand.section().equals("공통")) { - return savedForm.getSections().stream() - .filter(section -> !section.equals("공통")) - .map(section -> formFieldCommand.toEntityWithSection(savedForm, section)); - } else { - return Stream.of(formFieldCommand.toEntityWithSection(savedForm, formFieldCommand.section())); - } - }) + .flatMap(formFieldCommand -> formFieldCommand.section().equals("공통") + ? createCommonSectionFields(savedForm, formFieldCommand) + : Stream.of(formFieldCommand.toEntityWithSection(savedForm, formFieldCommand.section()))) .toList(); } + private Stream<FormField> createCommonSectionFields(Form savedForm, CreateFormFieldCommand command) { + return savedForm.getSections().stream() + .filter(section -> !section.equals("공통")) + .map(section -> command.toEntityWithSection(savedForm, section)); + }또한 "공통" 같은 문자열 상수는 클래스 상수로 선언하여 사용하는 것이 좋습니다.
269-282: "공통" 같은 문자열 상수는 클래스 상수로 정의하는 것이 좋습니다."공통"이라는 문자열이 코드 내에서 하드코딩되어 있습니다. 이런 문자열 상수는 클래스 상수로 정의하여 사용하는 것이 좋습니다. 이렇게 하면 오타 방지 및 향후 변경 시 용이합니다.
+ private static final String COMMON_SECTION = "공통"; private List<FormField> toCreateFormFieldsForMultipleSections(Form savedForm, List<CreateFormFieldCommand> createFormFieldCommands) { return createFormFieldCommands.stream() .flatMap(formFieldCommand -> { - if (formFieldCommand.section().equals("공통")) { + if (formFieldCommand.section().equals(COMMON_SECTION)) { return savedForm.getSections().stream() - .filter(section -> !section.equals("공통")) + .filter(section -> !section.equals(COMMON_SECTION)) .map(section -> formFieldCommand.toEntityWithSection(savedForm, section)); } else { return Stream.of(formFieldCommand.toEntityWithSection(savedForm, formFieldCommand.section())); } }) .toList(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java(3 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/dto/command/CreateFormCommand.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and analyze
🔇 Additional comments (2)
src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java (2)
72-77: 폼 섹션 수에 따른 분기 처리 구현이 잘 되었습니다.폼에 섹션이 여러 개인 경우와 단일 섹션인 경우를 명확하게 구분하여 처리하는 로직이 잘 구현되었습니다. 이 구현은 PR의 목적에 부합하며, 비즈니스 요구사항을 명확하게 반영하고 있습니다.
264-268: 단일 섹션 처리 메서드가 명확하게 구현되었습니다.기존의
toCreateFormFields메서드를 좀 더 명확한 이름인toCreateFormFieldsForSingleSection으로 변경한 점이 좋습니다. 이 메서드는 기존 구현과 동일하게 유지하면서 이름을 통해 그 목적을 더 명확히 전달하고 있습니다.
src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java
Outdated
Show resolved
Hide resolved
Seooooo24
changed the title
Seooooo24
changed the title
src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java
Outdated
Show resolved
Hide resolved
| return createFormFieldCommands.stream() | ||
| .flatMap(formFieldCommand -> { | ||
| if (formFieldCommand.section().equals("공통")) { | ||
| return savedForm.getSections().stream() | ||
| .filter(section -> !section.equals("공통")) | ||
| .map(section -> formFieldCommand.toEntityWithSection(savedForm, section)); | ||
| } else { | ||
| return Stream.of(formFieldCommand.toEntityWithSection(savedForm, formFieldCommand.section())); | ||
| } | ||
| }) | ||
| .toList(); |
There was a problem hiding this comment.
p3)
뎁스가 3인데,
flatMap -> if -> stream
가독성이 떨어져서 최대한 리팩토링 해주시면 좋을 것 같아요.
그리고 "공통"도 상수처리 하면 좋을 것 같네요
There was a problem hiding this comment.
private 메서드 두 개로 나누어보았습니다. 한 번 더 나눌까 생각했지만 그러면 너무 많은 메서드 때문에 오히려 가독성이 떨어질 것 같기도 해서... 어떻게 생각하시는지 말씀해주실 수 있을까요?
상수처리 완료했습니다!
There was a problem hiding this comment.
p3)
현재 코드상으론 Command 객체를 검증 후 FormField entity를 생성하는 로직이어서 FacadeCentralFormServiceImpl에 section을 바탕으로 FormField를 생성하는 로직이 포함된거 같습니다.
제 의견으로는 FacadeCentralFormServiceImpl에서는 FormField를 entity로 변환하고 저장하는 코드만 작성하고 FormField에서 section을 활용한 로직을 작성해 FormField를 생성하는게 좋아보입니다.
구체적으로
FacadeCentralFormServiceImpl에서 Command 객체를 요청 상태 그대로 entity로 변환- 변환된
FormFieldentity 내부에서 section의 개수 및 공통 여부를 체크하여 조건에 맞을 경우FormField객체를 각 섹션별로 복제 및 반환 FacadeCentralFormServiceImpl복제된FormField리스트를 저장
해당 흐름으로 작성되면 대부분의 로직을 FormField 내부에 작성할 수 있을 거 같습니다.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/ddingdong/ddingdongBE/domain/form/entity/FormField.java (1)
103-113: 개별 섹션 복사 메서드
copyWithSection(String newSection)를 통해 섹션만 다른 FormField 인스턴스를 생성해 재사용할 수 있습니다. 공통 속성은 그대로 복사하고 섹션만 변경하도록 구현되어 있어 섹션 확장 로직에 적합합니다. 다만, 새로운 FormField 생성 시 반드시 동일한Form참조가 필요한지 검토가 필요할 수 있습니다.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/ddingdong/ddingdongBE/domain/form/entity/FormField.java(2 hunks)src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/ddingdong/ddingdongBE/domain/form/service/FacadeCentralFormServiceImpl.java
🔇 Additional comments (5)
src/main/java/ddingdong/ddingdongBE/domain/form/entity/FormField.java (5)
16-16: 스트림 임포트 추가 확인
해당 스트림 임포트는 다중 섹션 처리 시 유연하게 FormField 객체를 생성하고 반환하기 위해 필요한 것으로 보입니다. 문제 없습니다.
26-27: 상수 선언의 명확성
COMMON_SECTION과SINGLE_SECTION_SIZE라는 상수를 통해 섹션 로직을 직접적으로 나타내므로 가독성이 좋습니다. 추가로, 상수명은 비즈니스 의도를 간결하게 표현하고 있어 유지 보수에 용이합니다.
29-72: 필드 정의와 빌더 패턴 적용
필드가 역할에 맞게 잘 구분되어 있으며, 빌더 패턴을 통해 객체 생성을 명확하게 처리하고 있습니다. 기존BaseEntity를 상속받고 있으므로, 엔터티 전반의 일관성이 잘 유지됩니다. 특별한 문제 없이 적절해 보입니다.
87-92: 복수 섹션 처리 로직
form.isLargerSectionThan(SINGLE_SECTION_SIZE)조건을 통해 섹션의 개수를 판별하고, 필요한 경우expandCommonSectionFormField를 호출하는 흐름이 직관적입니다. 섹션이 1개인 경우 단순화된 로직을 사용해 반환하므로, 개념적으로 분기 처리가 깔끔합니다.
94-101: 공통 섹션 분기 로직
COMMON_SECTION인지 확인하고, 해당 섹션을 제외한 나머지 섹션에 대해서만copyWithSection을 호출하여 새로운 FormField 스트림을 생성합니다. 다중 섹션 시 공통 필드가 올바르게 확장되도록 의도된 로직으로 보이며, 책임이 명확하게 분리되어 있습니다.
| .map(formFieldCommand -> formFieldCommand.toEntity(savedForm)) | ||
| .toList(); |
There was a problem hiding this comment.
p4)
| .map(formFieldCommand -> formFieldCommand.toEntity(savedForm)) | |
| .toList(); | |
| .map(formFieldCommand -> formFieldCommand.toEntity(savedForm)) | |
| .flatMap(formField -> formField.generateFormFieldsBySection(form)) | |
| .toList(); |
createForm() 메서드의 스트림과 합쳐도 괜찮을거 같습니다!
|
(cherry picked from commit 64edf8b)
3 participants
🚀 작업 내용
섹션 여러 개일 경우 폼지 생성 기능 수정하였습니다.
form의 sections 사이즈가 1보다 클 경우, 아닐 경우로 분기하고 그에 따른 private 메서드 toCreateFormFieldsForMultipleSections, toCreateFormFieldsForSingleSection을 각각 만들었습니다.
유저 폼지 조회도 수정하도록 하겠습니다.
🤔 고민했던 내용
분기를 private 메서드 외부에서 할지 내부에서 할지, private 메서드는 어떤 기준으로 분리해야 할지 고민되었습니다.
달리 선택지가 없어서 그대로 두긴 했지만 메서드명이 너무 긴 것 같다는 생각이 들었습니다.
람다식 내부에 if-else문도 있고... 내부 로직이 조금 난잡하다는 생각도 들었습니다.
그리고 이 로직이 Facade에 있는 게 맞는지 잘 모르겠습니다. FormStatistics처럼 따로 빼는 건 좀 이상하겠죠..??
💬 리뷰 중점사항
Summary by CodeRabbit