[FIX] 서버 통신에 성공한 경우에만 스크랩 갱신시키도록 변경

[leeeha]

📌 개요

closed #314

✨ 작업 내용

스크랩 post 서버 통신에 성공하면 응답값으로 코스 id 값을 받아서, 어댑터로부터 해당 아이템을 반복문으로 찾아 갱신시키는 방식으로 구현했습니다.

그런데 보관함 화면의 '내가 스크랩한 코스' 리스트에서는 스크랩을 취소하면 아이템이 삭제됩니다. 이걸 구현할 때도 스크랩을 취소할 때마다 방금 클릭한 코스의 id를 반복문으로 찾는 것은 상당히 비효율적인 거 같아서, 방금 클릭한 코스 아이템의 position (index)를 어댑터에 변수로 저장해뒀다가 서버 통신에 성공하면 해당 인덱스의 아이템을 삭제하는 식으로 구현해봤습니다.

이 방식의 사이드 이펙트는 없을지, 또는 더 좋은 의견 있으시다면 코멘트 부탁드립니다!

✨ PR 포인트

내가 스크랩한 코스 리스트 조회하는 코드는 UiStateV2 사용하도록 변경했습니다.

[unam98]

수고 많으셨습니다~! 생각을 해봤는데 아직까진 특별하게 문제가 될 만한 상황이 떠오르지 않네요

[unam98]

이제 보니 id가 3개나 되니까 각각이 뭔지 잘 모르겠고 너무 헷갈리는군여

[leeeha]

저도 처음에는 헷갈렸는데 어느정도 패턴이 있어서 적응이 되더라구요 😅

[unam98]

여기 null 처리 setupMyScrapCourseGetStateObserver()처럼 뷰모델에서 하는 건 어떨까요? 통일이 되면 좋을 것 같습니다.

[leeeha]

응답값이 널일 때 어떻게 처리할 것인지에 대한 논의가 미리 진행되지 않아서, 이런 식으로 그냥 리턴을 해버리는 코드도 상당히 많은 거 같아요..! 이런 코드들도 우남님이 말하신 것처럼 뷰모델에서 널체크 하고 UiState Failure 처리하는 것으로 수정이 필요할 거 같습니다.

[leeeha]

CourseDetailViewModel에서 대부분의 서버통신 코드가 널처리를 따로 하지 않고 그냥 nullable한 응답값을 프래그먼트로 보내는 방식이어서, 코드의 수정사항이 많을 것으로 예상됩니다. 배포 후에 별도 브랜치에서 리팩토링 진행할게요!

[unam98]

내가 스크랩한 코스 쪽 작업해주신 것처럼 여기도 for문 돌릴 필요 없이 index를 저장해놨다가 해당 부분 index에 바로 접근하는 방법은 어떨까요? 더 효율적인 것 같고 방법도 통일돼서 더 좋을 것 같습니다.

[leeeha]

현재 저희 코드에서 상당수의 어댑터가 for문 돌려서 아이템의 id를 찾는 방식으로 구현되어 있는 걸로 알고 있어요! 내가 스크랩한 코스 어댑터만 유일하게 클래스의 멤버변수를 따로 만들어서 사용하는 방식으로 구현했구요. 각각 장단점이 있는 거 같습니다.

for문 돌려서 아이템 찾는 방식

• 장점: 멤버변수를 따로 정의할 필요 x
• 단점: 선형 탐색 시간이 든다.

item position 멤버변수 정의

• 장점: 아이템 클릭 시 position 값을 변수에 저장해뒀기 때문에 탐색 시간 x
• 단점: 멤버변수를 잘못 조작할 여지가 있음.

우남님은 두번째 방식이 더 좋다고 생각하시는지 궁금합니다!

[unam98]

넵! 저도 탐색 시간이 걸린다는 부분을 생각했었어요! data의 개수가 많지 않으면 for문 돌려도 큰 차이 없을 수 있지만 data는 더 늘어날 여지가 있으니 변수에 저장해뒀다가 바로 꺼내 쓰는 게 낫지 않을까 싶었습니다.

[leeeha]

위의 어댑터에서 모두 for문을 이용하여 아이템을 갱신하고 있는데, 다른 팀원분들도 두번째 방식이 더 낫다고 생각하는지 의견 들어보고 수정하면 좋을 거 같습니다 :)

@Donghyeon0915 @sxunea

[leeeha]

동현님께서 회의 시간에 의견 주신 대로, 서버 통신이 로딩 상태일 때는 다른 아이템의 스크랩 버튼을 누르지 못하도록 로딩 프로그레스바 띄우도록 하겠습니다!

(로딩 중인 상태에서 다른 아이템을 클릭하면, 멤버변수 clickedItemPosition 값이 의도치 않게 갱신되는 문제가 발생하므로)

[dongx0915]

고생하셨습니다~!

[dongx0915]

이번 PR과는 무관하지만 이런 부분도 나중에는 인터페이스로 분리되면 좋을 것 같네요
Fragment에서 특정 Activity를 참조하다보니 결합도가 높아보이긴 하네요!

당장 수정할 필요는 없을 것 같고 나중에 다 같이 리팩토링 할 때 반영 되면 좋을 것 같아서 남겨둡니다!
Activity에서 Fragment를 굳이 가지고 있어야하는가도 같이 논의 해보면 좋을 것 같아요

interface StorageScrapListener {
    fun setStorageScrapFragment(fragment: Fragment?)
}

class MainActivity : AppCompatActivity(), StorageScrapListener {
    override fun setStorageScrapFragment(fragment: Fragment?) {
        storageScrapFragment = fragment
    }
    /*...*/
}

override fun onAttach(context: Context) {
    super.onAttach(context)
    if (context is StorageScrapListener) {
        context.setStorageScrapFragment(this)
    }
}

[leeeha]

이 코드는 기존에 계시던 팀원분이 작성한 거라 일단 가만히 두었는데, 메인 액티비티 코드를 보니까 다른 화면에 갔다가 다시 돌아올 때 서버로부터 새 데이터를 조회하기 위해 필요했던 것으로 보입니다.

var discoverFragment: DiscoverFragment? = null
var storageScrapFragment: StorageScrapFragment? = null

fun updateCourseDiscoverScreen() {
    discoverFragment?.refreshDiscoverCourses()
}

fun updateStorageScrapScreen() {
    storageScrapFragment?.getMyScrapCourses()
}

이것보다 분명 더 나은 방식이 있을 거 같다는 생각이 드네요. 저도 더 고민해봐야겠어요. 코멘트 감사합니다!