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

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

Merged
merged 2 commits into from
Dec 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package org.terning.terningserver.controller;

import lombok.RequiredArgsConstructor;
import org.springframework.data.domain.Pageable;
import org.springframework.data.web.PageableDefault;
import org.springframework.http.ResponseEntity;
import org.springframework.security.core.annotation.AuthenticationPrincipal;
import org.springframework.web.bind.annotation.*;
Expand All @@ -26,13 +28,13 @@ public class HomeController implements HomeSwagger {
@GetMapping("/home")
public ResponseEntity<SuccessResponse<HomeAnnouncementsResponseDto>> getAnnouncements(
@AuthenticationPrincipal Long userId,
@RequestParam(value = "sortBy", required = false, defaultValue = "deadlineSoon") String sortBy
){
HomeAnnouncementsResponseDto announcements = homeService.getAnnouncements(userId, sortBy);

@RequestParam(value = "sortBy", required = false, defaultValue = "deadlineSoon") String sortBy,
@PageableDefault(size = 10) Pageable pageable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

HomeAnnouncementsResponseDto announcements = homeService.getAnnouncements(userId, sortBy, pageable);
return ResponseEntity.ok(SuccessResponse.of(SUCCESS_GET_ANNOUNCEMENTS, announcements));
}


@GetMapping("/home/upcoming")
Comment on lines 36 to 38
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
Member Author

Choose a reason for hiding this comment

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

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

public ResponseEntity<SuccessResponse<List<UpcomingScrapResponseDto>>> getUpcomingScraps(
@AuthenticationPrincipal Long userId
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.tags.Tag;
import org.springframework.data.domain.Pageable;
import org.springframework.http.ResponseEntity;
import org.terning.terningserver.dto.user.response.HomeAnnouncementsResponseDto;
import org.terning.terningserver.dto.user.response.UpcomingScrapResponseDto;
Expand All @@ -15,7 +16,8 @@ public interface HomeSwagger {
@Operation(summary = "홈화면 > 나에게 딱맞는 인턴 공고 조회", description = "특정 사용자에 필터링 조건에 맞는 인턴 공고 정보를 조회하는 API")
ResponseEntity<SuccessResponse<HomeAnnouncementsResponseDto>> getAnnouncements(
Long userId,
String sortBy
String sortBy,
Pageable pageable
);

@Operation(summary = "홈화면 > 곧 마감인 스크랩 공고 조회", description = "곧 마감인 스크랩 공고를 조회하는 API")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,20 @@

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

좋은 질문 감사합니다! 🙏

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

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

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

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

boolean hasNext,
List<HomeResponseDto> result
) {
public static HomeAnnouncementsResponseDto of(final int totalCount, final List<HomeResponseDto> announcements){
public static HomeAnnouncementsResponseDto of(
final int totalPages,
final long totalCount,
final boolean hasNext,
final List<HomeResponseDto> announcements) {
return HomeAnnouncementsResponseDto.builder()
.totalPages(totalPages)
.totalCount(totalCount)
.hasNext(hasNext)
.result(announcements)
.build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,5 @@ public interface InternshipRepositoryCustom {

Page<InternshipAnnouncement> searchInternshipAnnouncement(String keyword, String sortBy, Pageable pageable);

List<Tuple> findFilteredInternshipsWithScrapInfo(User user, String sortBy);

Page<Tuple> findFilteredInternshipsWithScrapInfo(User user, String sortBy, Pageable pageable);
}
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,9 @@ private OrderSpecifier createOrderSpecifier(String sortBy) {
}

@Override
public List<Tuple> findFilteredInternshipsWithScrapInfo(User user, String sortBy){
return jpaQueryFactory
.select(internshipAnnouncement, scrap.id, scrap.color) // tuple -> Scrap 정보 한번에 불러오기
public Page<Tuple> findFilteredInternshipsWithScrapInfo(User user, String sortBy, Pageable pageable) {
List<Tuple> content = jpaQueryFactory
.select(internshipAnnouncement, scrap.id, scrap.color)
.from(internshipAnnouncement)
.leftJoin(internshipAnnouncement.scraps, scrap).on(scrap.user.eq(user))
.where(
Expand All @@ -125,9 +125,23 @@ public List<Tuple> findFilteredInternshipsWithScrapInfo(User user, String sortBy
sortAnnouncementsByDeadline().asc(),
getSortOrder(sortBy)
)
.offset(pageable.getOffset())
.limit(pageable.getPageSize())
.fetch();

JPAQuery<Long> countQuery = jpaQueryFactory
.select(internshipAnnouncement.count())
.from(internshipAnnouncement)
.where(
getJobTypeFilter(user),
getGraduatingFilter(user),
getWorkingPeriodFilter(user),
getStartDateFilter(user)
);

return PageableExecutionUtils.getPage(content, pageable, countQuery::fetchOne);
}

private BooleanExpression getGraduatingFilter(User user){
if(user.getFilter().getGrade() != Grade.SENIOR){
return internshipAnnouncement.isGraduating.isFalse();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
package org.terning.terningserver.service;

import org.springframework.data.domain.Pageable;
import org.terning.terningserver.dto.user.response.HomeAnnouncementsResponseDto;

public interface HomeService {

HomeAnnouncementsResponseDto getAnnouncements(Long userId, String sortBy);
HomeAnnouncementsResponseDto getAnnouncements(Long userId, String sortBy, Pageable pageable);
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import com.querydsl.core.Tuple;
import lombok.RequiredArgsConstructor;
import org.springframework.data.domain.Page;
import org.springframework.data.domain.Pageable;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
import org.terning.terningserver.domain.InternshipAnnouncement;
Expand All @@ -22,47 +24,71 @@
@Service
@RequiredArgsConstructor
@Transactional(readOnly = true)
public class HomeServiceImpl implements HomeService{
public class HomeServiceImpl implements HomeService {

private final InternshipRepository internshipRepository;
private final UserRepository userRepository;

@Override
public HomeAnnouncementsResponseDto getAnnouncements(Long userId, String sortBy){
User user = userRepository.findById(userId).orElseThrow(
() -> new CustomException(ErrorMessage.NOT_FOUND_USER_EXCEPTION)
);
public HomeAnnouncementsResponseDto getAnnouncements(Long userId, String sortBy, Pageable pageable) {
User user = getUserById(userId);

// 유저의 필터 정보가 없는 경우
if(user.getFilter() == null){
return HomeAnnouncementsResponseDto.of(0,List.of());
if (user.getFilter() == null) {
return createEmptyResponse();
}

List<Tuple> announcements = internshipRepository.findFilteredInternshipsWithScrapInfo(user, sortBy);
Page<Tuple> pagedAnnouncements = internshipRepository.findFilteredInternshipsWithScrapInfo(user, sortBy, pageable);

// 해당하는 공고가 없는 경우
if(announcements.isEmpty()){
return HomeAnnouncementsResponseDto.of(0, List.of());
if (pagedAnnouncements.isEmpty()) {
return createEmptyResponse();
}

List<HomeResponseDto> responseDtos = announcements.stream()
List<HomeResponseDto> responseDtos = mapToHomeResponseDtos(pagedAnnouncements);

return createResponse(pagedAnnouncements, responseDtos);
}

private User getUserById(Long userId) {
return userRepository.findById(userId)
.orElseThrow(() -> new CustomException(ErrorMessage.NOT_FOUND_USER_EXCEPTION));
}

private HomeAnnouncementsResponseDto createEmptyResponse() {
return HomeAnnouncementsResponseDto.of(0, 0, false, List.of());
}

private List<HomeResponseDto> mapToHomeResponseDtos(Page<Tuple> pagedAnnouncements) {
return pagedAnnouncements.getContent().stream()
.map(tuple -> {
InternshipAnnouncement announcement = tuple.get(internshipAnnouncement);
Long scrapId = tuple.get(scrap.id);
Color color = tuple.get(scrap.color);
boolean isScrapped = (scrapId != null); // 스크랩 여부

// scrap 하지 않은 경우 color는 지정되지 않아야 한다.
String colorValue = (isScrapped && color != null) ? color.getColorValue() : null;
boolean isScrapped = isScrapped(scrapId);
String colorValue = determineColorValue(isScrapped, color);

return HomeResponseDto.of(announcement, isScrapped, colorValue);
})
.toList();

return HomeAnnouncementsResponseDto.of(responseDtos.size(), responseDtos);
}

private boolean isScrapped(Long scrapId) {
return scrapId != null;
}
}

private String determineColorValue(boolean isScrapped, Color color) {
if (isScrapped && color != null) {
return color.getColorValue();
}
return null;
}
Comment on lines +79 to +84
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
Member Author

Choose a reason for hiding this comment

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

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

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

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


private HomeAnnouncementsResponseDto createResponse(Page<Tuple> pagedAnnouncements, List<HomeResponseDto> responseDtos) {
return HomeAnnouncementsResponseDto.of(
pagedAnnouncements.getTotalPages(),
pagedAnnouncements.getTotalElements(),
pagedAnnouncements.hasNext(),
responseDtos
);
}
Comment on lines +86 to +93
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

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

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

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

}
Loading