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 남지연 2주차 과제 Step 2 #47

Open
wants to merge 18 commits into
base: njiyeon
Choose a base branch
from

Conversation

nJiyeon
Copy link

@nJiyeon nJiyeon commented Jul 5, 2024

어려웠던 부분

  • MVVM 패턴을 적용하기
  • DiffUtil 사용하기

중점적으로 리뷰해주셨으면 하는 부분

  • RecyclerView를 알맞게 적용한 것이 맞는지 궁금합니다.
  • Step 1의 기능 구현에 대한 질문 사항은 해당 PR에 따로 남겼습니다.!
  • notifyDataSetChange() 호출로 인해 발생한 오버헤드가 있어서, DiffUtil을 사용하면 애니메이션 적용과 성능 향상 측면에서 장점이 있다는 것을 알게 되었는데, 적용하는 데는 어려움을 느껴 해당 기능을 사용하지는 못했습니다. 만약 여기서 DIffUtil을 이용한다면 데이터 클래스를 선언하고, 리스트를 갱신하는 방식으로 사용하면 되는 것이 맞는지 궁금합니다.
  • 전반적인 UI가 깔끔하지 않은데 현재 xml 파일들에서 나타나는 가장 큰 문제가 무엇인지 궁금합니다.

실행 화면

image

Copy link

@mkSpace mkSpace left a 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를 활용해보시는 것도 권장드립니다.

감사합니다.

Comment on lines 29 to 33
cursor?.apply {
moveToPosition(position)
val name = getString(getColumnIndexOrThrow(HistoryContract.COLUMN_NAME))
holder.bind(name)
}
Copy link

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
Copy link

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는 단순히 불러온 아이템을 표현하는 용도로만 사용해주세요

Copy link

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 로 데이터가 흐르게끔 작성해보세요

Copy link
Author

Choose a reason for hiding this comment

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

제가 이해한 것이 맞다면, Cursor를 Adapter에서 직접 사용하는 게 아니라 ViewModel에서 데이터를 불러와 Adapter에 전달하도록 바꾸면 될까요?

Copy link

Choose a reason for hiding this comment

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

네 맞습니다 :)

Comment on lines 31 to 33
viewModel.places.observe(this, Observer { cursor ->
placeAdapter.submitCursor(cursor)
})
Copy link

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 {
Copy link

Choose a reason for hiding this comment

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

addTestChangedListener 확장함수 잘 찾아보시면 원하는 함수만 재정의할 수 있습니다.
그리고 이렇게 만든 TextWatcher는 View가 Destroy될 때 해제해주세요

Comment on lines 30 to 32
moveToPosition(position)
val name = getString(getColumnIndexOrThrow(HistoryContract.COLUMN_NAME))
holder.bind(name)
Copy link

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 하게 작성하시는게 이해하기 편할 것 같습니다

Comment on lines 19 to 21
binding.root.setOnClickListener {
onClick(name)
}
Copy link

Choose a reason for hiding this comment

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

클릭 리스너를 bind 때에 작성하는건 Anti-pattern 입니다. 왜 그런지 생각해보시고 리팩토링 해주세요!

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