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

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

Conversation

fiveJinw
Copy link

@fiveJinw fiveJinw commented Jul 5, 2024

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

  • MVVM 패턴을 제대로 적용하고 있는지 감이 잘 안잡혔습니다.
  • activity에서 LiveData를 관찰하면서 데이터가 바뀌면 recyclerView를 갱신하는 방법을 구현하는 것이 어려웠습니다.
  • ViewModel, Repository를 사용하여 데이터를 갱신하는 과정을 구현하는 것이 어려웠던 것 같습니다.

코드 리뷰 시 멘토님이 중점적으로 리뷰해주셨으면 하는 부분

  • activity에서 이벤트가 발생할 때마다 ViewModel과 통신 방식이 적절한지
  • LiveData 타입의 데이터 갱신을 하는 방법이 적절한지
  • RecyclerView에서 이벤트가 발생할 때마다 갱신 과정이 적절한지

추가 질문

  • 제 코드에서 notifyDatachanged를 이용하여 데이터가 바뀔 때마다 갱신해주는 방식으로 구현하였습니다.
  • 이 방식으로 구현하니 검색창에서 하나씩 칠 때마다 notifyDataChanged가 발생하게 되서 비효율적인 것 같습니다. 제가 구현한 방식보다 더 좋은 방식이 있는지 궁금합니다!

언제나 감사드립니다!

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.

고생하셨어요!


제 코드에서 notifyDatachanged를 이용하여 데이터가 바뀔 때마다 갱신해주는 방식으로 구현하였습니다.
이 방식으로 구현하니 검색창에서 하나씩 칠 때마다 notifyDataChanged가 발생하게 되서 비효율적인 것 같습니다. 제가 구현한 방식보다 더 좋은 방식이 있는지 궁금합니다!

코멘트에서 확인하실 수 있는 것처럼 ListAdapter 를 사용한다면 notifyDataSetChanged 의 비효율을 제거할 수 있을 것 같습니다 :)

또한 Room 을 사용하신다면 조금 더 효율적으로 관리가 가능할 것 같습니다.

import campus.tech.kakao.map.model.Place

class PlaceViewAdapter(
val placeList: LiveData<MutableList<Place>>,
Copy link

Choose a reason for hiding this comment

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

PlaceViewAdapter 에서 LiveData 를 파라미터로 받고 있는데, 그런 사용은 지양해주시면 좋을 것 같아요! adapter 내에서 observe 하는 것도 없고, adapter 에서 필요한 것은 List 일 뿐이니까요!


class PlaceViewAdapter(
val placeList: LiveData<MutableList<Place>>,
val inflater: LayoutInflater,
Copy link

Choose a reason for hiding this comment

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

inflater 를 굳이 parameter 를 통해서 받을 필요는 없습니다. onCreateViewHolder 의 parent: RecyclerView 의 context 를 사용하시면 됩니다 :)

    override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): ViewHolder {
        val inflater = LayoutInflater.from(parent.context)
        val view = inflater.inflate(R.layout.place_item, parent, false)
        return PlaceViewHolder(view)
    }

val listener: OnClickPlaceListener
) : RecyclerView.Adapter<PlaceViewAdapter.PlaceViewHolder>() {

inner class PlaceViewHolder(itemView: View) : RecyclerView.ViewHolder(itemView) {
Copy link

Choose a reason for hiding this comment

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

여기도 또한 inner class 가 아닌 형태로 처리하는 것으로 고려해주시면 좋을 것 같아요! ViewHolder 에서 adapter 를 접근하는 것이 올바른 형태는 아니에요!

Comment on lines +5 to +17
object PlaceContract{
object PlaceEntry : BaseColumns {
const val TABLE_NAME = "PLACE"
const val COLUMN_NAME = "NAME"
const val COLUMN_LOCATION = "LOCATION"
const val COLUMN_CATEGORY = "CATEGORY"
}

object SavedPlaceEntry : BaseColumns{
const val TABLE_NAME = "SAVED_PLACE"
const val COLUMN_NAME = "SAVED_NAME"
}
}
Copy link

Choose a reason for hiding this comment

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

👍

viewModel.place.observe(this, Observer {
Log.d("readData", "검색창 결과 변경 감지")
val place = viewModel.place
searchRecyclerViewAdapter.notifyDataSetChanged()
Copy link

Choose a reason for hiding this comment

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

말씀해주신 것처럼 notifyDataSetChanged 는 사용하지 않는 것이 좋아요. notifyItemInserted 나 removed, moved 등을 사용하는 것이 좋은데, 그러려면 data 간 diff 를 알아야하니 매우 번거롭습니다. 이를 손쉽게 할 수 있는 것이 ListAdapter 에요! ListAdapter 를 사용하여 처리한다면 조금 더 효율적으로 처리가 가능해질 것이에요!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants