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.

Sign up for GitHub

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

[refac] 메뉴 추가 시 발생 쿼리문 개선 #244

Merged
merged 4 commits into from
Jan 11, 2025
Merged

[refac] 메뉴 추가 시 발생 쿼리문 개선 #244

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
51ebbb4
[refac] optimize query execution by reducing select queries
kgy1008 Jan 6, 2025
9d8f20d
[refac] improve nested loop
kgy1008 Jan 8, 2025
0c11e59
[refac] delete unnecessary query
kgy1008 Jan 8, 2025
34e5f0b
[refac] minimize interdependence between methods
kgy1008 Jan 8, 2025
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
7 changes: 4 additions & 3 deletions src/main/java/org/hankki/hankkiserver/api/menu/service/MenuCommandService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,9 @@ public void modifyMenu(final MenuPatchCommand command) {
@Transactional
public MenusPostResponse createMenus(final MenusPostCommand command) {
Store findStore = storeFinder.findByIdWhereDeletedIsFalse(command.storeId());
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.

저는 함수로 따로 분리하지 않아도, storeFinder.findByIdWhereDeletedIsFalse(command.storeId()) 자체로도 충분히 의미를 파악할 수 있다고 생각하여 분리하지 않았습니다. 또한, 해당 로직에 변경이 생길 일도 적을 것 같아 분리하지 않아도 될 것 같은데 어떻게 생각하시나용? @Parkjyun 의견도 궁금합니다. 이부분은 모든 사람의 의견을 들어보고 수정할게용

List<Menu> allMenus = menuFinder.findAllByStore(findStore);
Copy link
Contributor

Choose a reason for hiding this comment

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

저도 요즘 고민을 하고 있는 지점인데 가연님처럼 2줄로 쓰는게 좋은지, 아니면
List

allMenus = menuFinder.findAllByStore(storeFinder.findByIdWhereDeletedIsFalse(command.storeId()));

이렇게 한줄로 써주는게 깔끔한지 다른 분들 의견이 궁금합니다~ 뭔가 코드 이해하기에는 두줄로 쓰는게 좋을 것 같기도 한데 고민이에요ㅜ

Copy link
Member Author

Choose a reason for hiding this comment

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

제 개인적인 생각으로는 가독성 측면에서 2줄로 쓰는게 나을 것 같아요. 또한 storeFinder로 찾은 findStore 객체는 밑의 로직에서도 반복적으로 사용되고 있으니, 이 로직에서는 2줄로 나누는게 맞는 것 같다고 생각합니당

Copy link
Contributor

Choose a reason for hiding this comment

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

마자요 저도 가독성 측면에서는 2줄로 쓰면 조을 것 같아요 참고하겟습니다

List<Menu> menus = command.menu().stream()
.filter(c -> !validateMenuConflict(findStore, c.name()))
.filter(c -> !validateMenuConflict(allMenus, c.name()))
Copy link
Contributor

Choose a reason for hiding this comment

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

필터를 통해 유효한 요청 menu들만 모아서 Menus를 만드는 로직같은데 얘 자체도 함수화하면 어떨지요
과한 함수화같으면 말씀해주시라요

Copy link
Contributor

@PicturePark1101 PicturePark1101 Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

이 과정 2중 루프 도는 것 같은데 혹시 위에서 allMenu로 찾은 menu들의 name들을 HashSet에 저장해주 고 contains 함수를 통해 존재 여부를 검증해주는건 어떨까용

Copy link
Member Author

@kgy1008 kgy1008 Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

좋은 의견 같아요. 수정했습니당. 확인해주시면 감사하겠습니당

Copy link
Contributor

Choose a reason for hiding this comment

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

저는

  1. allMenus에서 stream을 돌아 name들을 해시 변수에 저장한다.
  2. command.menu()에서 stream 돌릴 때 1의 해시 변수의 contains로 확인한다.
    시간 복잡도로 생각해서 O(n^2) -> O(n + m)으로 생각했습니당

Copy link
Member Author

Choose a reason for hiding this comment

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

ㅋㅋ 맞아여.. 첨에 제가 잘못 생각했어여ㅋㅋ 의견 반영하여 수정했습니당

.map(c -> Menu.create(findStore, c.name(), c.price()))
.toList();
menuUpdater.saveAll(menus);
Expand All @@ -69,8 +70,8 @@ private void updateLowestPriceInStore(final Store findStore) {
findStore.updateLowestPrice(menuFinder.findLowestPriceByStore(findStore));
}

private boolean validateMenuConflict(final Store store, final String menuName) {
return menuFinder.existsByStoreAndName(store, menuName);
private boolean validateMenuConflict(final List<Menu> menus, final String menuName) {
return menus.stream().anyMatch(menu -> menu.getName().equals(menuName));
}

private void checkNoMenuInStore(final Store store, final long userId) {
Expand Down