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

[review] Android Retrofit Request Code #67

Open
wants to merge 2 commits into
base: fix/design
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* Created by jinsu4755
* DESC: callback 객체 구현하는 Request Class 시도해봄 그냥 궁금해서
*/

package meaning.morning.network.request

import android.text.TextUtils
import android.util.Log
import com.google.gson.GsonBuilder
import com.google.gson.reflect.TypeToken
import meaning.morning.network.response.BaseResponse
import retrofit2.Call
import retrofit2.Callback
import retrofit2.Response

class RequestCallbackReview<T> : Callback<BaseResponse<T>> {

private var onSuccessListener: ((BaseResponse<T>) -> Unit)? = null
private var onErrorListener: ((BaseResponse<Unit>) -> Unit)? = null
private var onFailureListener: ((t: Throwable) -> Unit)? = null

fun onSuccessListener(listener: ((BaseResponse<T>) -> Unit)) {
this.onSuccessListener = listener
}
Comment on lines +23 to +25

Choose a reason for hiding this comment

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

나라면 setOn...Listener라고 지을듯

Copy link
Member Author

Choose a reason for hiding this comment

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

나도 첨에 setOn을 고려했고 형이 올려준 Rx처럼 해보기 위함이었는데 빌더로도 만들어봤으나 만드는 과정에서
"이렇게 만들꺼면 빌더 패턴이 필요할까?" 란 생각과 빌더 패턴의 뭐시기는 싹다 무시하고 그냥 억지로 붙여넣는 코드가 되어가서 apply setOn보다는 SetEvent on~~ 가 적절하다 생각했는데

피드백 너무 잘 읽었고 항상 너무 감사...ㅠㅠ
바빠 죽어갈탠데 무리한 부탁 들어줘서 너무 고맙고 진짜 언젠간 크게 성장해서 크게 한번 갚을께

Choose a reason for hiding this comment

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

그래그래 크게 네이버 꽂는걸로 갚아라


fun onErrorListener(listener: (BaseResponse<Unit>) -> Unit) {
this.onErrorListener = listener
}

fun onFailureListener(listener: (t: Throwable) -> Unit) {
this.onFailureListener = listener
}

override fun onResponse(call: Call<BaseResponse<T>>, response: Response<BaseResponse<T>>) {
if (response.isSuccessful) {
onSuccessListener?.invoke(response.body() ?: return)
return
}
Comment on lines +36 to +39

Choose a reason for hiding this comment

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

이러면 통신은 됐는데 400번대 응답이라
body가 null로 들어오고 errorBody에서 꺼내야하는데, body ?: return 이면 errorbody까지 가지 못 갈듯
null 처리는 생각보다 고민을 많이해야함. exception 처리를 할 것인지 null처리를 할 것인지.

Copy link
Member Author

Choose a reason for hiding this comment

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

아 무친 정신없이 짜느라 생각을 못했었는데 역시 형님 감사... 무리한 부탁에 좋은 답변, 좋은 레퍼런스 던져주니 너무 감사ㅠㅠ

val errorBody = response.errorBody()?.string() ?: return
val errorResponse = createResponseErrorBody(errorBody)
onErrorListener?.invoke(errorResponse)
}

private fun createResponseErrorBody(errorBody: String): BaseResponse<Unit> {
val gson = GsonBuilder().create()
val responseType = object : TypeToken<BaseResponse<T>>() {}.type
return gson.fromJson(errorBody, responseType)
}
Comment on lines +45 to +49

Choose a reason for hiding this comment

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

난 이게 무슨 의미가 있나 싶음. 이렇게 코드를 거지같이 내가 직접 짜야된다는 것은 라이브러리 만드는 사람들이 고려 하지 않았다는 부분인데,
이건 역설적으로 그 라이브러리 설계자들이 errorbody를 파싱할 필요가 없다 판단했다고 생각함.

반대로 생각했을 때, 클라가 왜 errorbody를 받아야하나? 패킷에 message를 넣을 수 있는 란이 있는데, 거기에 넣으면 충분히 서버가 하고 싶은 말을 할 수 있을거라 생각함. response.message이런식으로 접근할 수 있을거임 아마. 나는 개인적으로 errorbody파싱에 대해 굉장히 회의적임. (본인도 이런 코드를 짠 적이 많음)

Copy link
Member Author

Choose a reason for hiding this comment

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

맞아 형 코드 보고 사실 나도 이부분에 대한 파싱을 잘 몰라서 그대로 가져오긴 했는데 사실 짜피 status코드는 파싱하지 않아도 알수 있다 생각해서 그거에 맞춘 처리만 적절히 해주는게 좋을지 아님 이렇게 에러바디를 자세하게 알아야할지 의심인 부분이 많았는데 형 말을 들어보니 라이브러리 설계자들이 errorbody를 파싱할 필요가 없다 판단 이 말이 와 닿는거 같아 감사감사


override fun onFailure(call: Call<BaseResponse<T>>, t: Throwable) {
Log.d("jinsu4755", "${t.message} \n")
Log.d("jinsu4755", "${t.localizedMessage} \n")
Log.d("jinsu4755", TextUtils.join("\n", t.stackTrace))
onFailureListener?.invoke(t)
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
* Created by jinsu4755
* DESC:
*/

package meaning.morning.network.request

import meaning.morning.network.MeaningService
import meaning.morning.network.response.TimeStampResponse
import okhttp3.MediaType.Companion.toMediaType
import okhttp3.MultipartBody
import okhttp3.RequestBody
import okhttp3.RequestBody.Companion.toRequestBody

class TimeStampPostRequestReview(
private val dateTime: String,
private val timeStampContent: String,
private val image: MultipartBody.Part,
) {
private var callback: RequestCallbackReview<TimeStampResponse>? = null

fun setEvent(block: RequestCallbackReview<TimeStampResponse>.() -> Unit): TimeStampPostRequestReview {
callback = RequestCallbackReview<TimeStampResponse>().apply(block)
return this
}

fun send(token: String?) {
MeaningService.getInstance()
.requestPostTimestamp(
token = token,
params = createPartMap(),
image = image
).enqueue(callback ?: return)
}
Comment on lines +27 to +34

Choose a reason for hiding this comment

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

여기도 null처리 애매함.
콜백을 넣지 않고 send를 call 했다 치자. 그럼 아무일도 안일어날거임 앱에서는. 심지어 로그조차 찍히지 않겠지. 그럼 디버깅을 어디서 부터 해야할지 모르게 돼. 차라리 exception을 터트리는게 낫다고 봄. 왜냐면 callback을 반드시 넣어야하게끔 설계가 되있는거 같거든.

Copy link
Member Author

Choose a reason for hiding this comment

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

맞습니다 행님... 그저 빛


private fun createPartMap(): HashMap<String, RequestBody> {
val partMap = HashMap<String, RequestBody>()
partMap["dateTime"] = dateTime.toRequestBody(MEDIA_TYPE_TEXT)
partMap["timeStampContents"] = timeStampContent.toRequestBody(MEDIA_TYPE_TEXT)
return partMap
}

companion object {
private val MEDIA_TYPE_TEXT = "text/plain".toMediaType()
}
}