[✨feat/#185]: 공고 조회 페이지네이션 기능 도입 및 서비스 로직 개선 #186

jsoonworld · 2024-12-20T08:10:07Z

📄 Work Description

변경 사항

공고 조회 API에 페이징 기능 추가
- Pageable을 활용하여 페이징 처리 구현.
- @PageableDefault 어노테이션을 사용하여 기본 페이지 크기를 10으로 설정하여 유연한 페이징 요청 가능.
DTO 수정
- HomeAnnouncementsResponseDto에 페이징 관련 필드 추가:
  - totalPages: 전체 페이지 수.
  - hasNext: 다음 페이지 존재 여부.
Repository 수정
- 기존에 리스트로 반환하던 공고 데이터를 페이징 처리된 형태로 반환하도록 쿼리 수정.
- Page<Tuple> 타입을 활용하여 페이징 로직을 구현하고, 데이터의 효율적 처리를 보장.
- JPAQueryFactory를 사용하여 필터 조건을 기반으로 데이터를 페이징 처리하여 조회.
Service 로직 리팩토링
- 중복된 로직과 책임을 분리하여 공통 메서드로 분리:
  - getUserById: 사용자 검증 및 조회 책임 분리.
  - createEmptyResponse: 데이터가 없을 경우 기본 응답을 생성.
- DTO 매핑 로직 분리:
  - mapToHomeResponseDtos: 조회된 데이터를 DTO 형태로 변환하는 로직.
  - isScrapped, determineColorValue: 스크랩 여부 및 색상 처리 분리.
- 페이징 결과 및 변환된 데이터를 포함한 최종 응답 생성 메서드 추가: createResponse.
Swagger 수정
- 페이징 관련 파라미터(page, size)를 명시적으로 반영하여 API 문서 개선.
- sortBy 파라미터와 함께 페이징 처리 내용을 API 명세에 포함.

⚙️ ISSUE

closed [✨ feat] 인턴 공고 조회 기능에 페이지네이션 기능 추가 #185

📷 Screenshot

💬 To Reviewers

이번 변경사항에서는 페이징 처리를 중심으로 서비스 로직과 관련된 모든 계층(HomeController, Service, Repository)을 수정하였습니다.
특히, 서비스 계층에서는 다음과 같은 역할과 책임 분리에 중점을 두고 메서드를 리팩토링하였습니다:
- getUserById: 사용자 정보 검증 및 조회를 담당합니다.
- createEmptyResponse: 데이터가 없는 경우 기본 응답을 생성합니다.
- mapToHomeResponseDtos: DB에서 조회한 데이터를 DTO로 매핑합니다.
- isScrapped, determineColorValue: 스크랩 여부와 색상 값을 분리하여 처리합니다.
- createResponse: 페이징 결과를 포함한 최종 응답을 생성합니다.
테스트 코드 작성에서는 Mocking이 많이 필요하여 어려움을 겪었습니다. 특히, HomeServiceImpl에서 데이터베이스 및 사용자 관련 작업들이 많아 Mock 객체를 구성하는 데 많은 시간이 소요되었습니다.
Swagger나 Postman을 사용하지 않고 테스트 코드를 통해 검증하려 했으나, 코드가 테스트 작성에 최적화되어 있지 않아 개선의 필요성을 느꼈습니다.

🚀 개선 제안

최근 참석했던 이동욱(인프런 CTO)님 테스트 코드 세미나에서 배운 내용을 기반으로, 팀과 공유하여 테스트 작성이 용이한 코드 구조를 도입해보고 싶습니다.
테스트를 쉽게 작성할 수 있도록 서비스 로직과 데이터 접근 로직을 더욱 명확히 분리하고, Mocking 의존성을 줄일 수 있는 구조로 개선할 계획입니다.

🙏 리뷰어님께

코드 가독성 및 페이징 로직 최적화에 대해 보완점이나 개선할 부분이 있다면 언제든 말씀 부탁드립니다.
더불어, 테스트 코드 작성 방안이나 좋은 사례가 있다면 추천 부탁드립니다! 🙇

🔗 Reference

[Spring Pageable 사용 가이드](https://spring.io/guides)](https://spring.io/guides)

- 공고 조회 API에 페이징 기능 추가 - Pageable을 활용한 페이징 처리 구현 - `@PageableDefault`를 통해 기본 페이지 크기 설정 - DTO 수정 - HomeAnnouncementsResponseDto에 페이징 관련 필드 추가 (`totalPages`, `hasNext`) - Repository 수정 - 페이징 처리된 공고 데이터를 반환하도록 쿼리 수정 - `Page<Tuple>`를 활용한 페이징 로직 구현 - Service 로직 리팩토링 - 공통 메서드 분리 (`getUserById`, `createEmptyResponse` 등) - Color 값 및 스크랩 여부 처리 메서드 분리 (`isScrapped`, `determineColorValue`) - Swagger 수정 - API 문서에 페이징 파라미터 반영

junggyo1020

페이징 처리 고생하셨네요!

어노테이션 처리, 메서드 분리 등등 기존의 코드를 개선한 부분들이 좋았습니다.
다만, 공백처리와 같은 기본적인 사항들은 최대한 통일성을 주는게 좋아보여요!

여러 개선점들은 아래의 코드리뷰로 남겨두고, 전체적인 코드에 대한 리뷰는 간단하게 아래에 적어두겠습니다!

Spring 공식 문서를 활용했다면, 어느 부분을 참고했는지도 자세히 적어주시면 확인이 더 편할 것이라는 생각이드네요..!(워낙 양이 많다보니 직접 서치가 힘드네요)
Mock를 사용한 테스트 파일은 커밋이 안되어있어 코드리뷰가 필요하다면 PR에 같이 첨부해주시면 좋을 것 같습니다!

junggyo1020 · 2024-12-21T10:44:39Z

src/main/java/org/terning/terningserver/controller/HomeController.java

-    ){
-        HomeAnnouncementsResponseDto announcements = homeService.getAnnouncements(userId, sortBy);
+            @RequestParam(value = "sortBy", required = false, defaultValue = "deadlineSoon") String sortBy,
+            @PageableDefault(size = 10) Pageable pageable) {


@PageableDefault 어노테이션 사용 좋네요!!!

@PageableDefault 어노테이션에 대해 긍정적으로 봐주셔서 감사합니다! 해당 어노테이션을 활용하면 기본 페이징 설정을 간결하게 처리할 수 있어 저도 유용하다고 생각했습니다. 혹시 추가로 개선하거나 고려할 점이 있다면 편하게 말씀 부탁드릴게요. 함께 더 나은 코드를 만들어가요! 🙌

junggyo1020 · 2024-12-21T10:48:11Z

src/main/java/org/terning/terningserver/controller/HomeController.java


+
    @GetMapping("/home/upcoming")


여기의 공백은 어떤 의도로 추가했는지 알 수 있을까요?

공백 부분에 대해 질문 주셔서 감사합니다! 🙌
이 공백은 특별한 의도를 가지고 추가한 것은 아니고, 가독성을 조금 더 높이려는 의도에서 작성되었습니다.
다만, 팀 컨벤션이나 스타일 가이드에 따라 다를 수 있으니, 필요하다면 바로 수정하도록 하겠습니다! 의견 주셔서 감사드려요. 😊

junggyo1020 · 2024-12-21T10:48:56Z

src/main/java/org/terning/terningserver/dto/user/response/HomeAnnouncementsResponseDto.java

@@ -6,12 +6,20 @@

 @Builder
 public record HomeAnnouncementsResponseDto(
-        int totalCount, // 필터링 된 공고 총 개수
+        int totalPages,
+        long totalCount,


totalCount의 필드형식을 long 으로 변경한 근거는 무엇일까요?

좋은 질문 감사합니다! 🙏

DTO에서 totalCount를 long으로 변경한 이유는 pagedAnnouncements.getTotalElements() 메서드가 long 타입을 반환하기 때문입니다.

만약 totalCount를 int로 유지하면 이 값에 대해 형변환을 추가로 수행해야 하는데, 이는 코드의 가독성을 떨어뜨리고 실수 가능성을 높일 수 있다고 판단했습니다.

또한, totalElements의 값이 int의 범위를 초과하는 경우를 대비하는 확장성 측면에서도 long 타입이 더 적합하다고 생각했습니다.

이 점에서 더 나은 방법이 있다면 의견 주시면 감사하겠습니다. 함께 고민하면 더 좋은 결과를 낼 수 있을 것 같아요! 😊

junggyo1020 · 2024-12-21T10:53:36Z

...a/org/terning/terningserver/repository/internship_announcement/InternshipRepositoryImpl.java

+
+
    private BooleanExpression getGraduatingFilter(User user){


이 부분도 공백이 추가되었네요! 만약에 의도가 있다면 다른 메서드들도 모두 공백을 주거나 해야할 것 같네요:)

공백 부분에 대한 세심한 피드백 감사합니다! 🙏

특별한 의도는 없었지만, 가독성을 높이기 위해 무의식적으로 추가한 것 같습니다. 다만, 말씀하신 대로 다른 메서드들과 일관성을 유지하지 못한 점은 고려가 부족했던 부분인 것 같아요. 😊

팀 스타일 가이드나 컨벤션에 맞게 전체적으로 일관성을 갖추는 방향으로 수정해 보겠습니다! 의견 주셔서 감사합니다. 함께 코드의 완성도를 높여 나가요! 🙌

junggyo1020 · 2024-12-21T11:02:26Z

src/main/java/org/terning/terningserver/service/HomeServiceImpl.java

+    private String determineColorValue(boolean isScrapped, Color color) {
+        if (isScrapped && color != null) {
+            return color.getColorValue();
+        }
+        return null;
+    }


삼항연산자 대신 메서드를 분리한 부분 좋네요!

삼항연산자를 대신해 메서드로 분리한 부분을 긍정적으로 봐주셔서 감사합니다! 🙏

저도 가독성과 재사용성을 높이기 위해 삼항연산자 대신 메서드를 활용하는 것이 더 적합하다고 생각했어요. 이렇게 하면 해당 로직을 명확하게 표현할 수 있고, 유지보수 시에도 더 쉽게 이해할 수 있을 것 같았습니다.

혹시 더 개선할 점이나 다른 의견 있으시면 언제든 공유 부탁드릴게요. 함께 더 좋은 코드를 만들어가요! 😊

junggyo1020 · 2024-12-21T11:12:28Z

src/main/java/org/terning/terningserver/service/HomeServiceImpl.java

+    private HomeAnnouncementsResponseDto createResponse(Page<Tuple> pagedAnnouncements, List<HomeResponseDto> responseDtos) {
+        return HomeAnnouncementsResponseDto.of(
+                pagedAnnouncements.getTotalPages(),
+                pagedAnnouncements.getTotalElements(),
+                pagedAnnouncements.hasNext(),
+                responseDtos
+        );
+    }


현재 HomeServiceImpl에서 비즈니스 로직(필터링 후 공고를 Tuple로 매핑)이 많아 보인다고 생각합니다!

Repository 레벨에서 필터링 후 Page를 받아오는 것은 좋지만, Tuple -> DTO 변환까지 모두 Service에 맡기면 Service 로직이 너무 커질 수 있다는 생각을 하고 있습니다. 어떻게 생각하시나요?

객체의 역할과 책임에 대한 좋은 피드백 감사합니다! 🙏

저도 Service 클래스의 비즈니스 로직이 커지는 부분에 대해 고민이 많았고, 지금 구조를 보니 Repository와 DTO의 책임이 명확히 나뉘지 않은 부분이 있다는 점에 공감합니다.

현재 private 메서드가 많다는 점은 객체의 역할과 책임 분리를 다시 생각해봐야 한다는 신호로 받아들여졌습니다. 비록 private 메서드로 로직을 나누며 역할을 단순화하려고 했지만, 하나의 메서드가 하나의 책임만 수행하도록 하면서도 객체의 책임 분리를 고민해야 할 시점이라고 생각합니다.

DTO는 데이터를 전달하는 데 집중하고, Repository는 저장과 조회에만 충실해야 한다는 원칙에도 동의합니다. 말씀하신 것처럼 Tuple -> DTO 변환을 다른 레이어로 위임하거나, 별도 객체를 통해 이 로직을 분리하는 방향도 고려해 보겠습니다.

피드백 정말 감사드리고, 함께 더 나은 구조를 고민해보면 좋을 것 같습니다! 😊

jsoonworld added ✨ feat 새로운 기능 추가 🦊장순🦊 labels Dec 20, 2024

jsoonworld requested a review from junggyo1020 December 20, 2024 08:10

jsoonworld self-assigned this Dec 20, 2024

junggyo1020 approved these changes Dec 21, 2024

View reviewed changes

[♻️ refactor/#185]: 공백 제거

be616ba

jsoonworld merged commit f56834d into develop Dec 21, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[✨feat/#185]: 공고 조회 페이지네이션 기능 도입 및 서비스 로직 개선 #186

[✨feat/#185]: 공고 조회 페이지네이션 기능 도입 및 서비스 로직 개선 #186

jsoonworld commented Dec 20, 2024

junggyo1020 left a comment

junggyo1020 Dec 21, 2024

jsoonworld Dec 21, 2024

junggyo1020 Dec 21, 2024

jsoonworld Dec 21, 2024

junggyo1020 Dec 21, 2024

jsoonworld Dec 21, 2024

junggyo1020 Dec 21, 2024

jsoonworld Dec 21, 2024

junggyo1020 Dec 21, 2024

jsoonworld Dec 21, 2024

junggyo1020 Dec 21, 2024

jsoonworld Dec 21, 2024

[✨feat/#185]: 공고 조회 페이지네이션 기능 도입 및 서비스 로직 개선 #186

[✨feat/#185]: 공고 조회 페이지네이션 기능 도입 및 서비스 로직 개선 #186

Conversation

jsoonworld commented Dec 20, 2024

📄 Work Description

변경 사항

⚙️ ISSUE

📷 Screenshot

💬 To Reviewers

🚀 개선 제안

🙏 리뷰어님께

🔗 Reference

junggyo1020 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment