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

전남대 Android_오진우_3주차 과제_Step1 #70

Open
wants to merge 8 commits into
base: fivejinw
Choose a base branch
from

Conversation

fiveJinw
Copy link

@fiveJinw fiveJinw commented Jul 11, 2024


코드 작성하면서 어려웠던 점

  • 비동기적으로 작업이 이루어지는 카카오 API를 사용하여 LiveData의 값을 언제 갱신해야 할 지, 어떻게 갱신해야 할 지 감을 잡기 어려웠던 것 같습니다.
  • 처음에는 Repository에게 LiveData를 직접 넘겨줘서 Repository 내에서 값을 갱신하는 방식으로 구현하였습니다. 이러한 방식이 좋지않은것 같아서 다른 방식으로 구현해보려고 하였습니다.
  • 결국 onResponse 내에서 callback 함수를 호출해서 ViewModel 내에서 값을 갱신하는 방식으로 구현했는데, 이러한 방식이 옳은것인지 잘 모르겠습니다.

멘토님께서 중점적으로 리뷰해주셨으면 하는 부분

  • 데이터 요청과 데이터 반환 및 UI 반영이 효율적으로 이루어 지는지

추가로 궁금한 점

  • 카카오API로 데이터를 받도록 바꾸다보니 원래의 PlaceRepository를 사용하지 않게 되었습니다. 이렇게 사용하지 않는 코드들은 모두 지워버리는 것이 좋은 지 궁금합니다!

오늘도 코드리뷰 부탁드립니다!
언제나 감사드립니다 :)

@bigstark bigstark self-requested a review July 14, 2024 15:19
Copy link

@bigstark bigstark left a comment

Choose a reason for hiding this comment

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

고생하셨어요 진우님! 전반적으로 아키텍쳐에 대한 학습을 더 하시면 큰 도움이 되지 않을까 싶습니다!


카카오API로 데이터를 받도록 바꾸다보니 원래의 PlaceRepository를 사용하지 않게 되었습니다. 이렇게 사용하지 않는 코드들은 모두 지워버리는 것이 좋은 지 궁금합니다!

데이터를 가지고 오는 목적지(data source 라고 부릅니다.) 가 달라졌기 때문에 PlaceRepository 는 그대로 두고, KakaoApiDataSource 로 처리하심이 좋습니다. 관심사 분리와 의존성 주입에 대해서 차근차근 찾아보시면 좋을 것 같네요 👍

Comment on lines +23 to +25
buildConfigField("String", "KAKAO_LOCAL_API_KEY", gradleLocalProperties(rootDir, providers).getProperty("KAKAO_LOCAL_API_KEY"))
buildConfigField("String", "KAKAO_BASE_URL", gradleLocalProperties(rootDir, providers).getProperty("KAKAO_BASE_URL"))

Choose a reason for hiding this comment

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

👍

Comment on lines +10 to +12
@SerializedName("road_address_name") val location : String,
@SerializedName("category_group_name") val category : String

Choose a reason for hiding this comment

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

Gson 을 사용한다면 null safety 하지 않을 수 있어요. 모두 null 처리해주시면 조금 더 안전하게 사용할 수 있어요.

Comment on lines +11 to +15
fun getPlaceData(
@Header("Authorization") key: String,
@Query("query") query: String

): Call<ResultSearch>

Choose a reason for hiding this comment

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

suspend fun 으로 선언한다면 ResultSearch 를 그대로 받을 수 있습니다. Call 을 사용하는 것은 너무 오래전 사용방법이고, 깊은 callback 을 유발할 수 있기 때문에 선호되지 않습니다.

https://seokzoo.tistory.com/4

Comment on lines +17 to +47
fun getPlaceData(text: String, callback: (List<Place>) -> Unit) {
Log.d("inputField", "inputText : ${text} ")
var placeList = listOf<Place>()
val retrofitService = Retrofit.Builder()
.baseUrl(BuildConfig.KAKAO_BASE_URL)
.addConverterFactory(GsonConverterFactory.create())
.build()
val kakaoApi = retrofitService.create(KakaoLocalApi::class.java)

kakaoApi.getPlaceData(BuildConfig.KAKAO_LOCAL_API_KEY, text)
.enqueue(object : Callback<ResultSearch> {
override fun onResponse(
call: Call<ResultSearch>,
response: Response<ResultSearch>
) {
if (response.isSuccessful) {
val body = response.body()
Log.d("testt", "body : ${body?.documents}")
placeList = body?.documents ?: listOf<Place>()
Log.d("inputField", "placeList : ${placeList} ")
callback(placeList)
}
}

override fun onFailure(call: Call<ResultSearch>, t: Throwable) {
Log.d("testt", "error : $t")
Log.d("inputField", "placeList : ${placeList} ")
callback(placeList)
}
})
}

Choose a reason for hiding this comment

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

callback 으로는 잘 처리해주셨으나, kotlin coroutine 의 기능을 조금 더 잘 이용해보면 좋지 않을까 싶어요! 만약 모르신다면, 공부해보시면 큰 도움이 될 것 같습니다 :)

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