[refac] 메뉴 추가 시 발생 쿼리문 개선 #244
Conversation
| @@ -49,8 +49,9 @@ public void modifyMenu(final MenuPatchCommand command) { | |||
| @Transactional | |||
| public MenusPostResponse createMenus(final MenusPostCommand command) { | |||
| Store findStore = storeFinder.findByIdWhereDeletedIsFalse(command.storeId()); | |||
| List<Menu> allMenus = menuFinder.findAllByStore(findStore); | |||
There was a problem hiding this comment.
저도 요즘 고민을 하고 있는 지점인데 가연님처럼 2줄로 쓰는게 좋은지, 아니면
List
이렇게 한줄로 써주는게 깔끔한지 다른 분들 의견이 궁금합니다~ 뭔가 코드 이해하기에는 두줄로 쓰는게 좋을 것 같기도 한데 고민이에요ㅜ
There was a problem hiding this comment.
제 개인적인 생각으로는 가독성 측면에서 2줄로 쓰는게 나을 것 같아요. 또한 storeFinder로 찾은 findStore 객체는 밑의 로직에서도 반복적으로 사용되고 있으니, 이 로직에서는 2줄로 나누는게 맞는 것 같다고 생각합니당
There was a problem hiding this comment.
마자요 저도 가독성 측면에서는 2줄로 쓰면 조을 것 같아요 참고하겟습니다
| @@ -49,8 +49,9 @@ public void modifyMenu(final MenuPatchCommand command) { | |||
| @Transactional | |||
| public MenusPostResponse createMenus(final MenusPostCommand command) { | |||
| Store findStore = storeFinder.findByIdWhereDeletedIsFalse(command.storeId()); | |||
There was a problem hiding this comment.
이 로직 다른 곳에서도 많이 사용되어서 함수로 따로 빼면 어떨까합니다
There was a problem hiding this comment.
저는 함수로 따로 분리하지 않아도, storeFinder.findByIdWhereDeletedIsFalse(command.storeId()) 자체로도 충분히 의미를 파악할 수 있다고 생각하여 분리하지 않았습니다. 또한, 해당 로직에 변경이 생길 일도 적을 것 같아 분리하지 않아도 될 것 같은데 어떻게 생각하시나용? @Parkjyun 의견도 궁금합니다. 이부분은 모든 사람의 의견을 들어보고 수정할게용
| List<Menu> menus = command.menu().stream() | ||
| .filter(c -> !validateMenuConflict(findStore, c.name())) | ||
| .filter(c -> !validateMenuConflict(allMenus, c.name())) |
There was a problem hiding this comment.
필터를 통해 유효한 요청 menu들만 모아서 Menus를 만드는 로직같은데 얘 자체도 함수화하면 어떨지요
과한 함수화같으면 말씀해주시라요
There was a problem hiding this comment.
이 과정 2중 루프 도는 것 같은데 혹시 위에서 allMenu로 찾은 menu들의 name들을 HashSet에 저장해주 고 contains 함수를 통해 존재 여부를 검증해주는건 어떨까용
There was a problem hiding this comment.
좋은 의견 같아요. 수정했습니당. 확인해주시면 감사하겠습니당
There was a problem hiding this comment.
저는
- allMenus에서 stream을 돌아 name들을 해시 변수에 저장한다.
- command.menu()에서 stream 돌릴 때 1의 해시 변수의 contains로 확인한다.
시간 복잡도로 생각해서 O(n^2) -> O(n + m)으로 생각했습니당
There was a problem hiding this comment.
ㅋㅋ 맞아여.. 첨에 제가 잘못 생각했어여ㅋㅋ 의견 반영하여 수정했습니당
| .filter(c -> !validateMenuConflict(findStore, c.name())) | ||
| .map(c -> Menu.create(findStore, c.name(), c.price())) | ||
| .toList(); | ||
| List<Menu> menus = filterNewMenu(command, findStore); |
There was a problem hiding this comment.
여기서 MenusPostCommand 바로 전달해주는 형식보다는 command().menu를 전달해주는 방식이 어떨지요
의존성 최소화 관점에서 제안해보아요
Related Issue 📌
close #243
Description ✔️
validateMenuConflict함수를 실행할 때마다 select 쿼리가 발생하던 것을, 처음부터 모든 메뉴를 가져오고(select 쿼리 1번), list에서 stream을 돌면서 판단하도록 수정했습니다.To Reviewers
엄청 간단한 수정 사항입니다.