[review] Android Retrofit Request Code #67
Conversation
너무 피곤해서 커밋 막적음 ㅈㅅ
There was a problem hiding this comment.
흠;; 넘에 오가니제이션까지 쳐들어와서 리뷰를 남기는게 썩 유쾌하지만은 않쿤요.
쨌든 부탁했으니 내가 느낀점들은 말해주는데.. 존나 쓸데 없을 수도있음.
내가 처음 이놈의 패턴을 만드려고 했던건 그냥 내가 쓰기 편하려고 만들려던거였음. rx같은 사용성을 가진것 처럼 마개조를 한거지
처음엔 완벽하게 rx처럼 빌더패턴으로 구현하려했었음.
ㅁㅁRequest.setParams()
.set...Listener
.set...Listener
.send()
뭐 이런식으로. 근데 만들다보니 빌더패턴을 만들 줄 몰랐어서 apply블락을 써야만 하는 개병신 코드를 만들게된거임. 근데 시간이 없으니 그냥 그대로 쓰게 된거고.
각설하고,
나는 Callback을 사용해야만 하는 제약조건 + rx OR Coroutine 같은 비동기 처리 라이브러리를 사용하면 안되는 제약조건 때문에 편의상 "나만의 라이브러리"를 만든 느낌이지.
나는 라이브러리를 만들거면, 어떻게 사용할 것인지에 대해 먼저 설계를 치는게 맞다고 봐. 내가 처음에 "Rx처럼 빌더패턴으로 사용하게 만들어야지" 라고 한 것 처럼. 그런 코드를 써서 사용할 수 있게 코드들을 끼워 맞춘거임.
너도 어떻게 사용할 건지에 대해 먼저 설계해야된다고 봐. 그 다음에나 레이어 분리, 책임 분리를 하는거고. 그러니 나처럼 똑같이 apply를 쓰지 않으면 안되는 개 이상한 코드가 나오는거임
생성자엔 생성자대로 파라미터를 넣어야하고, 또 send를 보낼 땐 또 send 대로 토큰을 넣어야하고. 리스너는 또 리스너대로 달아야되고.
사용적인 측면에서 잘 봐봐. 처음 보는 사람이 이 라이브러리를 사용하는 데 무리가 없는가? 내 코드는 적어도 사용하는 데 처음 보는 사람은 이해가 안될수도 있을것 같음.
rx는 보셈. 존나 직관적이고 알흠다움. 그것만 봐도 몬 흐름인지 조차 알 수 있음.
아래는 옛날에 부스트코스 할 때 Request 객체에 대한 멘토님의 설명이었음 지금 기억도 안나긴 하지만, 뭔가 너가 Request객체를 만들고 싶어하는거 같아서 도움이 될진 모르겠지만 첨부함. 이때 사용했던 Volley 라이브러리이고, Retrofit과는 하등 상관없을 수도 있음. 아마 멘토님은 직접 HttpUrlConnection등 netty사용해서 레트로핏 같은 걸 만들길 바랐을 수도. 그래도 인사이트를 넓혀줄 수 있을거라 생각해서 넣음
흠 원하는 답변이랑은 존나 딴소리만 해서 도움이 안됐을 거 같은데, 이런 시도들은 시도 자체에 의미가 있다고 봄. 그러면서 고민하고, 또 다른걸 만들어보고. 리팩터링 해보고. 이거 자체에 의미를 두세요. 저는 코루틴 쓸겁니다 ^^
| if (response.isSuccessful) { | ||
| onSuccessListener?.invoke(response.body() ?: return) | ||
| return | ||
| } |
There was a problem hiding this comment.
이러면 통신은 됐는데 400번대 응답이라
body가 null로 들어오고 errorBody에서 꺼내야하는데, body ?: return 이면 errorbody까지 가지 못 갈듯
null 처리는 생각보다 고민을 많이해야함. exception 처리를 할 것인지 null처리를 할 것인지.
There was a problem hiding this comment.
아 무친 정신없이 짜느라 생각을 못했었는데 역시 형님 감사... 무리한 부탁에 좋은 답변, 좋은 레퍼런스 던져주니 너무 감사ㅠㅠ
| private fun createResponseErrorBody(errorBody: String): BaseResponse<Unit> { | ||
| val gson = GsonBuilder().create() | ||
| val responseType = object : TypeToken<BaseResponse<T>>() {}.type | ||
| return gson.fromJson(errorBody, responseType) | ||
| } |
There was a problem hiding this comment.
난 이게 무슨 의미가 있나 싶음. 이렇게 코드를 거지같이 내가 직접 짜야된다는 것은 라이브러리 만드는 사람들이 고려 하지 않았다는 부분인데,
이건 역설적으로 그 라이브러리 설계자들이 errorbody를 파싱할 필요가 없다 판단했다고 생각함.
반대로 생각했을 때, 클라가 왜 errorbody를 받아야하나? 패킷에 message를 넣을 수 있는 란이 있는데, 거기에 넣으면 충분히 서버가 하고 싶은 말을 할 수 있을거라 생각함. response.message이런식으로 접근할 수 있을거임 아마. 나는 개인적으로 errorbody파싱에 대해 굉장히 회의적임. (본인도 이런 코드를 짠 적이 많음)
There was a problem hiding this comment.
맞아 형 코드 보고 사실 나도 이부분에 대한 파싱을 잘 몰라서 그대로 가져오긴 했는데 사실 짜피 status코드는 파싱하지 않아도 알수 있다 생각해서 그거에 맞춘 처리만 적절히 해주는게 좋을지 아님 이렇게 에러바디를 자세하게 알아야할지 의심인 부분이 많았는데 형 말을 들어보니 라이브러리 설계자들이 errorbody를 파싱할 필요가 없다 판단 이 말이 와 닿는거 같아 감사감사
| fun send(token: String?) { | ||
| MeaningService.getInstance() | ||
| .requestPostTimestamp( | ||
| token = token, | ||
| params = createPartMap(), | ||
| image = image | ||
| ).enqueue(callback ?: return) | ||
| } |
There was a problem hiding this comment.
여기도 null처리 애매함.
콜백을 넣지 않고 send를 call 했다 치자. 그럼 아무일도 안일어날거임 앱에서는. 심지어 로그조차 찍히지 않겠지. 그럼 디버깅을 어디서 부터 해야할지 모르게 돼. 차라리 exception을 터트리는게 낫다고 봄. 왜냐면 callback을 반드시 넣어야하게끔 설계가 되있는거 같거든.
| fun onSuccessListener(listener: ((BaseResponse<T>) -> Unit)) { | ||
| this.onSuccessListener = listener | ||
| } |
There was a problem hiding this comment.
나도 첨에 setOn을 고려했고 형이 올려준 Rx처럼 해보기 위함이었는데 빌더로도 만들어봤으나 만드는 과정에서
"이렇게 만들꺼면 빌더 패턴이 필요할까?" 란 생각과 빌더 패턴의 뭐시기는 싹다 무시하고 그냥 억지로 붙여넣는 코드가 되어가서 apply setOn보다는 SetEvent on~~ 가 적절하다 생각했는데
피드백 너무 잘 읽었고 항상 너무 감사...ㅠㅠ
바빠 죽어갈탠데 무리한 부탁 들어줘서 너무 고맙고 진짜 언젠간 크게 성장해서 크게 한번 갚을께
jinsu4755
added
P 3️⃣
Description
Request 부분 코드 작성
고려사항.
BaseRequest?
callback을 구현하는것이 Request의 진짜 하위일까?
그랬다면 애초에 call.enque의 형식이 아니지 않을까?
call객체에 enqueue라는 callback을 구현해서 넣어주는 이유에 어긋 나는 부분일 것 같다 생각
우리가 만드는 Request는 메소드 호출로 반환된 call에 callback을 전달해주는 형식이라 생각함.
-> 상속구조를 하지 않기로 마음 먹음
그럼 중복 코드는?
사실 이렇게 만들면 Reqeust라는 클래스 안에 반드시 Callback 인스턴스를 가지고 그것을 apply하는 코드를 적어야함
그렇다고 이 코드 중복을 제거하는 상속구조?는 상속이란 개념에서 어긋났다고 생각했음.
차라리 확장함수를 정의해서 둘까? 생각했으나 시간상 빠위..
Request의 범위는 뭐지?
우리가 쓰는 enqueue는 execute부분에서 호출될것이며 비동기로 받아오기에 비동기로 받았을때 어떤일을 할지 정의해주는 부분이 Request의 범위라고 생각했음
즉 Requet라는 객체는 우리가 서버에 어떤데이터를 보내고 어떤 일을 할지 지정하는 책임이 있다고 생각함.
그렇기에 callback을 상속이 아닌 내부에 인스턴스로 가지도록 함.
그리고 Request에서 보낼 데이터를 가공함.
생성자로 들어올 매개 변수가 많은 경우는 DataCalss로 받을 생각함.
결국 이 경우에 컴포지션을 어떻게 해야할지 감이 안잡힘 이팩티브 자바 예시는 Set이 달려있는데 그걸 이해하지 못하는 한계와 자신의 부족함으로 인해 현타옴
젠장.... 난 감자야... ㅅㅂ 말하는 감자...
참고
해당 부분 코드리뷰를 위해서 일부러 이름에 Review를 붙였습니다. 원래 Review가 없다고 생각해주시면 됩니다.
희망 리뷰 완료일
없음 혁이형 편할때 한가할때 해주면 진짜 매우 감사 ㅇㅈ?