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주차 과제_Step2 #97

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

Conversation

fiveJinw
Copy link

@fiveJinw fiveJinw commented Jul 12, 2024

오늘도 코드리뷰 부탁드립니다!

Step2 code : https://github.com/kakao-tech-campus-2nd-step2/android-map-search/pull/97/files/96d5f306b36dba9ad8278f1b02256da9330c43dd..3349aba86d26712151afe17a96d66166afea6c69

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

  • SDK를 Develop 페이지를 보며 구현하는 것은 처음이라 좀 어색했던 것 같습니다.
    • android Develop 페이지가 자세하게 설명되어 있다는 것을 깨달았습니다..
  • 아직 MVVM 패턴에 대해 익숙치 않은 것 같습니다. 액티비티 하나가 늘어날 때마다 어떠한 구조로 작성해야 할 지 감이 아직 안잡히는 것 같습니다.
    • 안드로이드 권장 아키텍쳐를 읽어보았지만 실제 적용하기에는 아직 어려운 것 같습니다.
  • 네트워크 오류처리를 어떻게 해야할 지 모르겠습니다.
    • 코드 단에서 log로 어떠한 문제가 발생했는 지 체크하는 것 외에는 어떤 방식으로 에러를 핸들링해야할 지 잘 모르겠습니다.

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

  • 코드 전체적으로 어색한 부분이 없는지, 혹은 개선사항이 있을지
  • 기능이나 역할을 분리할만한 코드나 클래스가 있는지

추가로 궁금한 점

  • makeSceneTransitionAnimation 으로 애니메이션을 적용해보려고 하였습니다.
    • requestFocus()가 makeSceneTransitionAnimation 애니메이션을 적용하면 제대로 동작하지 않는 것 같습니다.
    • Activity간 이동할 때 애니메이션을 어떤 방식을 사용하여 구현하는지 궁금합니다.
  • 기본적으로 에러처리를 어떠한 방식으로 하는 지 궁금합니다.
    • 또한 Repository에서 네트워크 에러가 발생할 때 어느 클래스에서 처리를 해줘야 하는지, 에러처리를 위한 클래스를 따로 만드는 편인지 궁금합니다.

언제나 바쁘실탠데 시간 내주셔서 감사합니다!

@fiveJinw fiveJinw changed the title Step2 전남대 Android_오진우_3주차 과제_Step2 Jul 12, 2024
@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.

3주차 과제도 진우님 고생 많으셨어요 👍


코드 전체적으로 어색한 부분이 없는지, 혹은 개선사항이 있을지
기능이나 역할을 분리할만한 코드나 클래스가 있는지

이부분은 코멘트 확인해주시면 좋을 것 같습니다! step1 도 함께 확인해주세요!

makeSceneTransitionAnimation 으로 애니메이션을 적용해보려고 하였습니다.
requestFocus()가 makeSceneTransitionAnimation 애니메이션을 적용하면 제대로 동작하지 않는 것 같습니다.
Activity간 이동할 때 애니메이션을 어떤 방식을 사용하여 구현하는지 궁금합니다.

API Key 가 존재하지 않아 실행시켜보지 못해 정확히 어떤 문제가 있는지 파악이 힘드네요... 일단 inputFieldTransition 이 두군데 존재하여 애니메이션이 제대로 작동하지 않는 것이 아닐까 추측해봅니다! 또한 Actvity 간 애니메이션은 적용해주신 것 처럼 shared elements 를 이용한 transition 을 주로 사용합니다 :)

기본적으로 에러처리를 어떠한 방식으로 하는 지 궁금합니다.
또한 Repository에서 네트워크 에러가 발생할 때 어느 클래스에서 처리를 해줘야 하는지, 에러처리를 위한 클래스를 따로 만드는 편인지 궁금합니다.

에러처리는 너무 광범위하긴 하네요.. 일단 어떤 에러처리냐에 따라 달라질 것 같기도 하구요. 다만 retrofit 에서의 에러처리 같은 경우 step1 의 리뷰를 참고해보시면 좋을 것 같아요!
또한 에러처리는 최대한 ViewModel 에서 처리해주시는 것이 좋다고 생각합니다. Repository 에서 하는 경우도 있구요. 어디서 하느냐는 큰 문제는 없지만, 어떤 에러가 발생했는지 는 별도의 Exception 클래스를 정의하여 처리하는 경우가 많습니다.

interface KakaoLocalApi {
@GET("v2/local/search/keyword.json")
fun getPlaceData(
@Header("Authorization") key: String,

Choose a reason for hiding this comment

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

보통 Authorization 같은 경우에는 모든 API 의 요청에 추가되어야하는 경우가 많습니다. 이럴 때 매번 endpoint 마다 @Header 를 추가하는 것은 번거로운 일이 됩니다.

OkHttpClient 의 request interceptor 를 통해 Header 에 authorization 을 추가해보는건 어떨까요?
https://square.github.io/okhttp/features/interceptors/

Comment on lines 20 to 24
val retrofitService = Retrofit.Builder()
.baseUrl(BuildConfig.KAKAO_BASE_URL)
.addConverterFactory(GsonConverterFactory.create())
.build()
val kakaoApi = retrofitService.create(KakaoLocalApi::class.java)

Choose a reason for hiding this comment

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

retrofit 과 api 의 생성은 대부분 한번만 이루어지는 것이 좋습니다. 여기에서는 getPlaceData 가 호출될 때마다 생성이 되기 때문에 리소스 낭비가 발생할 수 있습니다. 이부분 한번만 생성하는 쪽으로 고민해주세요 :)

Choose a reason for hiding this comment

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

object 로 선언해주셔도 좋고, hilt 를 사용하신다면 singleton component 로 제공해도 좋습니다.

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