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

Doodung step2 #7

Open
wants to merge 3 commits into
base: doodung
Choose a base branch
from
Open

Doodung step2 #7

wants to merge 3 commits into from

Conversation

doodung
Copy link

@doodung doodung commented Dec 1, 2021

Description

(아직) 유저 검색 화면만 변경했습니다.
DiffUtil , SingleLiveEvent, BindingAdapter 사용
버튼 클릭 등은 STEP3에 반영해서 바꿀 예정...

@doodung doodung self-assigned this Dec 1, 2021
Copy link
Contributor

@jinsu4755 jinsu4755 left a comment

Choose a reason for hiding this comment

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

과제 넘모 고생했고

대부분 일단 수정한다고 되어있는 것 같아서 이외로 살펴보면 좋을 부분들 리뷰 남길께

화이팅이고~~좀만 더 가면 될 것 같다

Comment on lines +6 to +10
<data>
<variable
name="DetailUserActivity"
type="place.pic.android.plus.detail.DetailUserActivity" />
</data>
Copy link
Contributor

Choose a reason for hiding this comment

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

엑티비티가 데이터바인딩 데이터로 들어있는데 실질적으로 아무것도 하지 않네

엑티비티를 따로 받은 이유가 있을까?

Copy link
Author

Choose a reason for hiding this comment

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

DetailUserActivity 부분은 아직..손을 안댓습니다 죄송합니다

setBindingUserData()
}

private fun setBindingUserData() {
Copy link
Contributor

Choose a reason for hiding this comment

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

일단 이 함수가 하는 일은 무엇일까? 보다 간단한 이름이 있지는 않을지 고민해보면 좋을 것 같다는 생각이 들어

보다 간단한 이름을 생각해보면 이 함수가 하는 일이 과연 엑티비티가 해야할 역할에 포함되는지 생각해볼 수 있을 것 같아

Copy link
Author

Choose a reason for hiding this comment

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

넵! 수정하게씁니다!

import place.pic.android.plus.databinding.ActivityDetailUserBinding

class DetailUserActivity : AppCompatActivity() {
private lateinit var binding: ActivityDetailUserBinding
Copy link
Contributor

Choose a reason for hiding this comment

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

코틀린은 불변성을 권장하는 언어라는 특징이 있어 "권장" 이기 때문에 대부분의 경우에서는 불변성을 자동으로 제공해주지만 프로그래머에게 그 역할이 위임 되어있는 경우도 존재한다 생각해

여기선 큰 문제점은 보이지 않지만 앞으로 코틀린을 계속 다룬다면 한번은 생각해보고 넘어가면 좋을 것 같아

애초에 여기서 binding 변수는 반드시 프로퍼티로 존재할 이유가 없다고 생각

Copy link
Author

Choose a reason for hiding this comment

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

옳습니다

fun onClick(view: View, position: Int)
}

var itemClick: ItemClick? = null
Copy link
Contributor

Choose a reason for hiding this comment

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

변수 선언 위치 지켜주십쇼!

Copy link
Author

Choose a reason for hiding this comment

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

네 선생님

Comment on lines +46 to +48
interface ItemClick {
fun onClick(view: View, position: Int)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

SAM 변환을 사용하는 게 어떨까?

Copy link
Author

Choose a reason for hiding this comment

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

넵! 사용해보겠습니다

Comment on lines +22 to +26
if (itemClick != null) {
holder.itemView.setOnClickListener { v ->
itemClick?.onClick(v, position)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

if문에서 null 체크를 하는 의미가 있을까?

Copy link
Author

Choose a reason for hiding this comment

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

없읍니다.

class SearchUserViewHolder(private val binding: ItemUserSearchBinding) : RecyclerView.ViewHolder(binding.root) {
fun bind(searchUserData: SearchUserData) {
binding.user = searchUserData
binding.executePendingBindings()
Copy link
Contributor

Choose a reason for hiding this comment

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

혹시 강제로 바인딩을 해야만 유저 데이터가 뜨는거야?

import java.io.IOException

class SearchUserViewModel : ViewModel() {
private val _recyclerListData = SingleLiveEvent<List<SearchUserData>>()
Copy link
Contributor

Choose a reason for hiding this comment

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

리스트 데이터를 한번만 emit 하는 객체로 만든 이유가 있을까?

예를들어 onCreate()가 다시 호출되어 Observer가 active 상태가 되었는데 그때 이 리사이클러 리스트 데이터는 이전에 가지고 있던 값을 emit 하면 안되는 걸까?

Copy link
Author

Choose a reason for hiding this comment

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

죄송함다.. 사실 싱라를 어느 순간(?)에 사용하는지 잘 몰라서 여기에 쓰는줄 알고 썼는데
찾아보니 Dialog, Toast, SnackBar 같이 한번만 떠야 하는 곳에 쓴다고 하더라고용
그래서 다혜랑 저번주에 고민한게
진수가 쓰라했던 싱라를 어디에 써야하냐...!? 였습니다.
지금 우리 예제에서 어느 부분에 써야 할까요......................................


class SearchUserViewModel : ViewModel() {
private val _recyclerListData = SingleLiveEvent<List<SearchUserData>>()
val recyclerListData: LiveData<List<SearchUserData>>
Copy link
Contributor

Choose a reason for hiding this comment

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

변수 이름 관련 내용인데 이 변수가 담고 있는 것이 재사용 리스트 데이터 일까?

Copy link
Author

Choose a reason for hiding this comment

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

아.. 재사용 리스트 데이터는 아니고
리사이클러뷰에 들어가는 데이터라서 리사이클러 리스트 데이터라고 했습니다 (...)
수정하겠습니다!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

}

private fun setBindingUserData() {
val user = intent.getSerializableExtra("user") as SearchUserData
Copy link
Contributor

Choose a reason for hiding this comment

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

intent.getSerializableExtra("user") 이 친구의 결과물은 뭘까?

지금 요구 사항에서는 필요 없는 부분 이지만

만약 DetailUserActivity 가 user 라는 정보를 받지 못하고 실행되었다면 어떻게 되어야 할지 처리해주는 것도 고려해봐야 하지 않을까?

Copy link
Author

Choose a reason for hiding this comment

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

넵! 반영하겠습니다.

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