[009-Kakao-Saving-Store-Info] 가게 정보 저장 및 검색 1차 #23
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
heeve1
approved these changes
Aug 4, 2024
Comment on lines
8
to
10
| //fixme: Domain Exception이 있는데 이렇게 PlaceException을 만들어도 되는지 궁금합니다. | ||
| // 제 생각에는 도메인 내부에도 여러 파트가 있을 수 있기에 Exception을 나누어서 구성하면 | ||
| // 어디서 난 에러인지 더 알기 쉬울 것같다고 생각해서 추가 하였습니다. |
There was a problem hiding this comment.
넵 상속구조로 표현하는 것이 적절하다고 판단되신다면 충분히 좋은 것 같습니다.
Comment on lines
22
to
27
| //fixme: 기존의 KakaoParser라는 클래스에 있던 기능을 아래의 parsePlaceInfo로 옮겼습니다. | ||
| // 원래 KakaoParser를 Helper Class로 사용하려고 했는데 Place란 클래스에 특수한것 같아서 공용공간으로 빼는게 부담 되었습니다. | ||
| // 그러던 중, KakaoAPI에서 받은 정보는 차츰 저장되고 내부 DB로 옮겨진 정보가 충분할 경우 내부 DB에서만 검색하려고 했어서 | ||
| // 내부 DB에 아직 없는 부분을 저장하는 책임을 KakaoAPIStoreInfoSaver로 옮겼습니다. | ||
| // 질문: 원래 KakaoAPIStoreInfoSaver를 Helper클래스로 사용하면 되겠다고 생각했는데 이 경우 의존성이 있어서 static으로 만들지 못 합니다. | ||
| // 이런 경우에는 어떻게 해야할까요? |
There was a problem hiding this comment.
- 한번이라도 계산된 input은 그 결과가 DB에 저장되고 다음에 같은 input이 들어오는 경우 api 호출없이 DB로 응답을 한려고 했다고 보면 될까요? 그런데 코드로 보기엔 kakaoAPIRequest.getStoreInfo() 가 매 요청마다 실행될것 같은데 맞을까요?
- 다른 bean이 필요한 메서드의 경우는 말씀대로 static으로 만들기 어렵습니다. 이 경우엔 다른 Service로 만들어야 겠지요. 여기서 호출받는 서비스와 호출하는 서비스를 구분할지는 선택일 것 같네요 (트레이드 오프 고려)
There was a problem hiding this comment.
- 네 일단 매 요청마다 저장할때 내부 DB에 이미 있는지 비교하고 없으면 저장하는데 이후 차이가 없어지면 그만 하려고 했습니다. 이 비교 값이 0이 되기 시작하면 API요청을 그만할까 했는데 이러면 사용자가 모든 값을 검색해 볼 수는 없을테니까 모든 가게 정보가 내부 DB에 저장되지 못할것 같았습니다. 실제로 서비스로 제작하였다고 하면 brute force로 직원이 하나씩 검색해서 저장 후 확인된 좌표에서는 검색을 안하는 방식으로 해야되나 여쭈어 보려고 하였습니다.
(API 요청 중단 타이밍 결정 #24 ) - 그렇군요. 트레이드 오프를 고민해 보겠습니다. 제작 이유는 저장하는 로직이 API 요청 중단 타이밍 결정 #24 에서 결정 된 후 그만 사용되어야 한다고 생각해서 따로 서비스로 제작하였습니다.
|
|
||
| public interface StoreRepository extends JpaRepository<Place, Long> { | ||
| @Query(value = "SELECT * FROM place p WHERE (POWER(:x - p.x, 2) + POWER(:y - p.y, 2)) <= POWER(:radius/POWER(10,5), 2)", nativeQuery = true) | ||
| Optional<List<Place>> getStoresByNameAndRadius(double x, double y, int radius); |
There was a problem hiding this comment.
where의 좌변 조건에 계산이 들어가서 index를 과연 잘 탈 수 있는 쿼리일지 약간 의문이 드는군요. 괜찮을지? 한번 검토해보시지요~
There was a problem hiding this comment.
쿼리 효율은 mysql 공부하며 검토해보도록 하겠습니다! #25
| public ResponseEntity<ExceptionResponse> webException(final WebException e){ | ||
| return ResponseEntity.badRequest().body(new ExceptionResponse(HttpStatus.BAD_REQUEST, e.getMessage())); | ||
| } | ||
| } |
There was a problem hiding this comment.
추상화된 DomainException, WebException 을 기준으로 별도 예외 응답을 처리하는 패턴은 좋아보입니다~
Comment on lines
19
to
22
| //fixme:처음에 통신이 잘 되는지 확인하기 위해 사용하였습니다. | ||
| // 이 부분은 통신이 필요할 것 같아서 통합테스트로 제작하여 기능작동 확인 하였지만 | ||
| // KakaoAPI에서 무언가 변경시 깨지기 쉬울것 같아서 Disabled 처리하였습니다. | ||
| // 차라리 지우는게 남들이 보기에 덜 헷갈릴 지 궁금합니다. |
There was a problem hiding this comment.
좋은 고민입니다. 뭔가 안될때 외부 API가 혹시 문제인지, 테스트를 통해서 쉽게 돌려볼 수 있다면 문제 파악에 도움이 될거에요. 하지만 매번 테스트 셋에서 돌린다면 네트워크 등의 이슈로 이따금씩 터지는 테스트가 되기 때문에 관리가 어렵기도 합니다.
저 또한 해주신 것 처럼 disable 처리하고 주석으로 왜 지우지않고 disabled 를 붙여두었는지를 명시해둘 것 같습니다
There was a problem hiding this comment.
disable의 이유를 명시하도록 하겠습니다.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
수정사항
새로 PR을 만들어도 머지한 커밋이 나오게 되는군요. 가게 정보 저장 및 검색 1차 커밋 부분만 보시면 될 것 같습니다!
fixme에 추가를 못 했는데 Exception을 저번에 말씀하신대로 DomainException, WebException을 common에 만들고 상속받아 사용하도록 하였습니다. (찾아보니 7/24일 스크립트 59분 쯤에 있는 내용입니다.) 이런 방식으로 적용하는 걸 이야기 하신게 맞는지 확인 받고 싶습니다.
저번에 이야기 나눈 Exception에 관해서 생각을 해보았는데 unchecked 예외는 개발자가 놓칠수 있는 부분에 커스텀 Exception으로 다른 개발자나 사용자가 알수 있도록 메시지를 보내 주도록 하고 추가하고 checked 예외를 사용하게 될 때는 언어에서 주의할 점을 미리 알려주는 것이라고 생각하여 재시도등으로 따로 처리로직을 둘지 이런 처리가 힘들다면 Runtime Exception으로 바꾸어서 rollback하거나 구체적인 메시지를 나오게 할지 결정을 하여야 한다고 생각했습니다.
현재는 모두 커스텀 Exception으로 구체적인 메시지를 받도록 해 놓았지만 이후 다른 로직으로 처리를 하여 보도록 하겠습니다. (Exception 처리 #22 )