-
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
[PC-363] 로그인 인증 리팩토링 #34
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.
진혁님 고생하셨습니다~
이전에 비해서는 코드가 위치할 모듈에 위치하고 개선이 많이 된 것 같습니다.
테스트 코드도 처음 짜보셨을텐데 고생하셨어요....!
아직 개선할 점이 많지만,
너무 많은 코멘트가 달리면 협업에 방어적인 태도로 변할 수가 있어서 이정도만 코멘트 달고 또 추가적으로 코멘트 달겠습니다ㅎㅎㅎㅎㅎㅎ
우선 코멘트 단 것 부터 확인해주세요~
core/domain/src/main/java/com/puzzle/domain/model/auth/Timer.kt
Outdated
Show resolved
Hide resolved
core/domain/src/main/java/com/puzzle/domain/model/auth/Timer.kt
Outdated
Show resolved
Hide resolved
core/domain/src/main/java/com/puzzle/domain/model/auth/Timer.kt
Outdated
Show resolved
Hide resolved
core/domain/src/main/java/com/puzzle/domain/model/auth/Timer.kt
Outdated
Show resolved
Hide resolved
core/domain/src/main/java/com/puzzle/domain/usecase/RequestAuthCodeUseCase.kt
Outdated
Show resolved
Hide resolved
core/domain/src/test/kotlin/com/puzzle/domain/RequestAuthCodeUseCaseTest.kt
Outdated
Show resolved
Hide resolved
feature/auth/src/main/java/com/puzzle/auth/graph/verification/contract/VerificationState.kt
Outdated
Show resolved
Hide resolved
feature/auth/src/main/java/com/puzzle/auth/graph/verification/contract/VerificationState.kt
Outdated
Show resolved
Hide resolved
core/domain/src/test/kotlin/com/puzzle/domain/VerifyAuthCodeUseCaseTest.kt
Outdated
Show resolved
Hide resolved
허허 감사합니다...!!!
이 부분은 전혀 고려하지 못했던 부분이었던 거 같아요... 곱씹어 생각해보니 지금까지 제 코멘트가 좀 불필요하게 많지 않았나 라는 생각이 조금 드네요... 뭔가 나와 다른 코드를 보면 내가 모르는 부분인가 싶어서 코멘트를 추가하다 보니 좀 태규님을 귀찮게 한 부분이 있었나 라는 생각이 들어 조금 죄송하네요.... 근데 진짜 대부분 순수한 궁금증에서 한거라 제 코멘트는 절대 지적의 의미를 담지 않았으니 오해 없으면 좋겠어요 😭 (그래서 사실 저는 제 코멘트에 따라서 코드 수정을 하시지 않으셔도 된다고 생각합니다!!!) |
앗 아닙니다 !! 저는 코멘트 많이 달아주시면 관심이 많이 주시는 것 같아서 좋아합니다 !! 물어보는 코멘트보다 피드백 쪽의 코멘트는 한번에 많이 달면 PR을 열 때 거부감이 생길 수도 있어서 저 스스로 조심하자는 의미에서 한 거였습니다 ㅎㅎㅎ 저는 코멘트 많이 많이 달아주세요 !!!!! 저는 하나도 귀찮지 않아요...!
이건 절대 아닙니다! 저도 많이 배우고 있습니다 !! 빠야!!!! |
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.
진혁님 고생하셨습니다...!
진혁님 제가 추가적으로 작업해봤는데,
변경된 사항이랑 코멘트 단 부분 체크 해주세요!!
private val dispatcher: CoroutineDispatcher = Dispatchers.Default | ||
) { | ||
fun startTimer(): Flow<Int> = flow { | ||
var remainingTime = durationInSec | ||
while (remainingTime > 0) { | ||
emit(remainingTime) | ||
delay(1000L) | ||
remainingTime-- | ||
} | ||
emit(0) // 타이머 만료 시 0을 방출 | ||
}.flowOn(dispatcher) |
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.
p3)
dispatcher를 빼고 외부에서 scope를 열어도 될 것 같아요!
0을 종료된다는 flag로 사용하시는 것 너무 좋습니다!
0 매직넘버를 상수화 시키면 더 좋을 것 같아요!!
// 정말 휴대폰 번호가 유효하지 않았을 경우 | ||
setState { copy(isValidPhoneNumber = false) } | ||
|
||
// Todo 네트워크 통신 오류 | ||
errorHelper.sendError(it) | ||
} |
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.
해당 API에 대해 실패 명세가 나올 경우 분기를 줄 수 있습니다.
authRepository.verifyAuthCode(code).onSuccess { | ||
// 인증에 성공했을 경우, | ||
timerJob?.cancel() | ||
|
||
setState { | ||
copy( | ||
remainingTimeInSec = 0, | ||
authCodeStatus = VerificationState.AuthCodeStatus.VERIFIED, | ||
) | ||
} | ||
|
||
navigationHelper.navigate( | ||
NavigationEvent.NavigateTo( | ||
route = AuthGraphDest.SignUpRoute, | ||
popUpTo = AuthGraph, | ||
) | ||
) | ||
|
||
// 인증에 실패했을 경우, | ||
//setState { copy(authCodeStatus = VerificationState.AuthCodeStatus.INVALID) } | ||
}.onFailure { errorHelper.sendError(it) } | ||
} |
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.
해당 API에 대해 실패 명세가 나올 경우 분기를 줄 수 있습니다.
verticalAlignment = Alignment.CenterVertically, | ||
modifier = Modifier | ||
.fillMaxWidth() | ||
.padding(vertical = 8.dp), | ||
) { | ||
BasicTextField( |
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.
p3)
TextField 부분 컴포넌트화 되어있는 것 같은데 이거 컴포넌트로 빼주시면 감사하겠습니다~
PC-363
1. ⭐️ 변경된 내용
2. 🖼️ 스크린샷(선택)
3. 💡 알게된 부분
4. 📌 이 부분은 꼭 봐주세요!