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

Add coroutine #10

Open
wants to merge 16 commits into
base: develop/view
Choose a base branch
from
Open

Add coroutine #10

wants to merge 16 commits into from

Conversation

Dan2dani
Copy link
Contributor

No description provided.

Copy link
Member

@youngjinc youngjinc left a comment

Choose a reason for hiding this comment

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

리뷰 완! 고생하셨습니다 :)

@@ -15,11 +16,11 @@ class rvAdapter(context : Context):RecyclerView.Adapter<RecyclerView.ViewHolder>
override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): RecyclerView.ViewHolder {
return when(viewType){
REPO_TYPE ->{
val binding = LayoutGithubRepoBinding.inflate(inflater, parent,false)
val binding = ItemUserInfoBinding.inflate(inflater, parent,false)
Copy link
Member

Choose a reason for hiding this comment

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

네이밍 좋습니다 :)

import timber.log.Timber

class LoginRepository {
suspend fun login(email: String, password: String) : Boolean=
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
suspend fun login(email: String, password: String) : Boolean=
suspend fun login(email: String, password: String) : Boolean =

Goooood! 코드 정렬만 시켜주세용!


class LoginRepository {
suspend fun login(email: String, password: String) : Boolean=
kotlin.runCatching {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
kotlin.runCatching {
runCatching {

}
})
fun login() {
viewModelScope.launch {
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 6 to 14
class LoginViewModelFactory(private val loginRepository: LoginRepository):ViewModelProvider.Factory {
override fun <T : ViewModel> create(modelClass: Class<T>): T {
return if (modelClass.isAssignableFrom(LoginViewModel::class.java)) {
LoginViewModel(loginRepository) as T
} else {
throw IllegalArgumentException()
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

나중에 hilt를 쓰게된다면 Factory는 필요없게됩니다! hilt도 공부해보시면 좋을 것 같아요~

@Body requestLoginDTO: RequestLoginDTO
): Call<ResponseLoginDTO>
): Response<ResponseLoginDTO>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
): Response<ResponseLoginDTO>
): ResponseLoginDTO

_userInfo.value = items
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

함수화를 해봅시다!

private var items : List<ResponseUserDTO.Data> = emptyList()

init{
userService.user().enqueue(object : Callback<ResponseUserDTO> {
Copy link
Member

Choose a reason for hiding this comment

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

함수명은 동사형태로 작성하는 것이 좋습니다! 어떤 로직을 수행하는 함수인지 알 수 있도록 구체적인 네이밍으로 수정해봅시다!!
예를들어 유저 목록을 불러오는 함수라면 fetchUser() 라는 함수명 등이 있겠어요~

login 부분만 presentation, domain, data로 폴더링해서 나눠봤어요! 아직 hilt 공부는 못했어요 ㅜㅜ
Copy link
Member

@YuBeen-Park YuBeen-Park left a comment

Choose a reason for hiding this comment

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

과제 수고했어용~~~

@@ -15,7 +15,7 @@
android:theme="@style/Theme.INSOPTAndroidPractice"
tools:targetApi="31">
<activity
android:name=".SignInActivity"
android:name=".presentation.SignInActivity"
Copy link
Member

Choose a reason for hiding this comment

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

패키지 나눈거 좋아용!!

}
viewModel.fatchUser()

viewModel.userInfo.observe(viewLifecycleOwner, Observer {
Copy link
Member

Choose a reason for hiding this comment

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

위에 adapter 설정해주는 부분은 initLayout()으로, 이 부분은 addObservers함수로 로직 분리해주면 좋을 거같아용!

class LoginDataSource(private val service: ServicePool) {
suspend fun login(requestLoginDTO: RequestLoginDTO): ResponseLoginDTO =
service.loginService.login(requestLoginDTO)
}
Copy link
Member

Choose a reason for hiding this comment

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

저도 DataSource클래스 만들어서 다시 코드 정리해볼게요!!

@Body requestSignupDTO: RequestSignupDTO
): Call<ResponseSignupDTO>
): Response<ResponseSignupDTO>
Copy link
Member

Choose a reason for hiding this comment

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

오호 Response로 감싸서 사용할 수도 있군요

Copy link
Member

@youngjinc youngjinc Dec 29, 2022

Choose a reason for hiding this comment

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

레트로핏이 버전업되면서 Response로 감싸지 않아도 사용가능해진 것으로 압니다!

@@ -1,12 +1,13 @@
package org.sopt.sample.remote

import retrofit2.Call
Copy link
Member

Choose a reason for hiding this comment

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

ctrl+alt+O 단축키로 현재 사용하고 있지 않은 import 제거할 수 있어용!!

@@ -1,9 +1,10 @@
package org.sopt.sample.remote

import retrofit2.Call
Copy link
Member

Choose a reason for hiding this comment

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

요기도 import 제거해주세용

val pattern = Pattern.compile(pwPattern)
val matcher = pattern.matcher(pw)
return matcher.matches()
companion object {
Copy link
Member

Choose a reason for hiding this comment

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

companion object 사용 좋아요~~

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

Successfully merging this pull request may close these issues.

3 participants