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

[FIX] 북마크하는 api 를 리팩토링하라 #131

Merged
merged 1 commit into from
Aug 10, 2023
Merged

Conversation

seunghaLim
Copy link
Collaborator

🍞 PR 타입

  • 기능 추가
  • 기능 수정
  • 기능 삭제
  • 버그 수정
  • 의존성, 환경 변수, 빌드 관련 코드 업데이트

🍞 반영 브랜치

refactor/#125 -> dev

🍞 변경 사항

  • 하나 큰 메서드를 여러 작은 메서드로 구분

🍞 테스트 결과

🍞 To Reviewer

함수 구분 기준이 감이 잘 안잡혀서 리뷰 볼 때 BookMarkService 클래스 위주로 봐주시면 감사하겠습니다

Copy link
Member

@sung-silver sung-silver left a comment

Choose a reason for hiding this comment

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

고생했습니다 승하언니!!

@@ -34,36 +34,50 @@ public BookMarkResponseDTO doBookMark(boolean isAddingBookMark, Long bakeryId, L
.orElseThrow(() -> new NotFoundException(ErrorType.NOT_FOUND_USER_EXCEPTION));

Optional<BookMark> foundBookMark =
bookMarkRepository.findByMemberAndBakery(foundMember, foundBakery);
bookMarkRepository.findByMemberIdAndBakeryId(memberId, bakeryId);
Copy link
Member

@sung-silver sung-silver Aug 10, 2023

Choose a reason for hiding this comment

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

제가 알기로는 본래 쓰셨던 코드처럼 객체 단위로 찾는 것이 더 효율적인 것으로 알고 있는데 읽었던 아티클을 지금당장 찾지 못했네요..!
제 생각에는 우선 JPA는 객체 지향적인 프로그래밍 모델을 데이터베이스에서 사용할 수 있게 설계되어 있고, 이에 대한 쿼리를 최적화 해주는 역할을 하고 있으니 객체 단위로 쓰는게 아닐까 싶습니다! 위 방식 대로라면 member 테이블, bakery 테이블에서 각각을 찾고 다시 그 객체로 bookMark를 조회할 것 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵 그렇게 수정했습니다~ 다만 해당 쿼리 전에 member와 bakery를 조회하는 쿼리가 나가서 거기서 캐싱해서 가져오는 것 같아요 그래서 객체 말고 id로 조회해도 객체로 조회할 때 날라가는 쿼리와 동일하긴 합니다
그래도 말씀하신대로 JPA가 객체 지향 프로그램과 SQL 기반 데이터베이스를 연결해주도록 설계된 툴이기 때문에 코드리뷰 남겨주신 대로 바꿨습니다!

@seunghaLim seunghaLim merged commit a2a2353 into dev Aug 10, 2023
1 check passed
@seunghaLim seunghaLim deleted the refactor/#125 branch August 14, 2023 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants