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주차 step2 과제 #80

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

Conversation

leeghy
Copy link

@leeghy leeghy commented Jul 26, 2024

감사합니다

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.

가현님 5주차 리팩토링도 고생 많으셨어요! 코멘트 확인해주시고 조금만 수정해주시면 좋을 것 같네요 :)


많은 부분들이 변경된 것 같아서 매우 좋네요! 조금식 패턴들에 익숙해지고 있는 것 같아서 좋습니다!

Comment on lines +16 to +21
return ViewHolder(
ItemViewBinding.inflate(
LayoutInflater.from(parent.context),
parent,
false
)

Choose a reason for hiding this comment

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

👍

override fun onBindViewHolder(holder: ViewHolder, position: Int) {
val location = getItem(position)
holder.bind(location)
holder.itemView.setOnClickListener { onItemClick(location) }

Choose a reason for hiding this comment

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

onBindViewHolder 시점에 setOnClickListener 는 성능이 좋지 않습니다. 안티패턴이에요.
holder 내부의 init 시점으로 옮겨주시는 편이 좋습니다.

ViewHolder 생성 시 onItemClick lambda 도 같이 추가하면 좋을 것 같네요 :)

private fun saveSearchList() {
val sharedPref = getPreferences(Context.MODE_PRIVATE)
val sharedPref = getPreferences(MODE_PRIVATE)

Choose a reason for hiding this comment

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

shared preference 로 처리하는 로직도 viewModel 로 옮겨주시면 어떨까요?
추가적으로 shared preference 또한 주입받는 대상으로 처리해주시는 것이 좋습니다!

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