[REFACTOR] 코스 상세페이지 / 전면 리팩토링

[leeeha]

📌 개요

• closed [REFACTOR] 코스 상세페이지 / 전면 리팩토링  #267

✨ 작업 내용

• 자신이 업로드한 코스 수정/삭제 시, 바텀시트 대신 팝업메뉴 뜨도록 변경
• 자신의 코스는 수정/삭제, 다른 사람 코스는 신고하기 팝업 뜨도록 구분
• 하드코딩한 문자열 리소스화
• 코스 상세페이지 조회/수정/삭제에 대한 서버통신 코드 수정 (BaseResponse, UiStateV2 사용하도록)
• 내용 수정하다가 중간에 뒤로가기 누르는 경우, 작성 중단 확인 다이얼로그 띄우기
• kotlin-android-extensions (deprecated) 사용하는 다이얼로그 걷어내기 (데이터 바인딩 사용)
• 코스 상세페이지의 이전 화면들을 문자열 대신 enum class로 구분하기
• 리팩토링 후 발생하는 각종 버그 해결하기

✨ PR 포인트

• 브랜치 사이즈를 작게 가져가려고 했으나,,,,, 코드들이 너무 긴밀하게 연결되어 있어서 전반적으로 같이 살펴보면서 작업하다보니 이번에도 작업량이 많아졌습니다,,, 이제 어느 정도 기존 코드 파악이 되고 있으니, 앞으로는 브랜치 사이즈를 훨씬 작게 가져가서 리뷰어분도 부담을 덜 수 있도록 노력하겠습니다!!
• 아직 해결하지 못한 기존 버그들이 몇개 더 남아있는데, 이는 별도 브랜치에서 작업하겠습니다!
• 서버 api 명세서와 실제 응답값이 달라서 에러가 발생하기도 했는데, 이는 서버 측에 얘기하도록 할게요!
• 기존 코드와 제 코드 스타일을 맞춰나가는 게 역시나 쉽지 않은 작업이네요! 그리고 제 코드 또한 최선이라는 보장이 없다고 생각해요! 그래서 pr 분량이 상당히 많긴 합니다만,,ㅎㅎ 시간이 되는 선에서!! 코드 변경사항에 대한 다양한 의견과 피드백 부탁드립니다!!
• 그리고 제가 미처 발견하지 못한 버그가 숨어있을 수도 있으니, 직접 실행시켜보면서 확인해주시면 감사하겠습니다!

[unam98]

kotlin-extension 걷어낸 거 굿굿 안 그래도 고민하고 있던 부분이었는데 말 하기도 전에 해결해줬네 너무 고생 많았어 땡큐!!

[unam98]

data 가공 함수를 이렇게 data class 내에 직접 작성하는 것과 확장함수로 따로 빼서 만드는 것 중 어떤 것이 더 나을까요?

[leeeha]

데이터 가공 작업은 util 패키지 보다는 data layer에서 진행하는 게 더 좋을 거 같습니다!
관련 없는 dto들을 전부 ResponseExt.kt 파일에 모아두는 것보다는
data layer의 각 dto 파일 안에서 가공 작업을 하는 게 좀 더 관심사가 분리된다는 느낌이 들었는데
어떻게 생각하시는지요!

[unam98]

좋은 의견입니다! 그동안 제 개인적인 생각으로 data class는 어떤 data를 다루는 건지 틀을 정의해놓는 공간이라 생각하여 가공 logic(함수)가 같은 파일 내에 담겨있는 게 깔끔하지 못하다고 생각했는데

한편으로는 data "class"니까 안에 함수가 포함돼도 문제될 게 없을 것 같다는 생각도 드네요. 또 같은 파일 내에서 함수를 작성해주는 만큼 변수 참조도 바로 가능해서 사소하지만 더 편할 것 같구요.

[unam98]

같은 질문을 안드로이드 오픈 채팅방에 올려봤는데 아래와 같은 의견을 받았습니다.

확장 함수는 선언한 파일의 정적 메서드로 생기는 것으로 알고 있어요
그래서 이에 따라 생기는 문제를 알고 쓰시면 될 것 같습니다

확장함수를 쓸 때 유의할 점이 있는지 관련 포스팅을 찾아봤습니다.

1. https://www.roach-dev.com/kotlin/kotlin_extension_function/
2. https://todaycode.tistory.com/176

요약하자면,

1. 의도대로 동작을 안 할 수도 있으니 한 클래스에 대한 확장함수를 만들 때 다른 클래스의 값을 참조하지 않게 하라
2. 상속 불가능
3. public 멤버만 참조 가능
4. 같은 이름의 함수가 클래스 내에 존재할 때 확장함수보다 더 우선순위로 채택이 된다
5. 정적 바인딩

지금 서버에서 받아온 data를 가공하는 case는 상속 관계가 이루어질 일이 없으니 확장 함수를 써도 문제 될 일(조심해야 할 일)은 없지만 편의상 말씀해 주신 것처럼 data class 내에 함수를 만들어 쓰는 게 더 좋겠다고 느꼈습니다.

[unam98]

추후에 이것만을 다룬 브랜치를 파서 리팩토링 후 pr 올리면 좋을 것 같습니다.

[dongx0915]

근데 effective 코틀린에서 확장 함수를 클래스 내에 정의하는 것은 좋지 않다고 나와있는 것 같네요

• https://onlyfor-me-blog.tistory.com/730
• https://thdev.tech/kotlin/2020/10/27/kotlin_effective_08/

제가 제안 드린 방법은 DataMapper를 통한 계층간 접근 제한 + 클래스 내부에 Mapper를 정의하여 관계를 명확히 표현하는 것에 초점을 맞춘 방법이었는데, 확장 함수 위치에 대한 건 생각을 못한 것 같습니다! 저도 많이 배워가네요

정리하자면 확장 함수를 클래스 내에 정의하는 것은 좋지 않으니 하은 님이 구현하신 것처럼 Dto 내에 멤버 메소드로 정의하는 것도 괜찮아보입니다.

클래스의 멤버로 Mapper 메소드를 선언한다면 굳이 확장 함수를 쓰지 않아도 될 것 같구요!
확장 함수로 쓸 지, 클래스 내의 멤버 메서드로 쓸 지는 팀 스타일에 맞춰 이용하면 될 것 같아요

개인적으로 확장 함수는 클래스 내부를 건들기 힘들 때 속성이나 메소드를 추가하기 위한 목적이 큰 것 같아서 지금 하은님이 구현해주신 방식이 제일 괜찮아 보이는 것 같습니다!

[dongx0915]

어디까지를 "하나의 책임"으로 볼 것인지가 논점인 것 같네요. 데이터 정의'만' 하는 것도 하나의 책임으로 볼 것인지, 아니면 구체적으로 어떤 동작(mapping)을 해야만 하나의 책임으로 취급을 해줄 것인지.

이 부분은 저희 팀원끼리 어떤 관점으로 볼 지 정하면 될 것 같아요!

질문 내용도 남길게요~!

[leeeha]

다들 코드리뷰에 진심이시군요!!! 정성 있는 코멘트 많이 작성해주셔서 넘 감사합니다 🙇‍♀️🙇‍♀️
앞으로도 디스커션 많이 하면서 같이 배워나가요~~!

[unam98]

말씀해주신 내용이 서버로 부터 받은 Response 객체가 여러 Domain 모델로 변경될 수 있다는 뜻일까요?

아뇨 그런 의미는 아니었고 단순히 data가 여러 형태로 가공될 수 있는 여지가 제한된다는 점만 언급한 것이었습니다! Response 객체를 다룬다는 상황에만 특정한다면 저도 여러 형태로 가공될 여지는 적거나 없다고 생각합니다!

[unam98]

근데 effective 코틀린에서 확장 함수를 클래스 내에 정의하는 것은 좋지 않다고 나와있는 것 같네요

링크 감사합니다~! 저는 프리미티브 타입과 같이 클래스 내에 멤버로 함수를 추가할 수 없을 때 확장함수를 쓰는 효용이 있다고 생각하는데요. 클래스 내에 멤버로 추가할 수 있다면 굳이 확장함수를 쓸 이유가 없는 것 같습니다! 아마 적어주신 내용은 한 클래스에 여러 클래스의 확장함수를 멤버로 추가해쓰는 걸 지양하라는 맥락 같은데 맞겠죠?!

저도 하은이 방법이 괜찮은 것 같습니다~!

[unam98]

뷰모델이 아닌 Impl에서 runCatching을 작성해주신 특별한 이유가 있을까요?

[leeeha]

runCatching을 통해 Result 타입으로 바꾸는 작업은
UI Layer 보다 Data Layer에서 먼저 처리해주는 게 개발 작업 시 편하더라구요!

서버통신 결과를 Result 타입으로 넘겨주는 건 Data Layer에서 (RepositoryImpl)
그 값을 onSuccess, onFailure 블록으로 나눠서 UiState를 갱신해주는 건 UI Layer의 뷰모델에서

진행하는 것이 개인적으로 편했습니다! Data, UI 라는 각 Layer의 역할에도 더 맞는 거 같구요..!

[unam98]

코드 상으로 큰 차이는 아니지만 Impl 쪽으로 옮겨줌으로써 뷰모델에 작성하는 코드량을 좀 더 줄인 것으로 이해해도 될까요?!

[leeeha]

넵넵!

[unam98]

BaseResponse는 무엇인가요?

[leeeha]

@Serializable
data class BaseResponse<T>(
    @SerialName("status")
    val status: Int,
    @SerialName("success")
    val success: Boolean,
    @SerialName("message")
    val message: String,
    @SerialName("data")
    val data: T? = null
)

서버통신 응답값에서 매번 중복되는 부분을 이렇게 제네릭 타입을 이용해서 일반화 해준 것입니다!
데이터 타입 T만 바꿔주면 되기 때문에 중복되는 코드를 줄일 수 있어 좋더라구요!!

[unam98]

중복 코드가 어떤 식으로 발생하는지 여쭤봐도 될까요? 이렇게 BaseResponse를 만들어 썼을 때의 효용이 잘 체감이 안 돼서요!

[leeeha]

서버 api 명세서를 보시면 매번 status, success, message, data가 응답값으로 오며, data의 타입만 바뀌는 구조라는 걸 알 수 있습니다! 그래서 data 타입만 제네릭으로 만들어서 중복 코드를 줄인 것입니다!

[unam98]

t를 좀 더 구체적인 이름으로 바꿔주시면 좋을 것 같습니다!

[unam98]

여기서 발생할 수 있는 HttpException이란 어떤 것일까요?

[leeeha]

말그대로 HTTP 서버통신 과정에서 발생할 수 있는 예외를 처리하는 클래스입니다.

/** Exception for an unexpected, non-2xx HTTP response. */
public class HttpException extends RuntimeException {
  private static String getMessage(Response<?> response) {
    Objects.requireNonNull(response, "response == null");
    return "HTTP " + response.code() + " " + response.message();
  }

  private final int code;
  private final String message;
  private final transient Response<?> response;

  public HttpException(Response<?> response) {
    super(getMessage(response));
    this.code = response.code();
    this.message = response.message();
    this.response = response;
  }

  /** HTTP status code. */
  public int code() {
    return code;
  }

  /** HTTP status message. */
  public String message() {
    return message;
  }

  /** The full HTTP response. This may be null if the exception was serialized. */
  public @Nullable Response<?> response() {
    return response;
  }
}

이 클래스를 이용하면 위와 같이 code, message, response 정보들을 참조할 수 있어서 사용했습니다!

[unam98]

이렇게 if문으로 (t is HttpException)를 안 만들어줘도 fail 시 message 등의 값들을 참조할 수 있지 않나요?!

저는 그동안 이렇게 HttpException 조건문 블록을 열어주지 않았는데도 fail 시 어떤 상황 때문에 일어난 건지 log를 찍어볼 수 있었거든요. 제가 잘 모르는 것일 수도(그동안 잘못알고 있던 것일 수도) 있는데 이렇게 HttpException 조건문 블록을 열어주는 것이 어떤 의도인지, 어떤 효용을 얻을 수 있는지 궁금합니다.

[leeeha]

넵 onFailure 블록이 넘겨주는 Throwable 타입의 인자로도 다음과 같은 정보들을 얻을 수 있습니다!

그런데 그 Throwable이 HttpException인 경우에는 상태 코드와 같이 HTTP 와 관련된 좀 더 구체적인 정보를 얻을 수 있어서 유용하다 생각했습니다!

[leeeha]

그런데 현재 HttpLoggingInterceptor 이용해서 서버통신과 관련된 자세한 로그 정보를 확인할 수 있기 때문에, Timber로 로그를 찍는 건 불필요하다고 판단하여 관련 코드를 삭제했습니다!

[unam98]

이 부분도 t를 더 구체적인 이름으로 바꿔주시면 좋을 것 같아요!

[unam98]

getString과 stringOf는 어떤 차이인가요?

[leeeha]

사실 기능은 똑같은데, 아래와 같이 ContextExt.kt 에서 리소스를 가져와 사용하는 코드를 확장함수로 만들다보니 이걸 사용하려고 getString 대신 stringOf를 사용했습니다.

fun Context.stringOf(@StringRes resId: Int) = getString(resId)

fun Context.colorOf(@ColorRes resId: Int) = ContextCompat.getColor(this, resId)

fun Context.drawableOf(@DrawableRes resId: Int) = ContextCompat.getDrawable(this, resId)

colorOf, drawableOf는 코드량을 줄여줘서 굉장히 편리하게 사용했었는데, stringOf는 불필요하게 확장함수를 한번 거쳐서 getString을 호출하는 거 같아서 getString을 직접 사용하는 게 더 좋을 거 같습니다!

stringOf 함수는 지우고 getString 사용하도록 바꾸겠습니다!

[unam98]

Glide가 아닌 Coil을 쓰신 이유가 궁금합니다.

[leeeha]

https://velog.io/@jshme/Android-Image-Loader-Library

저도 위의 글 읽어보며 더 공부해보겠습니다! 저번 기수 세미나 과제 때 Coil 사용을 권장했던 OB 분이 있었는데, 그 이후로 이 라이브러리 자체에 대해 자세히 알아본 적이 없어서, 저도 지금 사용법만 대략 알고 있는 정도네요!

질문 주셔서 감사합니다!!

[unam98]

저도 알아보겠습니다! 공부할 게 끝이 없네요 (털썩)

[unam98]

굿

[leeeha]

데이터 바인딩이 안티 패턴이 될 수도 있다고 하네요,, 성능이나 가독성 측면에서 안 좋다고 듣긴 했는데, 저도 구체적인 이유를 더 알아봐야 할 거 같아요! 그래서 요즘엔 Compose를 권장하는 추세인 거 같아요.

[unam98]

아래와 같은 포스팅이 있네요! 테스트 코드를 짜본 적이 없어서 체감은 잘 안 되지만, 가끔 data binding 관련 error 뜰 때 보통의 error는 어떤 부분에서 문제가 생긴 건지 구체적으로 알려주는 반면 data binding은 그냥 이쪽에 문제가 있다고만 떴던 거 같아서 문제 추적이 어렵다는 게 이런 걸 얘기하는 것인가 싶네요.

새로 알게 된 내용이 있으면 공유해주세요! 땡큐!

https://velog.io/@beokbeok/%EC%99%9C-%EB%8D%B0%EC%9D%B4%ED%84%B0%EB%B0%94%EC%9D%B8%EB%94%A9-%EC%82%AC%EC%9A%A9%EC%9D%84-%EB%A9%88%EC%B6%B0%EC%95%BC%ED%95%A0%EA%B9%8C

[unam98]

저는 Dialog를 확장함수로 만들어서 썼는데 이렇게 class를 만들어 쓰는 것과 비교해서 어떤 것이 더 낫다고 생각하시는지 의견 궁금합니다! (어떤 특정 방법을 강요하는 것이 아닌 discussion의 목적으로 물어보는 것)

제가 만든 Dialog 확장함수는 setActivityDialog, setFragmentDialog입니다!

[leeeha]

AlertDialog vs. DialogFragment 차이점에 대한 질문으로도 생각해볼 수 있는데요!

https://stackoverflow.com/questions/13765127/dialogfragment-advantages-over-alertdialog

위의 글을 읽어보니 DialogFragment는 프래그먼트이다보니 생명주기에 따라 처리를 해줄 수 있다는 게 큰 장점인 거 같아요!

Have you ever seen window leaks if you didn't close a dialog when its Activity was getting destroyed?
So to prevent that, have you ever tried to close the dialog when onPause() was called?
So to do that, have you ever had to make a reference of that dialog to a class level object?
With DialogFragment, it's all handled. And you get all lifecycle callbacks. Then you can provide more intelligence to the dialog and make it do some smart work on its own rather than Activity telling it what to do.

이거 읽고 저번 회의 때 언급했던 WindowLeak 에러가 났던 이유를 알 수 있었습니다,, ㅎㅎ

프래그먼트 생명주기에 따라 처리를 해줄 수 있는 DialogFragment를 사용하는 게 더 좋을 거 같습니다!

[leeeha]

아 그리고 최근에 위니에서 이 부분을 놓치고 있었다는 걸 다른 팀원이 알려줘서 알았는데요!

CommonDialogFragment, RequireLoginDialogFragment에 모두 반영하여 다시 푸시하도록 하겠습니다!!

[unam98]

Activity가 소멸됐는데도 dialog는 남아있어서 에러가 떴던 거군요. 감사합니다!

[unam98]

Activity 하위에 DialogFragment를 만들면 해당 Activity가 소멸될 때 DialogFragment도 자동으로 객체 소멸이 되나요?

[unam98]

AlertDialog가 아닌 DialogFragment를 썼을 때의 단점은 뭐가 있을까요? (discussion 목적으로 물어보는 것)

[leeeha]

액티비티와 프래그먼트는 각각 독립적인 생명주기를 가지고 있지만, 프래그먼트는 액티비티에 종속적인 존재입니다. 따라서 액티비티가 파괴되면, 그에 연결된 프래그먼트들도 같이 파괴되는 것이 기본적인 동작입니다.
하지만 이러한 동작은 프래그먼트를 사용하는 방식이나 상황에 따라 다르게 처리될 수 있습니다. 예를 들어, 프래그먼트를 백스택에 추가하거나, setRetainInstance(true)를 호출하여 인스턴스 상태를 유지하도록 설정하면, 액티비티가 파괴되어도 프래그먼트는 파괴되지 않을 수 있습니다. 이런 경우, 액티비티가 다시 생성될 때 원래의 프래그먼트 인스턴스를 재사용할 수 있습니다.

gpt 설명이 맞는 거 같습니다! ㅎㅎ

[leeeha]

안드로이드에서 AlertDialog와 DialogFragment는 화면에 다이얼로그를 표시하는 데 사용되는 두 가지 주요 방법입니다. 이들은 각각 다음과 같은 장단점을 가지고 있습니다.

AlertDialog

• 장점: AlertDialog는 간단하고 빠르게 구현할 수 있는 장점이 있습니다. 코드를 적게 사용하므로 빠르게 다이얼로그를 생성하고 표시할 수 있습니다.
• 단점: 하지만 생명주기 관리 측면에서는 단점이 있습니다. AlertDialog는 그 자체로 생명주기를 가지지 않아서, 화면 회전이나 액티비티의 종료 등의 상황에서 다이얼로그가 자동으로 관리되지 않습니다. 따라서 액티비티가 파괴될 때 다이얼로그를 닫지 않으면 윈도우 누수(window leak)가 발생할 수 있습니다. 이를 방지하기 위해 onPause()가 호출될 때 다이얼로그를 닫는 등의 추가 작업이 필요합니다. 이를 위해선 다이얼로그에 대한 참조를 클래스 레벨 객체로 유지해야 할 수도 있습니다.

DialogFragment

• 장점: DialogFragment는 프래그먼트의 생명주기를 따르므로, 화면 회전이나 액티비티의 종료 등의 상황에서 자동으로 관리됩니다. 따라서 윈도우 누수와 같은 문제를 자동으로 방지할 수 있습니다. 또한 모든 생명주기 콜백을 받게 되므로, 다이얼로그에 더 많은 지능을 제공하고 액티비티 대신 스스로 일을 처리하도록 만들 수 있습니다.
• 단점: 그러나 DialogFragment의 구현은 AlertDialog보다 복잡할 수 있습니다. 프래그먼트의 생명주기를 이해하고 관리해야 하므로 코드가 복잡해질 수 있습니다.

따라서 어떤 방식을 선택할지는 사용 상황과 개발자의 선호에 따라 달라질 수 있습니다. 간단한 다이얼로그를 빠르게 표시하려면 AlertDialog를, 생명주기 관리와 윈도우 누수 방지 등의 장점을 원한다면 DialogFragment를 사용하는 것이 좋을 수 있습니다.

gpt가 설명을 참 잘하네요..^^ 제가 하고 싶은 말 그대로 정리해놨어요!

[unam98]

굿