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

[Week7] Compose 필수 과제 #20

Open
wants to merge 3 commits into
base: develop-compose
Choose a base branch
from

Conversation

arinming
Copy link
Member

@arinming arinming commented Jun 7, 2024

🧩 Issue number

이슈 번호 : #18

✨ Summary

팔로워 리스트 레포지토리 패턴 적용

🔍 PR Type

  • 필수 XML
  • 필수 Compose
  • 심화 XML
  • 심화 Compose
  • 도전 XML
  • 도전 Compose

📷 Screenshot

Screen_recording_20240607_213745.mp4

📔 Other Information

졸작 마감 이슈로 심신미약인 점 양ㅇ해 부탁드려요.........흑흐ㅠㅡ,ㅠㅡ,ㅜㅡ

Copy link
Member

@junseo511 junseo511 left a comment

Choose a reason for hiding this comment

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

xml 코드 야무지게 쓰셨군요~~ 삼성티엠~~

app/build.gradle Show resolved Hide resolved
Copy link

@murjune murjune 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 +12 to +14
suspend fun getUserList(page: Int): Response<ResponseUserDto> {
return withContext(Dispatchers.IO) {
followerService.getUserList(page).execute()
Copy link

Choose a reason for hiding this comment

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

레트로핏 Service 에서 suspend 키워드를 붙이면 내부적으로 코루틴으로 서버 통신을 비동기 처리합니다!

Copy link

Choose a reason for hiding this comment

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

조금 어려운 내용일 수 있는데요..!
안드로이드에서 Dispatcher 의 경우 하드코딩하는 것보다는 생성자 주입 받는 것을 권장하고 있습니다.

https://developer.android.com/kotlin/coroutines/coroutines-best-practices#inject-dispatchers

Copy link

Choose a reason for hiding this comment

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

withContext 와 CoroutineScope 의 차이는 뭘까요?! 🤔

이것도 추후 코루틴에 대해 자세히 학습하실 때 공부하시면 좋을 것 같습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

CoroutineScope는 코루틴을 시작하고 관리하지만 withContext는 다른 Dispatcher에서 실행할 때 사용된다..정도일까용!! 코루틴 관련해서 더 깊게 공부해야 할 것 같아요 감사합니다 ㅜㅜ

Comment on lines +22 to +25
private var _eventNetworkError = MutableLiveData(false)
val eventNetworkError: LiveData<Boolean>
get() = _eventNetworkError

Copy link

Choose a reason for hiding this comment

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

해당 변수는 사용하고 있지 않군요 🥲
이 부분은 왜 StateFlow 가 아닌 LiveData를 사용하셨을까요??

Copy link
Member Author

Choose a reason for hiding this comment

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

StateFlow로 전체적용하려고 하다가 빠뜨린 것 같습니당..! 지우도록 하겄습니다 🫡

@@ -2,5 +2,6 @@ package com.sopt.now.compose.ui.signUp

sealed class SignUpSideEffect {
Copy link

Choose a reason for hiding this comment

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

UiEvent 의 네이밍을 왜 XXXSideEffect 로 명명하셨는지 궁금하군요!

Copy link

Choose a reason for hiding this comment

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

추가적으로 Loading 중에 message 가 정말 필요한지 궁금합니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

message는 따로 필요하지 않을 것 같아요..! 삭제하겠습니다,.!
상태를 피드백해주는 코드라고 생각해서 SideEffect로 명명했는데, Event로 쓰는게 좀더 명확할까요?!..!!

Comment on lines +9 to +10
class FollowerRepository {
private val followerService = ServicePool.followerService
Copy link

Choose a reason for hiding this comment

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

    1. 왜 Repository 라는 중개자 역할의 저장소라는 개념이 나왔을까요??
    1. service 를 생성자 주입이 아닌 필드에 선언하신 이유가 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. 데이터가 변경되어도 비즈니스 로직에 영향을 주지 않기 위함인 것 같아요.! 단일 책임 원칙과도 관련이 있는 것 같습니다.
  2. 의존성 주입을 하다 말았는데, 현재 Hilt 공부하면서 리팩토링중입니당..!

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

Successfully merging this pull request may close these issues.

3 participants