-
Notifications
You must be signed in to change notification settings - Fork 0
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/#12] 6주차 과제 구현 #13
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코루틴 야무지게 적용하신 거 너무 좋네요 !! 고생하셨어용 ~~
class SignInUseCase @Inject constructor( | ||
private val signInRepository: SignInRepository | ||
) { | ||
suspend operator fun invoke(user: User): Result<SignInResponse> = | ||
signInRepository.signInUser(user).onFailure { | ||
return Result.failure(Throwable(MESSAGE_FAILURE)) | ||
} | ||
|
||
companion object { | ||
private const val MESSAGE_FAILURE = "아이디 또는 비밀번호가 일치하지 않습니다." | ||
|
||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
유즈케이스에서 에러 처리 해주는 거 야무지군요 ㅎㅎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넘넘 고생하셨어용 ~~ 🤗👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM~ 🤙 고생하셨습니다~!!
@@ -1,4 +1,4 @@ | |||
package org.sopt.and.core.model | |||
package org.sopt.and.domain.entity | |||
|
|||
import org.sopt.and.R |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주석 남겨주시는 습관 너무 좋습니다~!
@@ -1,6 +1,6 @@ | |||
package org.sopt.and.domain.repository | |||
|
|||
import org.sopt.and.core.model.HomeRecommendation | |||
import org.sopt.and.domain.entity.HomeRecommendation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(맞다 틀리다가 아닌 개념 확인 차 남기는 질문) model이라는 용어를 사용하실 수도 있었을 것 같은데 entity라는 용어를 사용하신 이유가 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
클린 아키텍처에 대한 공부를 하면서 도메인 엔티티라는 말이 입에 붙어서 패키지 이름을 entity로 지은 것 같아요,,
import org.sopt.and.presentation.signin.state.SignInUiState | ||
import javax.inject.Inject | ||
|
||
@HiltViewModel | ||
class SignInViewModel @Inject constructor( | ||
private val signInRepository: SignInRepository | ||
private val signInUseCase: SignInUseCase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UseCase를 사용하신 의도가 있으실까요?
@@ -57,14 +56,14 @@ class SignUpViewModel @Inject constructor( | |||
fun registerUser() = viewModelScope.launch { | |||
with(_uiState.value) { | |||
if (isButtonEnabled) { | |||
signUpRepository.registerUser(User(id, password, hobby)) | |||
registerUserUseCase.invoke(User(id, password, hobby)) | |||
.onSuccess { response -> | |||
_sideEffect.emit(SignUpSideEffect.Toast(response.message)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_sideEffect.emit 이 부분 함수로 분리해서 더 간결하게 쓸 수도 있을 것 같아용
import org.sopt.and.presentation.signup.state.SignUpUiState | ||
import javax.inject.Inject | ||
|
||
@HiltViewModel | ||
class SignUpViewModel @Inject constructor( | ||
private val signUpRepository: SignUpRepository | ||
private val registerUserUseCase: SignUpUseCase | ||
) : ViewModel() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거는 구냥 남겨보는 질문인데 ViewModel에 context를 전달해서 쓰면 안 좋다는 얘기가 있는데 어떤 이유 때문일까용?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
화면 전환 같은 일이 발생하면 액티비티는 잠깐 종료되었다가 다시 생성되는데 뷰모델은 종료되지 않고 남아있기 때문에 뷰모델은 존재하지 않는 액티비티의 context를 참조하게 되어 에러가 발생할 수 있다고 들었습니다!
@@ -2,5 +2,5 @@ package org.sopt.and.domain.entity | |||
|
|||
data class SignInResponse( | |||
val token: String? = null, | |||
val message: String? = null, | |||
val message: String = "" , |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코드를 자세히 본 건 아니지만 responseDTO의 필드 타입들을 모두 nullable하게 바꾸고 각 dto들을 뷰 단에서 활용할 객체(entity)로 맵핑하는 함수를 만들어서 최종적으로 entity에서 다뤄지는 필드들은 non-nullable하게 만들면 어떨까요?
서버에서 null 값이 날아올 수도 있는데 맵핑이라는 절차를 통해 방어 로직 작성이 가능하고
entity로 참조하는 값들은 null이 아님을 보장할 수 있으니 추후 문제가 생겼을 때 원인 추적이 수월해질 것 같다고 생각해서요.
만약 이렇게 한다면 맵핑 로직을 어디서 관리할 것인가에 대한 고민이 필요할 텐데 별도의 맵퍼 클래스를 하나 만들고 모든 맵핑 함수를 그 안에서 관리하는 게 좋을지, 아니면 responseDTO 파일 안에서 같이 관리하는 게 좋을지 고민해보시면 좋을 것 같습니다.
정답은 없지만 저는 개인적으로 대응되는 맵핑 함수를 바로 찾아볼 수 있어 편리하다고 생각해서 dto 파일 안에 넣어서 관리하는 편입니다. 다양한 의견이 궁금하시다면 23년 드나 스피치 중 빈혈 도메인이란 주제가 있었는데 영상 참고해보시면 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거는 개인적인 생각인데 SignInResponseDTO와 SIgnInResponse가 이름이 유사해서 헷갈릴 수 있을 것 같습니다. 좀 더 직관적으로 구분되는 네이밍이면 더 좋지 않을까 의견 남겨봅니다 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jm991014 @kangyein9892 @serioushyeon 다른 분들도 이 코멘트 같이 봐보시면 좋을 것 같아용
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"서버에서 null 값이 날아올 수 있다"까지 생각을 못했네요.. 감사합니다!!
맵핑 함수의 위치는 팀 간 컨벤션에 따라 다르겠지만.. 저는 한 번에 모아 보는 것이 더 편리한 것 같습니다!! (하지만 앱의 규모가 커지면 또 생각이 달라질 수도 있을 것 같습니다..ㅎㅎ)
@@ -1,4 +1,4 @@ | |||
package org.sopt.and.core.model | |||
package org.sopt.and.domain.entity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 프로젝트에서 domain 레이어의 역할은 무엇인가요?
Related issue 🛠
Work Description ✏️
Screenshot 📸
Uncompleted Tasks 😅
To Reviewers 📢