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_이지은 5주차 1단계 #60

Open
wants to merge 11 commits into
base: jieunyume
Choose a base branch
from

Conversation

JieunYume
Copy link

어려웠던 점

  • Room과 Hilt 둘 다 써보지 않아서 헤맸는데.. Hilt가 더 어려웠다. Component와 Scope를 신경써서 의존성 선언을 해야 해서 단순하지만은 않았다.

중점적으로 봐주셨으면 하는 부분

  • 3개의 모듈을 만들었는데, 컴포넌트를 정하는 기준에 대해서 모르겠어서 모두 SingletonComponent로 설정해뒀습니다. 저 혼자 생각하기로는.. Repository가 ViewModel에서 쓰이고, ViewModel은 Activity의 생명 주기에 영향을 받지 않으니까 Repository의 의존성 선언을 할 때는 ViewModelComponent로 설정해도 되지 않을까? 라는 생각을 해봤습니다.. 컴포넌트를 정하는 기준을 잘 모르겠습니다..

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.

지은님 step1 고생 많으셨어요! 코멘트 확인해주시고, 수정 부탁드려요 :)

Comment on lines +16 to +17
// ViewModelComponent로 설정해도 될까요? Repository가 ViewModel에서 쓰이니까 ViewModelComponent를 쓰면 되겠다고 생각했는데요..
// 적절한 Component를 선택하는 기준이 뭔지 모르겠습니다!

Choose a reason for hiding this comment

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

본인이 정하기 나름대로 사용하시면 됩니다. 만약 Repository 자체가 전역에서 사용이 될 수 있다고 한다면 SingletonComponent 를 쓰는 것이 적당하겠고, 특정 ViewModel 에서만 사용이 된다면 ViewModelComponent 를 사용하시면 됩니다.

보통 이런 부분은 소규모 과제에서 체감되기 쉽지 않아요. 조금 더 많은 기능을 구현하다보면 자연스럽게 깨닫는 부분이 오지 않을까 싶어요 :)

private const val RESULT_SIZE = 15
}

private val client = RetrofitInstance.getInstance().create(KakaoAPI::class.java)

Choose a reason for hiding this comment

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

KakaoApi 를 주입받으면 조금 더 좋은 코드가 될 수 있겠네요 :)

import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.withContext

class LocationApi {

Choose a reason for hiding this comment

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

또한 LocationApi 는 kakao api 라는 remote data source 를 사용하는 것이니, LocationRemoteDataSource 정도로 사용해보시면 어떨까요?

private val client = RetrofitInstance.getInstance().create(KakaoAPI::class.java)

suspend fun getLocations(keyword: String): SearchFromKeywordResponse? {
return withContext(Dispatchers.IO){

Choose a reason for hiding this comment

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

dispatchers 또한 하나의 주입을 받는 대상이라고 인식하시는 것이 조금 더 좋습니다!

https://developer.android.com/kotlin/coroutines/coroutines-best-practices

Comment on lines +9 to +14
App.sharedPreferencesManager.putString("id", location.id.toString())
App.sharedPreferencesManager.putString("longitude", location.longitude.toString())
App.sharedPreferencesManager.putString("latitude", location.latitude.toString())
App.sharedPreferencesManager.putString("title", location.title.toString())
App.sharedPreferencesManager.putString("address", location.address.toString())
App.sharedPreferencesManager.putString("category", location.category.toString())

Choose a reason for hiding this comment

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

SharedPreferences 도 App 에 직접 접근하는 것이 아닌, hilt 를 통해서 주입받을 수 있도록 해주세요!

Choose a reason for hiding this comment

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

또한 DataStore 로 대체하는 작업을 해보시는 것은 어떨까요?

} else return null
private fun getLocationByIntent(): Location? {
if (intent.hasExtra("location")) {
val location = intent.getSerializableExtra("location") as Location

Choose a reason for hiding this comment

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

serializable 보다는 parcelable 을 사용해주세요. 굳이 serializable 을 사용하는 것 보다는 이전처럼 필드 하나하나를 받는 것이 더 좋습니다.

parcelable >>>>>>>>> 필드정의 >> serializable

Copy link
Author

Choose a reason for hiding this comment

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

serializable로 하면 reflection을 사용하기 때문에 성능 저하로 이어질 수 있군요!

Comment on lines +43 to +44
val savedLocation = getItem(bindingAdapterPosition) as SavedLocation
itemSelectedListener.onSavedLocationXButtonClicked(savedLocation)

Choose a reason for hiding this comment

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

요것도 리팩토링 대상으로 봐주시는 것이 좋을 것 같아요.

ViewHolder 를 inner class 로 정의하여 adapter 에 접근하는 것은 안티패턴이에요!

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