-
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/#8] 4주차 필수 과제 구현 #9
base: develop
Are you sure you want to change the base?
Conversation
회원가입 관련 userService 추가 및 retrofit okhttp provide 구현
WavveActionTextField에서도 키보드 옵션 인자 추가
hobby와 SnackBarText 추가
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.
구조 변경이랑 멀티모듈까지 적용하다니 대단합니다 ㅎㅎ 고생하셨습니다!
import org.json.JSONObject | ||
import org.json.JSONException | ||
|
||
inline fun <T> execute(block: () -> T): Result<T> = runCatching { |
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.
야무지네요
import kotlinx.serialization.Serializable | ||
import org.sopt.and.domain.model.MyHobbyResponse | ||
|
||
@Serializable |
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.
요기는 왜 internal을 안쓰나요?
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.
너무 깔끔한 코드였습니다!!
dependencies { | ||
implementation(project(":data")) | ||
implementation(project(":domain")) | ||
implementation(project(":presentation")) |
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.
뭐야 이걸 못봤네ㄷㄷ 엄청나다!
override fun onCreate() { | ||
super.onCreate() | ||
PreferenceUtil.init(this) | ||
} |
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.
궁금한 점!! 지현이 코드에도 이렇게 구현되어 있는데, 초기화 작업이 따로 없는 것 같은데 onCreate()
함수를 작성한 이유가 따로 있나요
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.
아뇨...! 원래 PreferenceUtil.init 때문에 썼던 거였습니다...!!
suspend fun signIn(request: SignInRequestDto): Result<SignInResponse> = execute { | ||
authService.signIn(request).result.toDomainModel() | ||
} |
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.
execute 확장함수로 만들어서 핸들링 하는거 좋은데요?
when(networkState){ | ||
is NetworkState.Loading -> { | ||
// TODO 로딩 추가 | ||
} | ||
is NetworkState.Error -> { | ||
viewModel.handleSignUpIntentError(networkState.title + networkState.msg) | ||
} | ||
is NetworkState.Success -> { | ||
viewModel.handleSignUpIntentSuccess() | ||
} |
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.
Qualifier가 필요없어 보이는데 사용하신 이유가 있을까요??
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.
엇 필요하다고 생각했는데 생각해보니 필요없을 것 같네요! 수정하겠습니다!
val username: String | ||
) | ||
|
||
internal fun User.toRequestBody(): SignUpRequestDto { |
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.
User의 확장 함수가 dto 파일에 들어있으면 파일의 책임이 너무 모호해진다고 생각합니다!
저는 mapper
라는 패키지로 관리하는 것을 선호합니다!
fun toDomainModel(): MyHobbyResponse { | ||
return MyHobbyResponse( | ||
hobby = hobby | ||
) | ||
} |
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.
dto -> model 변환도 매퍼 패키지에서 관리하면 책임 분리가 명확해진다고 생각합니다!
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.
presentation에서 네트워크 통신에서 발생하는 에러를 처리해주고 관리해주는 클래스라고 생각하시면 될 것 같아요!
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.
고생하셨습니다 예외 처리를 완전 꼼꼼하게 잘 해주셨네요 합세도 아자자-!
import org.json.JSONObject | ||
import org.json.JSONException | ||
|
||
inline fun <T> execute(block: () -> T): Result<T> = runCatching { |
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.
야무지네요
|
||
var id: String | ||
get() = userSharedPreference.getString(ID, "").toString() | ||
set(value) = userSharedPreference.edit().putString(ID, value).apply() |
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.
edit과 apply의 차이는 뭘까요?
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.
edit로 데이터를 변경할 준비를 하고 그 뒤에 putString으로 변경사항을 정의해주고, apply로 이 정의된 변경사항을 실제로 적용시켜주는 개념으로 이해하고 있습니다...!
@InstallIn(SingletonComponent::class) | ||
internal interface RepositoryModule { | ||
|
||
@Binds |
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.
에러 처리 좋네용
@@ -0,0 +1,5 @@ | |||
package org.sopt.and.domain.model | |||
|
|||
data class MyHobbyResponse( |
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에 있는 data class에 Response라는 네이밍이 사용되면서 너무 data 레이어스러운 네이밍이 된 것 같습니다! (개인적으로 ㅋㅋ)
한 번 어떻게 네이밍하는 게 좋을지 고민해보면 좋을 것 같습니다.
val updatedException = when(exception){ | ||
is HttpCodeError -> { | ||
when(exception.title) { | ||
"400" -> HttpCodeError( |
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.
상수화를 시켜도 좋을 것 같네요!
init { | ||
getMyHobby() | ||
} |
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.
init 블록은 어떤 역할을 할까요? 그리고 어떠한 로직들을 담는 게 좋을까요?
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.
init 블록에서 무거운 작업(서버 통신, 데이터베이스 쿼리 등과 같은,,)을 진행하는 것이 좋지 않고, 가벼운 초기화 로직만 들어가는 게 좋습니다.
init 블록에서 가벼운 초기화 로직만 작성해야 하는 이유는 여러 가지가 있는데요,, 일단 첫번째로 init 블럭은 사실상 생성자의 일부로 볼 수 있어서 비동기로 처리되어야 하는 로직을 작성하는 게 좋지 않습니다. 그리고 init 블럭에서 함수를 호출하면 그 시점이 앱의 다른 생명주기 이벤트와 충돌할 수 있어 문제가 됩니다. (예를 들어,, 뷰모델이 액티비티보다 먼저 초기화 되었는데 그 부분에서 UI를 업데이트 시키는 경우 -> 이로 인한 NPE) 또, 뷰모델에서 의존성 주입을 하는 경우에는 의존성이 주입되기 전에 함수가 호출될 수 있어요!
추가로 저도 리뷰 달다가 본 글인데 (종명 오빠 땡큐 ㅎ.ㅎ) 이 글 읽어보셔도 좋을 것 같네요 ~
@Inject | ||
lateinit var networkDelegate: NetworkDelegate |
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.
생성자에 넣지 않고 따로 inject 시켜준 이유가 있나요?
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.
엇 아뇨 따로 이유는 없고 사실 생성자로 다 수정하려고 했는데 수정을 다 못한 것 같네요...!
Related issue 🛠
Work Description ✏️
Screenshot 📸
회원가입
default.mp4
default.mp4
로그인
default.mp4
default.mp4
마이페이지
Uncompleted Tasks 😅
To Reviewers 📢
미리 사과의 말씀을 드리면... 바뀐 것도 많고 보기도 힘드실 수도 있습니다...^^
감사드려요...
혼자서 처음부터 끝까지 hilt랑 멀티모듈 적용하고, 클린아키텍쳐 적용하는게 처음이라 잘한 건지 모르겠네요...! 잘부탁드려요~~
상태코드와 에러 처리 하는 부분에서 각각 api 마다 상태코드에서 에러코드의 케이스가 다르더라구요...!
예를 들어 상태코드가 400이고 에러 코드가 01이라면 회원가입에서는 "username 혹은 password 혹은 hobby가 8자를 넘기는 경우"이고, 유저로그인에서는 "request body가 유효하지 않은 경우"더라고요...!
(참고로 회원가입에서는 400에 00이 reqeust body가 유효하지 못한 경우입니다!)
그래서 이런 부분들을 처리하느라 조금 어려웠습니다... 같으면 좋았읉텐데 하하
제가 서버를 잘 몰라서 그런걸 수도 있는데 아예 같은걸로 통일을 하면 안되는건가요...? 다들 아시는게 있다면 말해주시와요,...!
이번주 코드리뷰도 잘 부탁드립니다!