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

Feat/week xml07 #19

Open
wants to merge 5 commits into
base: develop-xml
Choose a base branch
from
Open

Feat/week xml07 #19

wants to merge 5 commits into from

Conversation

OliviaYJH
Copy link
Member

@OliviaYJH OliviaYJH commented Jun 7, 2024

📌𝘐𝘴𝘴𝘶𝘦𝘴

📎𝘞𝘰𝘳𝘬 𝘋𝘦𝘴𝘤𝘳𝘪𝘱𝘵𝘪𝘰𝘯

  • Repository를 활용해 UI와 Data로직의 분리하기

📷𝘚𝘤𝘳𝘦𝘦𝘯𝘴𝘩𝘰𝘵

Uploading xml07.mp4…

@OliviaYJH OliviaYJH added the 📕 필수 과제 필수 과제 label Jun 7, 2024
@OliviaYJH OliviaYJH requested a review from a team June 7, 2024 14:07
@OliviaYJH OliviaYJH self-assigned this Jun 7, 2024
@OliviaYJH OliviaYJH requested review from cacaocoffee, kez-lab, chanubc and hyoeunjoo and removed request for a team June 7, 2024 14:07
Copy link

@librarywon librarywon left a comment

Choose a reason for hiding this comment

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

안녕하세요 효은님 마지막 주차 코드리뷰를 맡게된 명예 OB 서재원입니다!
코드리뷰하면서 정말 34기 YB분들은 대단하다는 생각 밖에 들지 않네요 Hilt, repository, flow까지..
공부를 하시는 중인것 같고 반영부분이 많지 않아서 저도 리뷰를 많이 달지는 않았지만 layer 분리를 하는 이유와 이점을 잘 공부해보시면 앱잼에도 도움이 많이 될 것 같습니다
솝트 세미나 완주하시느라 고생하셨습니다

Comment on lines +19 to 23
@Module
@InstallIn(SingletonComponent::class)
object ApiFactory {
private const val BASE_URL: String = BuildConfig.AUTH_BASE_URL

Choose a reason for hiding this comment

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

ApiFactory와 모듈이 utils 폴더 아래에 있는 것은 어색하다고 느껴지네요!
엄연히 data layer의 친구들을 다루니 data 아래로 옮기는 것도 좋을 것 같습니다.

Comment on lines 23 to 24
private val _state = MutableStateFlow<UiState<Unit>>(UiState.LOADING)
val state get() = _state.asStateFlow()

Choose a reason for hiding this comment

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

사소하지만 변수의 타입을 명시적으로 적어주는 것이 좋습니다!

@@ -0,0 +1,12 @@
package com.sopt.now.repository

Choose a reason for hiding this comment

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

repository역시 독립적으로 나와있을 친구가 아닌 것 같군요!
아래 안드로이드 앱 아키텍처 공식 문서를 제공 첨부해드릴테니 레이어분리를 하는 이유와 레이어 분리시 이점을 잘 알아가시면 앱잼에서도 도움이 많이 되실 것 같습니다.

https://developer.android.com/topic/architecture?hl=ko
P.S 클린 아키텍처는 완전 다른 친구입니다.

Copy link

@hyoeunjoo hyoeunjoo 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 +88 to +90
// dagger hilt
implementation "com.google.dagger:hilt-android:2.51"
kapt "com.google.dagger:hilt-compiler:2.51"

Choose a reason for hiding this comment

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

키야.. 믓찌다..

Comment on lines +16 to 30
@HiltViewModel
class SignUpViewModel @Inject constructor(
private val authRepository: AuthRepository
) : ViewModel() {
private val _state = MutableStateFlow<UiState<Unit>>(UiState.LOADING)
val state = _state.asStateFlow()
val state get() = _state.asStateFlow()

fun signUp(data: RequestSignUpDto) {
viewModelScope.launch(Dispatchers.IO) {
_state.value = UiState.LOADING
runCatching {
authService.signUp(data)
authRepository.signupUser(data)
}.onSuccess {
if (it.isSuccessful) _state.value = UiState.SUCCESS(null)
else {

Choose a reason for hiding this comment

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

아 진짜 멋있다..이 언니

Comment on lines +21 to +23
@AndroidEntryPoint
class MyPageFragment : BaseFragment<FragmentMypageBinding>(FragmentMypageBinding::bind, R.layout.fragment_mypage) {
private val myPageViewModel: MyPageViewModel by viewModels()

Choose a reason for hiding this comment

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

뷰모델과 달리 액티비티, 프래그먼트 등과 같은 컴포넌트에는 이 어노테이션을 쓰는 거군요..

Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines 49 to 51
inline fun <reified T> create(): T =
retrofit.create(T::class.java) // create를 통해서 retrofit 구현체 생성
}

Choose a reason for hiding this comment

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

어 inline함수다 코인액에서 본..!! 역시 아는 만큼 보인다..

Copy link
Member

@chanubc chanubc left a comment

Choose a reason for hiding this comment

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

hilt사용 믓집니다!!
아니 왜케 잘해용

@Module
@InstallIn(SingletonComponent::class)
object AuthModule {
@Provides
Copy link
Member

Choose a reason for hiding this comment

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

@Singleton

컴포넌트도 붙여주세요!
싱글톤을 사용해야 해당 객체가 어플리케이션에서 생성될때 하나만 생성되고 모든 곳에서 동일한 인스턴스를 사용하게 됩니다!
이로 인해 리로스를 절약할 수 잇어요

Copy link
Member

Choose a reason for hiding this comment

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

그리고 repository 인터페이서의 경우 provides보단 binds가 적합합니다!
image
https://developer.android.com/training/dependency-injection/hilt-android?hl=ko
https://hungseong.tistory.com/29

Copy link
Member

Choose a reason for hiding this comment

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

현재 뷰모델의 서버 통신 로직들은 ui단의 뷰모델이 아닌 data단의 repostiory impl에서 수행하는게 더 좋을 것 같아요!

Comment on lines +21 to +23
@AndroidEntryPoint
class MyPageFragment : BaseFragment<FragmentMypageBinding>(FragmentMypageBinding::bind, R.layout.fragment_mypage) {
private val myPageViewModel: MyPageViewModel by viewModels()
Copy link
Member

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
📕 필수 과제 필수 과제
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[7차 세미나]: xml 필수과제
4 participants