dahyee step2 #8
dahyee step2 #8
Conversation
| userDetailViewModel.userDetail.observe(this) { | ||
| // userDetailViewModel.setUserDetail() | ||
| } |
There was a problem hiding this comment.
필요 없는 코드면 지워도 될 것 같다는 생각이 듭니다~
| val binding = ActivityUserDatailBinding.inflate(layoutInflater) | ||
| binding.viewModel = userDetailViewModel | ||
| binding.lifecycleOwner = this | ||
| val username = intent.getStringExtra("username").toString() |
There was a problem hiding this comment.
intent.getStringExtra("username") 이 친구의 결과물은 뭘까?
지금의 요구 사항에선 필요 없는 부분이지만
만약 UserDetailActivty가 username을 받지 않고 실행되었다면 어떻게 되어야 할 것 같은지 고려해봐야 하지 않을까?
|
|
||
| private val token = "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" | ||
|
|
||
| private lateinit var user: UserDetail |
There was a problem hiding this comment.
코틀린은 불변성을 권장하는 언어라는 특징이 있어 "권장" 이기 때문에 대부분의 경우에서는 불변성을 자동으로 제공해주지만 프로그래머에게 그 역할이 위임 되어있는 경우도 존재한다 생각해
큰 문제가 된다고 보여지지는 않지만
애초에 여기서 user: UserDetail 가 프로퍼티로 존재할 이유가 없다고 생각해
| override fun onResponse(call: Call<UserDetail>, response: Response<UserDetail>) { | ||
| if (response.isSuccessful) { | ||
| user = UserDetail( | ||
| imageUrl = response.body()?.imageUrl, | ||
| name = response.body()!!.name, | ||
| bio = response.body()?.bio, | ||
| followers = response.body()!!.followers, | ||
| htmlUrl = response.body()!!.htmlUrl | ||
| ) | ||
| } | ||
| setUserDetail() | ||
| } |
There was a problem hiding this comment.
만약 서버에서 status 가 200~300이 아닌 경우의 처리도 필요하지 않을까?
그럴 일이 있을진 모르겠지만 만약 검색하고 결과에는 유저가 나오는데
검색 이후 ~ 디테일 페이지 열기 전 서버에서 유저 정보가 삭제된 경우가 절대 없다고 할 수는 없지 않을까?
혹은 모종의 이유로 200~300사이가 떨어지지 않을 수 있는 것이라 생각해
| fun setItem(users: List<User>) { | ||
| data = users as MutableList<User> | ||
| notifyDataSetChanged() | ||
| } |
There was a problem hiding this comment.
어뎁터 내부로 리스트를 추가하고 notify 하는 역할을 주는 것은 매우 잘한 일이라 생각해 👍
다만 아쉬운 것은 MutableList 를 사용하지만 MutableList를 사용하는 이유가 어뎁터 내부에 없는 것 같아
결국 MutableList 의 참조를 변경해버릴 것이면 MutableList 를 사용하는 이유가 없지 않을까?
개인적으로는 MutableList를 사용하면서 이를 더 잘 활용할 수 있는 방법을 사용해보면 좋을 것 같아!
늘어나는 마법의 주머니를 가지고 있으면서
늘어나지도 않는 주머니를 가져와서 늘어나는 마법 주머니로 변경 시키고 있는 것 같아 보여
|
|
||
| fun bind(user: User) { | ||
| binding.user = user | ||
| binding.clickListener = createUserItemClickListener(user) |
There was a problem hiding this comment.
이 친구로 인해서 뷰홀더가 inner class로 선언되어야만 한 것 같은데
다른 여러 방법들이 있는 것 같아
다른 방법들도 조금 더 고민해보고 더 좋은 수단이 있다면 이를 사용해보는 것이 어떨까?
아이템 뷰의 부모 뷰의 클릭 이벤트를 설정한다는 관점에서 보면 이때 클릭 이벤트는 뷰홀더의 일이어야 할 것 같은 생각이 들어
다반 반대로 아이템을 눌렀을때 이벤트라는 관점에서 보면 아이템을 관리하는 놈의 일이라고 볼 수 있지 않을까?
| userSearchViewModel.showErrorToast.observe( | ||
| this, | ||
| Observer { | ||
| it.getContentIfNotHandled().let { | ||
| Toast.makeText(this, "검색어를 입력하세요!", Toast.LENGTH_SHORT).show() | ||
| } | ||
| } | ||
| ) |
There was a problem hiding this comment.
searchUser 함수는 잘 써두고 여긴 왜...ㅋㅋㅋ
searchUser 처럼 쓰는게 더 깔끔한 것 같아
| fun addUser(user: User) { | ||
| list.add(user) | ||
| _users.value = list | ||
| } |
There was a problem hiding this comment.
이 메소드는 어디서 쓰는 거야? 만약 외부에서 사용하지 않는다면 감춰주는 것이 좋지 않을까?
| if (response.isSuccessful) { | ||
| Log.d("server check", "통신 성공") | ||
| list.clear() | ||
| for (i in response.body()!!.items.indices) { |
There was a problem hiding this comment.
해당 부분은 코틀린에서 제공하는 함수를 사용해 보는게 어떨까?
There was a problem hiding this comment.
쓰앵님 ... 제가 나름 구글링을 해봣는데 저기서 어떤 부분 ..? 예를 들어 리스트 초기화 ㅇ아니면 서버통신 뭐 이런것 중에 어느걸 코틀린에서 제공하는 함수를 바꿔야 되는건갑슈 ? 찾지를 못하것어 ㅠ
| companion object { | ||
| @JvmStatic | ||
| @BindingAdapter("detailImageUrl") | ||
| fun loadDetailImage(imageView: ImageView, url: String?) { | ||
| Glide.with(imageView.context).load(url).into(imageView) | ||
| } | ||
| } |
There was a problem hiding this comment.
이미지를 불러오는 부분은 UserSearchBindingAdapter 에도 있는 것 같아 결국 static이니까 공통되는 부분은 같이 사용해도 되지 않을까?
There was a problem hiding this comment.
이거 하다가 궁금한거 생겼는데 그거 내일 물어볼게 !!!!!
1. 에러 처리 추가 2. 쓸데없는 코드 삭제 3. 코드 스타일 정리
1. 클릭리스너 부분 수정
Description
일단 브랜치 너무 많이 만들어서 죄송하구요 ....
dahyee가 찐이고 dahyee-> kimdahyee로 풀리케를 날립니다 ...
(꾸벅)
Screenshots or Viedeo
Link to the related issue(s)