fix: statistics valid 데이터 동기화 #242
Conversation
Summary of ChangesHello @JjungminLee, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 Pull Request는 리뷰 통계 및 목록 조회 시 유효하지 않은 메뉴(예: 김치, 단무지 등 공통 반찬)가 포함되어 표시되던 문제를 수정합니다. 변경 사항은 리뷰 서비스 로직과 응답 데이터 전송 객체(DTO)를 수정하여, 오직 리뷰에 적합한 유효한 메뉴들만이 관련 엔드포인트에서 정확하게 표시되도록 보장합니다. 이를 통해 데이터의 정확성을 높이고 사용자 경험을 개선하는 것을 목표로 합니다. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 데이터 흐름 속, 유효한 것만 걸러내, 정확성 빛나. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이 PR은 리뷰 통계 및 목록 조회 시 '김치', '단무지'와 같은 유효하지 않은 메뉴가 포함되는 문제를 해결하는 것을 목표로 하고 있습니다. MenuFilterUtil을 사용하여 메뉴를 필터링하고, 응답 DTO에 유효한 메뉴만 포함하도록 수정하여 문제를 올바르게 해결한 것으로 보입니다.
리뷰 과정에서 몇 가지 개선점을 발견했습니다. 성능 향상을 위해 스트림 내에서 중첩된 순회를 피하는 방법, 불필요한 데이터베이스 조회 및 중복된 코드를 제거하여 코드의 효율성과 가독성을 높이는 방안에 대한 의견을 댓글로 남겼습니다. 또한, 로컬 환경 설정 파일인 application-local.yml이 삭제되었는데, 이 변경 사항이 의도된 것인지 확인해보시는 것이 좋겠습니다. 전반적으로 좋은 수정이라고 생각합니다.
| if (!mealRepository.existsById(mealId)) { | ||
| throw new BaseException(NOT_FOUND_MEAL); | ||
| } |
There was a problem hiding this comment.
mealRepository.findById(mealId).orElseThrow(...)를 통해 이미 mealId에 해당하는 Meal이 없는 경우 예외 처리를 하고 있습니다. 따라서 바로 아래의 mealRepository.existsById(mealId)를 통한 중복 확인은 불필요합니다. 이 부분을 제거하여 코드를 간결하게 만들 수 있습니다.
| List<Long> validMenuIds = mealMenuRepository.findMenusByMeal(meal) | ||
| .stream() | ||
| .filter(menu -> !MenuFilterUtil.isExcludedFromReview(menu.getName())) // 공통메뉴 제외 | ||
| .map(Menu::getId) | ||
| .toList(); | ||
|
|
||
| if (validMenuIds.isEmpty()) { | ||
| return SliceResponse.empty(); | ||
| } | ||
|
|
||
| List<Long> mealIds = mealMenuRepository.findMealIdsByMenuIds(menuIds); | ||
| List<Long> mealIds = mealMenuRepository.findMealIdsByMenuIds(validMenuIds); | ||
| if (mealIds.isEmpty()) { | ||
| return SliceResponse.empty(); | ||
| } | ||
|
|
||
| Page<Review> pageReviews = reviewRepository.findReviewsByMealIds(mealIds, lastReviewId, pageable); | ||
|
|
||
| Long userId = (userDetails != null) ? userDetails.getId() : null; | ||
|
|
||
| List<Menu> menus = mealMenuRepository.findMenusByMeal(meal); | ||
|
|
||
| List<ValidMenuForViewResponse.MenuDto> validMenus = menus.stream() | ||
| .filter(menu -> !MenuFilterUtil.isExcludedFromReview( | ||
| menu.getName())) | ||
| .map(menu -> ValidMenuForViewResponse.MenuDto.builder() | ||
| .menuId(menu.getId()) | ||
| .name(menu.getName()) | ||
| .build()) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
findMealReviewList 메소드 내에서 mealMenuRepository.findMenusByMeal(meal)이 두 번 호출되어 불필요한 데이터베이스 조회를 유발하고 있습니다. 또한, 유효 메뉴 ID를 얻는 로직과 유효 메뉴 DTO를 만드는 로직이 분리되어 있어 비효율적입니다. 데이터베이스 조회를 한 번으로 줄이고, 조회한 메뉴 목록을 재사용하여 validMenus DTO 리스트를 먼저 생성한 후, 이 리스트에서 ID를 추출하도록 리팩토링하는 것을 제안합니다. 이렇게 하면 코드의 효율성과 가독성이 향상됩니다.
List<Menu> menus = mealMenuRepository.findMenusByMeal(meal);
List<ValidMenuForViewResponse.MenuDto> validMenus = menus.stream()
.filter(menu -> !MenuFilterUtil.isExcludedFromReview(
menu.getName()))
.map(menu -> ValidMenuForViewResponse.MenuDto.builder()
.menuId(menu.getId())
.name(menu.getName())
.build())
.collect(Collectors.toList());
if (validMenus.isEmpty()) {
return SliceResponse.empty();
}
List<Long> validMenuIds = validMenus.stream().map(ValidMenuForViewResponse.MenuDto::getMenuId).toList();
List<Long> mealIds = mealMenuRepository.findMealIdsByMenuIds(validMenuIds);
if (mealIds.isEmpty()) {
return SliceResponse.empty();
}
Page<Review> pageReviews = reviewRepository.findReviewsByMealIds(mealIds, lastReviewId, pageable);
Long userId = (userDetails != null) ? userDetails.getId() : null;| List<String> likedMenuNames = review.getMenuLikes().stream() | ||
| .filter(ReviewMenuLike::getIsLike) | ||
| .map(like -> like.getMenu().getName()) | ||
| .filter(name -> validMenus.stream() | ||
| .map(ValidMenuForViewResponse.MenuDto::getName) | ||
| .anyMatch(validName -> validName.equals(name))) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
likedMenuNames를 필터링할 때 스트림 내에서 다른 스트림을 순회하며 anyMatch를 사용하고 있습니다. validMenus 리스트가 클 경우 이 방식은 O(N*M)의 시간 복잡도를 가져 성능 저하를 유발할 수 있습니다. validMenus의 이름들을 Set으로 미리 만들어두면 O(1) 시간 복잡도로 조회가 가능하여 전체 성능을 O(N+M)으로 개선할 수 있습니다.
java.util.Set<String> validMenuNames = validMenus.stream()
.map(ValidMenuForViewResponse.MenuDto::getName)
.collect(java.util.stream.Collectors.toSet());
List<String> likedMenuNames = review.getMenuLikes().stream()
.filter(ReviewMenuLike::getIsLike)
.map(like -> like.getMenu().getName())
.filter(validMenuNames::contains)
.collect(Collectors.toList());| List<ValidMenuForViewResponse.MenuDto> validMenus = menus.stream() | ||
| .filter(menu -> !MenuFilterUtil.isExcludedFromReview( | ||
| menu.getName())) | ||
| .map(menu -> ValidMenuForViewResponse.MenuDto.builder() | ||
| .menuId(menu.getId()) | ||
| .name(menu.getName()) | ||
| .build()) | ||
| .toList(); |
There was a problem hiding this comment.
유효한 메뉴 목록을 필터링하고 ValidMenuForViewResponse.MenuDto로 변환하는 로직이 findMealReviewList, findMealReviews, validMenuForReview 메소드에 중복되어 나타납니다. 이 로직을 별도의 private 메소드로 추출하면 코드 중복을 줄이고 유지보수성을 높일 수 있습니다. 예를 들어, private List<ValidMenuForViewResponse.MenuDto> getValidMenus(Meal meal)와 같은 헬퍼 메소드를 만들어 사용하는 것을 고려해 보세요.
There was a problem hiding this comment.
크흠.. 리펙토링때 손보는걸로,,,
#️⃣ Issue Number
📝 요약(Summary)
💬 공유사항 to 리뷰어
✅ PR Checklist
PR이 다음 요구 사항을 충족하는지 확인하세요.