Skip to content

Comments

[Team-04][Android][Linus_Jay][3주차 금요일 여섯번째 PR]#285

Open
cherrylime69 wants to merge 17 commits intocodesquad-members-2022:team-04from
shim506:main
Open

[Team-04][Android][Linus_Jay][3주차 금요일 여섯번째 PR]#285
cherrylime69 wants to merge 17 commits intocodesquad-members-2022:team-04from
shim506:main

Conversation

@cherrylime69
Copy link

🤷‍♂️ Description

  • 수요일 PR 반영
  • 네트워크 통신 결과 예외 처리 적용
  • Room을 활용한 caching 기능 구현 완료
  • Firebase 를 활용하여 이슈 목록 Paging 처리 구현으로 무한 스크롤 기능 구현 완료

📷 Screenshots

캐싱 구현 영상

2022-07-01.9.07.05.mov

무한 스크롤 구현 영상

2022-07-01.9.08.39.mov

🎉 질문 사항

안녕하세요 레노! 페이징 처리를 위해 firebase 의 limit 과 startAfter 메서드를 사용하고 있습니다. (참고자료)

첫번째 페이징 처리로 10개의 데이터를 먼저 불러오고 맨 마지막으로 스크롤할 때 loadNextPageIssues 가 호출되는 구조입니다.
첫번째로 데이터를 불러오고 10번째의 데이터를 lastVisibleDocument 에 저장하고 loadNextPageIssues 가 호출될 때, startAfter(lastVisibleDocument) 를 사용하여 11번째 데이터부터 다시 10개의 데이터를 불러오도록 구현을 원했습니다.

하지만 어찌된 일인지, lastVisibleDocument 값은 정상적으로 존재하는데, 자꾸 issueData 가 빈 리스트로 값이 반환됩니다. (밑에 에러 코드 첨부)

원인을 도저히 찾지 못해 어쩔 수 없이, page 번호를 fragment -> ViewModel -> repository 까지 불러와 lastVisibleDoc 에서 볼 수 있듯이 10번째 데이터 값(20번째, 30번째, 40번째 등등)을 다시 할당하고 이를 startAfter() 인자로 넘겨주는 방식으로 구현을 완료했습니다. 하지만 이 경우, 페이지 번호가 높을수록 불러오는 데이터양이 많아져 성능이 저하될 수 밖에 없는 단점이 있습니다. (즉, 지금은 그냥 임시방편으로 페이징을 구현하고 있습니다.)

혹시 이에 대해 조언이 있을까요?

class HomeRepositoryImpl @Inject constructor(
    private val fireStore: FirebaseFirestore
) : HomeRepository {

    private var lastVisibleDocument: DocumentSnapshot? = null
    private val collectionData =
        fireStore.collection(FIREBASE_COLLECTION_ISSUE_PATH).orderBy("title")

    override suspend fun loadNextPageIssues(currentPage: Int): List<IssueList> {
        val list = mutableListOf<IssueList>()
        val lastVisibleDoc = fireStore.collection(FIREBASE_COLLECTION_ISSUE_PATH).orderBy("title")
            .limit(currentPage * PAGE_NUMBER).get().await().documents.last()
        val issueData = collectionData
            .startAfter(lastVisibleDocument)
//            .startAfter(lastVisibleDoc)
            .limit(PAGE_NUMBER).get().await()

        println("해보자 $currentPage")

        issueData.documents.forEach {
            val issueObj = it.toObject(IssueDto::class.java)
            issueObj?.id = it.id
            issueObj?.let { issueDto ->
                issueDto.toIssue()?.let { issue ->
                    list.add(issue)
                }
            }
        }

        // 데이터를 추가하는 코드
        // fireStore.collection(FIREBASE_COLLECTION_ISSUE_PATH).document().set(it1)

        lastVisibleDocument = issueData.last()

        println("${issueData.last()["id"]}")
        println(lastVisibleDocument)

        return list
    }

    override suspend fun loadFirstPageIssues(): NetworkResult<List<IssueList>> {
        try {
            val list = mutableListOf<IssueList>()
            val collectionData =
                fireStore.collection(FIREBASE_COLLECTION_ISSUE_PATH).limit(PAGE_NUMBER).get().await()

            collectionData.documents.forEach {
                val issueObj = it.toObject(IssueDto::class.java)
                issueObj?.id = it.id
                issueObj?.let { it1 ->
                    it1.toIssue()?.let { it2 -> list.add(it2) }
                    // 데이터를 추가하는 코드
                    // fireStore.collection(FIREBASE_COLLECTION_ISSUE_PATH).document().set(it1)
                }
            }

            return if (list.isEmpty()) {
                NetworkResult.Error(EMPTY)
            } else NetworkResult.Success(list)

        } catch (e: Exception) {
            return NetworkResult.Exception(e)
        }
    }

image

🎃 기타

  • 레노, 마지막으로 피드백을 받게 되어 영광입니다! 그동안 피드백 해주셔서 진심으로 감사하여, 좋지 않은 코드를 보여드려 죄송합니다. 항상 건강하시기 바라며, 혹시나 나중에 궁금한 점이 있으면 DM 하겠습니다. 감사합니다 :D

cherrylime69 and others added 17 commits June 29, 2022 19:42
- 필터된 이슈 목록 리스트가 빈값이 아닐 때만 이슈 목록이 갱신되도록 수정
1. 네트워크 O -> 리모트에서 로컬에 캐시해줌, 로컬에서 내려주기
2. 네트워크 X -> 로컬에서 내려주기
- sealed class 를 생성하여 실제 issue 와 progress bar 구분
- ProgressBar ViewType 타입 할당
- Is 연산자를 활용하여 sealed class 구분
- firebase의 limit 과 startAfter 메서드를 활용하여 페이징 처리
- 코루틴 수정 : Coroutine() -> suspend 함수인 coroutine() 으로 수정하고 .join() 을 삭제
  - suspend 함수가 아닌 Coroutine 에서 선언된 launch 실행문들은 무시되고 함수를 리턴 함 , 최초에는 join 이 있어서 문제가 없었음
- 컨벤션에 맞게 변수이름 수정
- 맨 마지막 스크롤까지 가면 page 숫자 증가
- page 숫자를 가지고 페이징 처리 구현
caching 적용 및 네트워크상태 관리
페이징 처리를 하여 이슈 목록 화면 표시 구현
[Team-04][Linus_Jay] Upstream에 마지막 PR 요청을 위한 PR
@shim506 shim506 requested a review from renovatio0424 July 1, 2022 12:18
@shim506 shim506 added the review-android Further information is requested label Jul 1, 2022
@renovatio0424
Copy link

질문 답변

안녕하세요 레노! 페이징 처리를 위해 firebase 의 limit 과 startAfter 메서드를 사용하고 있습니다. (참고자료)
첫번째 페이징 처리로 10개의 데이터를 먼저 불러오고 맨 마지막으로 스크롤할 때 loadNextPageIssues 가 호출되는 구조입니다.
첫번째로 데이터를 불러오고 10번째의 데이터를 lastVisibleDocument 에 저장하고 loadNextPageIssues 가 호출될 때, startAfter(lastVisibleDocument) 를 사용하여 11번째 데이터부터 다시 10개의 데이터를 불러오도록 구현을 원했습니다.
하지만 어찌된 일인지, lastVisibleDocument 값은 정상적으로 존재하는데, 자꾸 issueData 가 빈 리스트로 값이 반환됩니다. (밑에 에러 코드 첨부)
원인을 도저히 찾지 못해 어쩔 수 없이, page 번호를 fragment -> ViewModel -> repository 까지 불러와 lastVisibleDoc 에서 볼 수 있듯이 10번째 데이터 값(20번째, 30번째, 40번째 등등)을 다시 할당하고 이를 startAfter() 인자로 넘겨주는 방식으로 구현을 완료했습니다. 하지만 이 경우, 페이지 번호가 높을수록 불러오는 데이터양이 많아져 성능이 저하될 수 밖에 없는 단점이 있습니다. (즉, 지금은 그냥 임시방편으로 페이징을 구현하고 있습니다.)
혹시 이에 대해 조언이 있을까요?

안녕하세요 ~
아래와 같이 로그를 찍어보면 마지막이어야할 아이템이 첫번째 아이템으로 나오네요
sort 를 어떻게 하셨는지 한번 확인해보세요~

        // line: 70
        // 데이터를 추가하는 코드
        // fireStore.collection(FIREBASE_COLLECTION_ISSUE_PATH).document().set(it1)

        lastVisibleDocument = issueData.documents.last()

        Log.d("reno", "[loadNextPageIssues] last visible document id: ${lastVisibleDocument?.id}")
        Log.d("reno", "[loadNextPageIssues] last visible document id: ${(list.last() as Issue).title}")
        Log.d("reno", "[loadNextPageIssues] last visible document id: ${(list.last() as Issue).description}")
        
------------------------------------------------  Log ------------------------------------------------ 
//첫번째 loadNextPageIssues 함수가 호출되었을 때 결과
9799-9799/com.example.issu_tracker D/reno: [loadNextPageIssues] last visible document id: 1PM0sqRmEzgwngIKNdV4
9799-9799/com.example.issu_tracker D/reno: [loadNextPageIssues] last visible document id: 제이 야근 그만
9799-9799/com.example.issu_tracker D/reno: [loadNextPageIssues] last visible document id: 테스트입니다

Comment on lines +19 to +39
sealed class IssueList : Serializable {

object IssueProgressBar : IssueList()

@Entity
data class Issue(
@ColumnInfo(name = "issue_id")
var id: String,
val comments: List<Comment>,
val description: String,
val label: List<Label>,
val mileStone: String,
val state: Boolean,
val title: String,
@Embedded
val user: User
) : IssueList(), Serializable {
@PrimaryKey(autoGenerate = true)
var idx: Int = 0
}
}

Choose a reason for hiding this comment

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

sealed class 로 구현하셨던 이유가 무엇일까요?

Comment on lines +7 to +8
class Error<T : Any>(val code: Int = EMPTY) : NetworkResult<T>()
class Exception<T : Any>(val e: Throwable) : NetworkResult<T>()

Choose a reason for hiding this comment

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

ErrorException 을 구분하셨던 이유가 있나요?? (궁금해서요 ㅎㅎ)

Comment on lines +29 to +36
friendData.forEach {
val user =
User(
it["uid"] ?: "", it["name"] ?: "",
it["userPhoto"] ?: ""
)
friendList.add(user)
}

Choose a reason for hiding this comment

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

map 연산자를 사용하면 좀더 깔끔하겠네요~ ㅎㅎ

Comment on lines +58 to +67
issueData.documents.forEach {
val issueObj = it.toObject(IssueDto::class.java)
issueObj?.id = it.id
issueObj?.let { issueDto ->
issueDto.toIssue()?.let { issue ->
list.add(issue)
}
}
}

Choose a reason for hiding this comment

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

여기도 map 연산자와 toList 연산자를 잘 사용한다면 가독성있는 코드를 만들 수 있겠네요~

Comment on lines +10 to +15
@Database(entities = [User::class , IssueList.Issue::class ], version = 2,exportSchema = false)
@TypeConverters(Converters::class)
abstract class IssueTrackerDatabase : RoomDatabase() {
abstract fun userDao(): UserDao
abstract fun issueDao(): IssueDao
} No newline at end of file

Choose a reason for hiding this comment

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

Use, Issue 정보를 Disk 에 저장하는 이유가 있나용?
Memory 에 저장해도 되지 않았나 싶네요 ㅎㅎ

Comment on lines +42 to +48
if (issueListResponse is NetworkResult.Success) {
val issueList = issueListResponse.data.toMutableList()
issueList.removeLast()
issueList.addAll(repository.loadNextPageIssues(currentPage))
issueList.add(IssueList.IssueProgressBar)
_issueListStateFlow.value = NetworkResult.Success(issueList)
}

Choose a reason for hiding this comment

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

NetworkResult.Success 를 제외한 나머지 경우들은 처리 안하는 이유가 있나요?

Comment on lines +72 to +94
.filter {
if (condition.state.isNotEmpty() && it is IssueList.Issue) {
it.state
} else {
true
}
}.filter {
if (condition.writer.isNotEmpty() && it is IssueList.Issue) {
it.user.name == condition.writer
} else {
true
}
}
.filter {
if (it is IssueList.Issue) {
it.label.any { label ->
if (condition.label.isNotEmpty()) {
label.content == condition.label
} else {
true
}
}
}

Choose a reason for hiding this comment

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

하나의 filter 연산자에서 처리할 수 있지 않나용?
나눠서 처리하는 이유가 있나요??

@renovatio0424
Copy link

레노, 마지막으로 피드백을 받게 되어 영광입니다! 그동안 피드백 해주셔서 진심으로 감사하여, 좋지 않은 코드를 보여드려 죄송합니다. 항상 건강하시기 바라며, 혹시나 나중에 궁금한 점이 있으면 DM 하겠습니다. 감사합니다 :D

고생 많았어요~
누구나 실수를 하기 마련이에요~ (저도 그렇고 저보다 경력 많으신 분들조차도) 실수를 부끄러워하지 마시고 지금처럼 실수를 계속해서 개선하려는 모습을 보여주신다면 사랑받는 개발자가 될거에요 ㅎㅎ
화이팅~

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

Labels

review-android Further information is requested

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants