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

Feature/jstak/kakao #88

Merged
merged 16 commits into from
Oct 10, 2023
Merged

Feature/jstak/kakao #88

merged 16 commits into from
Oct 10, 2023

Conversation

jiseongTak
Copy link
Member

해결하려는 문제가 무엇인가요?

  • 검색어로 카카오 주소 검색 api 호출 및 검색 이력 저장

어떻게 해결했나요?

  • SearchLocationHistory 엔티티, 서비스 생성
  • kakao api 호출 응답용 RoadAddressResponse 추가
  • domain 패키지 삭제
  • kakao 관련 클래스 위치 변경

Attachment

@github-actions
Copy link

github-actions bot commented Oct 9, 2023

Test Results

15 tests     1 ✔️  10m 26s ⏱️
  5 suites    0 💤
  5 files    14

For more details on these failures, see this check.

Results for commit 1bc6b49.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@TonyKim9401 TonyKim9401 left a comment

Choose a reason for hiding this comment

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

LGTM 👯

Comment on lines +20 to +25
@PostMapping
fun saveLocation(@RequestBody request: RequestLocationHistory): ResponseEntity<ResponseLocationHistory> {
val user = userService.findByUserId(userId = request.userId)
val response = locationHistoryService.saveLocationHistory(user, request.keyword)
return ResponseEntity.ok(response)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

전 이렇게 userId가 필요할 경우 request에 user정보가 있더라도 @RequestParam 으로 따로 받게 했는데
도메인별로 좀 나누고자 했던 마음도 있던것 같아요. 어떤게 더 나을지 의견 나누고 싶습니다 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

post 라서 그냥 body에 넣으면 될 것 같았는데 생각해보니까 pathvariable이나 requestparam으로 쓰는게 맞는것 같네요

Copy link
Contributor

Choose a reason for hiding this comment

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

아 현준님께서는 제 코드 보시고 reuqestbody 값에서 꺼내 쓰면 되지 않냐고 물어보여서 어떤게 나을지 잘 모르겟어서요..
내일 허재 코치님께 여쭈어봐도 괜찮을것 같습니다!

Comment on lines 10 to 27
@Service
class LocationHistoryService(
private val locationHistoryManager: LocationHistoryManager,
private val kakaoApiManager: KakaoApiManager,
) {

fun saveLocationHistory(user: User, keyword: String): ResponseLocationHistory {
val response = kakaoApiManager.requestAddressSearch(keyword)

checkNotNull(response) {
failWithMessage("")
}

val roadAddress = response.documents[0].roadAddress

return locationHistoryManager.addLocationHistory(user, keyword, roadAddress).toResponseDto()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

service에서 user 뿐만 아니라 다른 도메인 관련 객체 사용시

  1. 넘어온 객체 그대로 사용
  2. 넘어온 객체의 id로 db에서 조회해서 해당 객체 사용
    저는 주로 2번을 쓰는데요, 1번 사용 하더라도 Service 계층으로 넘어왔을 때 user에 대한 검증이 한번 더 이루어진 상태에서 사용하면 더 안전할것 같은 생각이 들어요.

Copy link
Member Author

Choose a reason for hiding this comment

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

controller에서 user service로 find하고 넘기게끔 해놓았는데 그냥 다 service 계층에서 처리하는게 맞는건가 고민이되긴 하네요

Copy link
Contributor

Choose a reason for hiding this comment

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

저는 controller에서는 api 호출 / 응답 에만 초점을 두고 싶어서 serivce 계층에서 처리하는걸로 했거든요 ㅎㅎ
처음에 즐겨찾기 만들때 userRepository를 통해 user를 확인해야 하니 component에서 user검증을 진행 했는데 그건 아닌것 같아 service 계층으로 모두 옮겨서 진행했습니다!

Comment on lines +15 to +19
@Transactional
fun addLocationHistory(user: User, keyword: String, roadAddress: RoadAddress): SearchLocationHistory {
val history = SearchLocationHistory(user = user, keyword = keyword, roadAddress = roadAddress)
return locationHistoryRepository.save(history)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

성향 차이일것 같은데요, 코틀린이여서 굳이 필요 없을것 같기도 한대
저는 return 시 결괏값을 따로 지정해서 보내줘야 보기 편한것 같다고 생각이 들어서요.
예를 들면 val findLocationHistory = locationHistoryRepository.save(history), return findLocationHistory 같이요.
리턴값이 명확하게 적혀 있으니 괜찮을것 같긴 한데..

Copy link
Member Author

Choose a reason for hiding this comment

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

저도 좀 고민했던 점인데 일단 인텔리제이에서 밑줄 생기는게 신경쓰여서... ㅎㅎ 그리고 return 하기전에 추가로 해야되는 작업이 있으면 그때 바꾸면 되지 않을까 싶어서 이런식으로 했습니다.

@jiseongTak jiseongTak merged commit ad605e7 into dev Oct 10, 2023
1 check failed
@jiseongTak jiseongTak deleted the feature/jstak/kakao branch October 26, 2023 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants