-
Notifications
You must be signed in to change notification settings - Fork 33
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 남지연 2주차 과제 Step 2 #47
base: njiyeon
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
한 주 동안 수고 많으셨습니다. 리뷰 남겨드리니 의견 부탁드립니다 :)
현재 RecyclerView에서 cursor를 갖고 있어서 DB와 Adapter가 직접적으로 연결되어 있습니다. 이에 대한 내용은 리뷰로 남겨놓았으니 ViewModel 쪽으로 Item을 갖고 오도록 변경해보세요.
DiffUtil은 애니메이션 성능 뿐 아니라 아이템이 변경될 때 변경되는 부분만 다시 그려서 성능적인 효율을 높일 수 있는 장치입니다. 현재 프로젝트에도 충분히 적용하면 성능적인 이점이 있을 듯 합니다.
혹시 시간적 여유가 있으시면 ListAdapter를 활용해보시는 것도 권장드립니다.
감사합니다.
cursor?.apply { | ||
moveToPosition(position) | ||
val name = getString(getColumnIndexOrThrow(HistoryContract.COLUMN_NAME)) | ||
holder.bind(name) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어떤 로직을 원하신 건가요?
|
||
class HistoryAdapter(private val onDelete: (String) -> Unit) : RecyclerView.Adapter<HistoryAdapter.ViewHolder>() { | ||
|
||
private var cursor: Cursor? = null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor를 Adapter까지 끌고 들어오는건 좋지 않아요. DB와 연결되는 객체이기 때문에 Adapter의 역할을 한참 벗어난것 같네요. ViewModel 단에서 Item을 load 하고 Adapter는 단순히 불러온 아이템을 표현하는 용도로만 사용해주세요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
쉽게 말하자면 신뢰할 수 있는 데이터의 집합인 LocalDB 에서 Adapter 까지 내려와야 합니다. LiveData 등을 활용해서
LocalDB -> Repository(Optional) -> ViewModel -> View -> Adpater 로 데이터가 흐르게끔 작성해보세요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 이해한 것이 맞다면, Cursor를 Adapter에서 직접 사용하는 게 아니라 ViewModel에서 데이터를 불러와 Adapter에 전달하도록 바꾸면 될까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네 맞습니다 :)
viewModel.places.observe(this, Observer { cursor -> | ||
placeAdapter.submitCursor(cursor) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Data Observing 하는 로직도 분리하면 좋겠네요.
historyAdapter.submitCursor(cursor) | ||
}) | ||
|
||
binding.etSearch.addTextChangedListener(object : TextWatcher { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addTestChangedListener 확장함수 잘 찾아보시면 원하는 함수만 재정의할 수 있습니다.
그리고 이렇게 만든 TextWatcher는 View가 Destroy될 때 해제해주세요
moveToPosition(position) | ||
val name = getString(getColumnIndexOrThrow(HistoryContract.COLUMN_NAME)) | ||
holder.bind(name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 잘 안읽히는 이유 중 하나도 apply의 역할때문입니다. apply는 객체의 속성을 정할 때 많이 사용되어서 getString, getColumnIndexOrThrow가 어디에 속한 메서드인지 명확하게 파악하기 힘드네요.
이 경우에는 apply 말고 그냥 plain 하게 작성하시는게 이해하기 편할 것 같습니다
binding.root.setOnClickListener { | ||
onClick(name) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
클릭 리스너를 bind 때에 작성하는건 Anti-pattern 입니다. 왜 그런지 생각해보시고 리팩토링 해주세요!
어려웠던 부분
MVVM
패턴을 적용하기DiffUtil
사용하기중점적으로 리뷰해주셨으면 하는 부분
RecyclerView
를 알맞게 적용한 것이 맞는지 궁금합니다.notifyDataSetChange()
호출로 인해 발생한 오버헤드가 있어서,DiffUtil
을 사용하면 애니메이션 적용과 성능 향상 측면에서 장점이 있다는 것을 알게 되었는데, 적용하는 데는 어려움을 느껴 해당 기능을 사용하지는 못했습니다. 만약 여기서DIffUtil
을 이용한다면 데이터 클래스를 선언하고, 리스트를 갱신하는 방식으로 사용하면 되는 것이 맞는지 궁금합니다.실행 화면