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.

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

체크리스트 방 정보 작성 API를 구현한다. #75

Merged
merged 50 commits into from
Jul 24, 2024

Conversation

shin-jisong
Copy link
Contributor

❗ Issue

✨ 구현한 기능

  • 체크리스트 방 정보 작성 기능

📢 논의하고 싶은 내용

없음

🎸 기타

분량이 많습니다! 읽고 자유롭게 질문해 주세요

shin-jisong and others added 30 commits July 22, 2024 14:59
Co-Authored-By: tkdgur0906 <[email protected]>
Co-Authored-By: tkdgur0906 <[email protected]>
Comment on lines 7 to 10
@Component
public class QuestionList {

public Map<Integer, Question> questions;
Copy link
Contributor

Choose a reason for hiding this comment

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

QuestionList를 enum으로 구현하지 않은 이유가 있으신가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

첫 번째로, ENUM으로 추가할 경우 불필요한 네이밍 리소스가 발생하며 30개가 넘는 질문을 다 작성하는 것이 불필요하다는 판단이 있었습니다
두 번째로, Question 내부의 필드들을 stream 등을 통하여 필터링하거나 정렬해야 하는데 이것이 ENUM의 의도와 부합하지 않는다 판단하였으며 실제 해당 로직을 구현하는 것도 매우 어렵게 되어있습니다
따라서 enum이 아닌 map으로 구현하였습니다

Copy link
Contributor

Choose a reason for hiding this comment

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

일단 Map으로 진행후 필요성이 느껴지면 Enum으로 변경

Comment on lines +8 to +13
public record ChecklistCreateRequest(@Valid RoomCreateRequest room, List<Integer> options,
@Valid List<QuestionCreateRequest> questions) {

public Room toRoomEntity() {
return room.toRoomEntity();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

RoomCreateRequest의 변수명을 room이라고 하니, domain 객체인 Room과 헷갈리는 느낌이 드네요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

API 문서에 따라 작성하였는데 API 문서를 수정해야 할까요?
외부에서 저희 서비스를 바라볼 때 지칭하는 room이 저희가 정의한 도메인 room과는 어느 정도 개념적 차이가 있을 수 있겠지만 해당 DTO는 외부와 소통해야 하기 때문에 외부에서 사용하는 네이밍을 사용하여 room으로 두어도 괜찮을 것 같습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

찾아보고 없으면 API 문서대로 진행

Copy link
Contributor

@JINU-CHANG JINU-CHANG left a comment

Choose a reason for hiding this comment

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

고생많으셨습니다 🥺 첫 API인데 작업량이 엄청나네요
1차 리뷰 남겨뒀으니 확인부탁드려요 리뷰분량이 많아서 끊어서 남길게요~

@Transactional
public long createChecklist(ChecklistCreateRequest checklistCreateRequest) {
Room room = roomRepository.save(checklistCreateRequest.toRoomEntity());
ChecklistInfo checklistInfo = checklistCreateRequest.toChecklistInfo();
Copy link
Contributor

Choose a reason for hiding this comment

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

  • ChecklistInfo와 Checklist가 구분이 안되는데, Info라는 네이밍대신 DTO임을 명시해주는 건 어떨까요?
  • ChecklistCreateRequest.toChecklistEntity로 바로 Checklist 엔티티 생성해도 좋을 것 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ChecklistInfo와 Checklist가 구분이 안되는데, Info라는 네이밍대신 DTO임을 명시해주는 건 어떨까요?

현재 DTO라는 명명을 직접적으로 사용하지 않고 있기 때문에 통일성을 지키기 위해 사용하지 않았습니다.
request, response를 제외하고 DTO 네이밍을 사용하지 않고 있는데, 이런 경우에는 DTO를 직접 명시해도 될까요?

ChecklistCreateRequest.toChecklistEntity로 바로 Checklist 엔티티 생성해도 좋을 것 같아요!

말씀하신 방식으로 구현하기 위해서는 DTO에 추가적인 정보로 도메인을 넘겨야 하기 때문에 지양하고자 하였습니다.

다른 방식으로 user와 room을 null로 채우고 checklist를 생성한 후에 setter로 주입하는 방식이 있었는데, 이 방식보다는 현재의 방식인 DTO에서 ChecklistInfo를 반환한 뒤에 Checklist를 직접 생성하는 방식이 적합하다 생각하여 구현했습니다!

for (QuestionCreateRequest questionCreateRequest : questions) {
Integer questionId = questionCreateRequest.questionId();
validateQuestion(questionId);
ChecklistQuestion checklistQuestion = new ChecklistQuestion(checklist, questionId,
Copy link
Contributor

Choose a reason for hiding this comment

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

questionId, answer를 필드로 가지는 QuestionAnswer 라는 도메인을 만드는건 어떨까요? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

현재로써는 필요성이 와닿지 않아 추후에 다시 고민해보겠습니다!

shin-jisong and others added 16 commits July 24, 2024 14:01
@JINU-CHANG
Copy link
Contributor

필요한 부분 모두 반영되어 approve합니다 👍

@tsulocalize tsulocalize merged commit 837a02d into dev-be Jul 24, 2024
1 check passed
@tsulocalize tsulocalize deleted the feat/47-write-checklist-api branch July 24, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants