-
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-259] Network 기초 세팅 #21
Conversation
da46036
to
266ec82
Compare
2f1eaaa
to
39a1189
Compare
39a1189
to
0ec4539
Compare
6b03f69
to
01d0d60
Compare
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.
태규님! 코멘트가 늦어 죄송합니다... 😭
enum class OAuthProvider(val apiValue: String) { | ||
GOOGLE("google"), KAKAO("kakao") | ||
} |
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 : OAuthProvider라고 하면 뭔가 더 다양한 기능이 있을 것 같은 느낌이 드는데, 혹시 OAuth나 이전에 태규님이 enum class에 ~~type 이라고 정의했던 것처럼 OAuthType은 어떠신가요? 그리고 apiValue도 뭔가 Value가 너무 큰 의미인 것 같은 느낌이 조금 드는데 혹시 apiName는 어떠신가요?(뭔가 enum의 name을 피할려고 Value를 쓰신거 같기도 하고...) 🤔
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.
OAuthProvider라고 하면 뭔가 더 다양한 기능이 있을 것 같은 느낌이 드는데, 혹시 OAuth나 이전에 태규님이 enum class에 ~~type 이라고 정의했던 것처럼 OAuthType은 어떠신가요?
서버 명세서에 ProviderName
라는 Key 값을 사용하셔서 OAuthProvider라고 이름을 지었는데,
여기서 말하는 Provider는 OAuth기능을 제공해주는 제공자의 뜻인 것 같아요.
OAuthType은 OAuth의 타입(?!) 을 뜻하는 것 같아서 (종류가 있는 건지는 잘 모르겠지만..)
OAuthProvider가 더 적합하다고 생각하는데 어떻게 생각하시나요?! 더 착착 감기는 이름이 있을까요 ㅠㅠㅠㅠ?
그리고 apiValue도 뭔가 Value가 너무 큰 의미인 것 같은 느낌이 조금 드는데 혹시 apiName는 어떠신가요?(뭔가 enum의 name을 피할려고 Value를 쓰신거 같기도 하고...) 🤔
api명세의 Key : value 쌍에서 value로 들어간다는 뜻으로 apiValue
라고 작명을 했었어요!
apiName은 어떤 뜻을 내포하고 있나용 ?!?!?
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.
제 느낌?!으로는 저희가 enum class를 상수를 표현할 때 주로 사용하니깐 뭔가 상수 정보 이외의 다른 기능은 크게 제공하지 않는 느낌이어서 약간 data class 처럼 모델 명같이 딱딱한 클래스명이 저는 조금 더 익숙한 거 같아요. 뭔가 Provider는 메서드가 많을 거 같은 느낌?! 인거 같아요. 뭐 근데 이건 개발자마다 다 다른 거니깐 크게 중요한 부분은 아닌거 같긴 해요! 요건 api 문서 보고 하신만큼 OAuthProvider로 표현해도 괜찮을 것 같아요!
apiName 은 저희가 쓰는 kakao api, google api의 이름이다라는 의미인데, 명세서를 보니깐 providerName을 프로퍼티로 해도 좋지 않을가라는 생각이 살짝 드는데 이건 어떠신가유?!
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.
apiName 은 저희가 쓰는 kakao api, google api의 이름이다라는 의미인데, 명세서를 보니깐 providerName을 프로퍼티로 해도 좋지 않을가라는 생각이 살짝 드는데 이건 어떠신가유?!
이 경우 Domain Model이 Data의 DTO에 너무 세부적인 사항에 의존하고 있다는 생각이 들어요!
어떻게 생각하시나요?!?!?!
ex) providerName에서 prover로 API 명세가 바뀌면 클라이언트 내부적으로도 바뀌어야 함!
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 _errorEvent = MutableSharedFlow<Throwable>(extraBufferCapacity = 1) | ||
val errorEvent = _errorEvent.asSharedFlow() |
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.
혹시 errorEvent는 eventflow와 다르게 sharedFlow로 하신 이유가 있나요?
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.
혹시 errorEvent는 eventflow와 다르게 sharedFlow로 하신 이유가 있나요?
앗 이 친구도 어차피 소모하는 곳이 MainViewModel밖에 없어서 Channel로 바꿔도 됩니다!
일관성을 위해 바꿔놓을까용?!
@Serializable | ||
data class LoginOauthResponse( | ||
val status: String?, | ||
val message: String?, | ||
val data: LoginOauthResponseData?, | ||
) | ||
|
||
@Serializable | ||
data class LoginOauthResponseData( | ||
val smsVerified: Boolean?, | ||
val registerCompleted: Boolean?, | ||
val accessToken: String?, | ||
val refreshToken: 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.
- p4 : LoginOauthResponse 라는 부분이 중복되는데 혹시 중첩 클래스로 쓰는건 어떻게 생각하시나요?
@Serializable
data class LoginOauthResponse(
val status: String?,
val message: String?,
val data: Data?,
) {
@Serializable
data class Data(
val smsVerified: Boolean?,
val registerCompleted: Boolean?,
val accessToken: String?,
val refreshToken: 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.
- p4 : LoginOauthResponse 라는 부분이 중복되는데 혹시 중첩 클래스로 쓰는건 어떻게 생각하시나요?
@Serializable data class LoginOauthResponse( val status: String?, val message: String?, val data: Data?, ) { @Serializable data class Data( val smsVerified: Boolean?, val registerCompleted: Boolean?, val accessToken: String?, val refreshToken: String?, ) }
좋은데용?!
저희 Response가 항상 status, message, data 포맷으로 떨어져서,
각 Response마다 다른 data를 어떻게 네이밍해야하나 고민하고있었는데..
훨씬 좋은 것 같아요!
internal fun <T> Response<T>.onResponse(): Result<T> { | ||
if (isSuccessful) { | ||
return Result.success(body() ?: Unit as T) | ||
} else { | ||
errorBody()?.let { | ||
return Result.failure( | ||
HttpResponseException( | ||
status = HttpResponseStatus.create(code()), | ||
msg = "", // Todo | ||
) | ||
) | ||
} ?: return Result.failure( | ||
HttpResponseException( | ||
status = HttpResponseStatus.create(-1), | ||
msg = "알 수 없는 에러입니다." | ||
) | ||
) | ||
} | ||
} |
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 : 확장 함수가 아닌 CallAdapter 를 상속해서 addCallAdapter에 변환하는 로직을 넣고 바로 Result로 받아오는 방식은 혹시 어떻게 생각하시나요?
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 : 확장 함수가 아닌 CallAdapter 를 상속해서 addCallAdapter에 변환하는 로직을 넣고 바로 Result로 받아오는 방식은 혹시 어떻게 생각하시나요?
CallAdapter가 어떤 건지는 알고있는데, 사용해본 적은 없어서 이번 기회에 한번 써보면 재밌을 것 같아요!
한번 CallAdapter로 바꿔볼게용~~
if (BuildConfig.DEBUG) { | ||
val loggingInterceptor = HttpLoggingInterceptor() | ||
loggingInterceptor.level = HttpLoggingInterceptor.Level.BODY | ||
builder.addInterceptor(loggingInterceptor) | ||
} |
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.
역시 디테일하십니다👍 👍
cbabe64
to
82c4957
Compare
82c4957
to
4f7b84f
Compare
1. ⭐️ 변경된 내용
2. 📌 이 부분은 꼭 봐주세요!
MainViewModel도 MavericsViewModel을 적용할 지 고민입니다!
그렇게 되면 MainState를 정의해야하고, MainViewModel을 MavericksViewModel로 구현해야 하며 뷰모델 팩토리가 추가적으로 생깁니다.
추가적으로 MainActivity에 MavericksView를 구현해야해서 불필요한 코드가 생깁니다..
일단 오버엔지니어링인 것 같아서 적용하진않았습니다...!
Mavericks Core Concept
Call Adapter를 처음으로 적용해봤습니다!
생각보다 이해하니까 쉽네요 ㅎㅎ,,, 어차피 저쪽 부분은 크게 수정사항이 없을 것 같아서 주석을 좀 덕지덕지 발라봤어요.
++ 코드에서 궁금한 부분 바로바로 질문해주세용!!
아직 API 명세가 fix된 것 같지 않아서 언제든지 바뀔 수 있습니다!