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

[BE] 체크리스트 리스트 조회 API를 리팩토링한다. #618

Merged
merged 7 commits into from
Sep 24, 2024

Conversation

JINU-CHANG
Copy link
Contributor

❗ Issue

✨ 구현한 기능

  • 체크리스트 리스트 조회메서드 서비스 분리하고 이와 관련된 테스트 코드 보충했습니다.
  • ChecklistFixture 내부 숫자 리터럴 Enum에서 id값 가져오도록 변경했습니다.

📢 논의하고 싶은 내용

  • updateChecklistById vs updateChecklist 서비스 메서드 네이밍 통일이 필요한 것 같습니다.
  • ChecklistListService에 hasUserLikedChecklist() 메서드가 있는데 더 좋은 네이밍 추천받습니다 😅

🎸 기타

  • ChecklistLike 서비스가 아직 리팩토링되지 않은 상태에서 임시로 작업해두었습니다. 카피 PR 머지되면 충돌날 것 같은데 이때 수정해보겠습니다.

Copy link
Contributor

@shin-jisong shin-jisong left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!

(+) 코드를 살펴보다가 본문의 질문을 놓쳤네요 😖

  1. 저는 save를 제외한 메서드들은 다른 요소들의 조합이 key가 될 가능성이 있기 때문에 ById를 명시해 주는 것이 좋다 생각합니다!
  2. 저도 카피와 같이 isLikedChecklist 추천해 봅니다

Copy link
Contributor

@tkdgur0906 tkdgur0906 left a comment

Choose a reason for hiding this comment

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

  1. updateChecklist 한표요~
  2. isLikedChecklist 어떤가요~

# Conflicts:
#	backend/bang-ggood/src/main/java/com/bang_ggood/checklist/controller/ChecklistController.java
#	backend/bang-ggood/src/main/java/com/bang_ggood/checklist/dto/response/UserChecklistsPreviewResponse.java
#	backend/bang-ggood/src/main/java/com/bang_ggood/checklist/service/ChecklistManageService.java
#	backend/bang-ggood/src/main/java/com/bang_ggood/checklist/service/ChecklistService.java
#	backend/bang-ggood/src/main/java/com/bang_ggood/like/service/ChecklistLikeService.java
#	backend/bang-ggood/src/test/java/com/bang_ggood/auth/service/AuthServiceTest.java
#	backend/bang-ggood/src/test/java/com/bang_ggood/checklist/service/ChecklistManageServiceTest.java
#	backend/bang-ggood/src/test/java/com/bang_ggood/checklist/service/ChecklistServiceTest.java
Comment on lines 157 to 161
private List<ChecklistPreviewResponse> mapToChecklistsPreview(List<Checklist> checklists) {
return checklists.stream()
.map(this::mapToChecklistPreview)
.toList();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

이정도 코드도 메서드 분리가 필요할까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

분리했을 때 가독성이 훨씬 좋아진다고 생각했습니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-> 메서드 삭제했습니다.

Comment on lines 100 to 104
ChecklistPreviewResponse previewResponse1 = response.checklists().get(0);
ChecklistPreviewResponse previewResponse2 = response.checklists().get(1);

assertThat(previewResponse1.checklistId()).isEqualTo(checklist2); // 최신순으로 조회
assertThat(previewResponse2.checklistId()).isEqualTo(checklist1);
Copy link
Contributor

Choose a reason for hiding this comment

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

containsExactly를 활용해보는 건 어떨까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

containsExactly를 사용할 때 객체의 동등성으로 확인하게 되는데 현재 구현방식으로는 동등성을 확인하기 어려워 id값을 확인하는 방식으로 구현했습니다.

Long checklist1 = checklistManageService.createChecklist(user, checklistRequest1);
Long checklist2 = checklistManageService.createChecklist(user, checklistRequest2);

반환값이 id값여서 객체의 동등성을 확인하기 어려워요 🥲

Copy link
Contributor Author

Choose a reason for hiding this comment

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

-> 변수명을 checklist -> checklistId로 변경했습니다.

Copy link
Contributor

@tsulocalize tsulocalize left a comment

Choose a reason for hiding this comment

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

updateChecklist 한표
isliked 한표

@tsulocalize tsulocalize merged commit dffdf0f into dev-be Sep 24, 2024
2 checks passed
@tsulocalize tsulocalize deleted the 617/checklists branch September 24, 2024 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants