[부나] 2, 3단계 자동 DI 미션 제출합니다

[tmdgh1592]

안녕하세요 반달!
2, 3단계 미션 완료하여 리뷰요청드립니다.

요구사항 체크리스트는 다음과 같습니다.

2단계

• ViewModel 내 필드 주입을 구현한다.
• 의존성 주입이 필요한 필드와 그렇지 않은 필드를 구분할 수 없다.
• Annotation을 붙여서 필요한 요소에만 의존성을 주입한다.
• 내가 만든 의존성 라이브러리가 제대로 작동하는지 테스트 코드를 작성한다.
• CartRepository가 다음과 같이 DAO 객체를 참조하도록 변경한다.
• 현재는 장바구니 아이템 삭제 버튼을 누르면 RecyclerView의 position에 해당하는 상품이 지워진다.
• 상품의 position과 CartRepository::deleteCartProduct의 id가 동일한 값임을 보장할 수 없다는 문제를 해결한다.
• 뷰에서 CartProductEntity를 직접 참조하지 않는다.

3단계

• 하나의 인터페이스의 여러 구현체가 DI 컨테이너에 등록된 경우, 어떤 의존성을 가져와야 할지 알 수 없다.
• 상황에 따라 개발자가 Room DB 의존성을 주입받을지, In-Memory 의존성을 주입받을지 선택할 수 있다.
• 내가 만든 DI 라이브러리를 모듈로 분리한다.
• DSL을 활용한다.
• 내가 만든 DI 라이브러리를 배포하고 적용한다.

data class Cache(
    private val cache: MutableMap<DependencyKey, Any?> = mutableMapOf(),
) {
    fun caching(module: Module, provider: KFunction<*>) {
        val dependencyKey = DependencyKey.createDependencyKey(provider)
        val dependency = provider.call(module)

        cache[dependencyKey] = dependency
    }
    ...

지금 당장 하지 않은 로직

한 번 생성한 객체를 Cache 객체에 저장해두는 구조입니다.
지금 당장은 문제가 되지 않지만, 경우에 따라 캐싱없이 매번 새롭게 생성되어야 하는 객체들도 있을 것입니다.
그런 문제는 이후에 @Singleton과 같은 애노테이션을 통해 해결해보려고 합니다.

리뷰 잘 부탁드립니다 : )

[woowahan-leah]

안녕하세요 부나!
오프라인으로 드렸던 피드백에 이어서 이렇게 PR 리뷰를 하게 되었네요 ㅋㅋㅋ
구조 설계하시느라 고생 많으셨습니다.
제가 드린 코멘트는 참고만 해주시고 리뷰어인 반달의 가이드를 따라가주시면 됩니다.

[woowahan-leah]

사용되지 않는 모듈은 제거해주세요.

[woowahan-leah]

프로젝트 내에서 VERSION_1_8, VERSION_11이 혼용되고 있어요.

[tmdgh1592]

오우..!
모두 11로 통일하였습니다 👍

[woowahan-leah]

Repository 구현체 선언부에서 DatabaseCartRepositoryQualifier를 선언하는 것도 좋지만 Repository 구현체가 직접 DI 로직을 참조해야 한다는 단점이 있어요. 아키텍처 피드백은 최대한 드리지 않으려고 했지만, 지금 구조에서는 DI 라이브러리가 교체된다면 해당 Repository 구현체도 수정되어야 합니다.

Repository 구현체가 Qualifier를 의존하기보다, 의존 관계를 선언하는 선언부에서 Qualifier를 설정해두면 어떨까요?

        // 기존
        types {
            type(CartRepository::class to InMemoryCartRepository::class)
            type(CartRepository::class to DatabaseCartRepository::class)
        }

        // 개선 예시
        // InMemoryCartRepositoryQualifier인 경우 -> CartRepository의 구현체는 InMemoryCartRepository이다
        // DatabaseCartRepositoryQualifier인 경우 -> CartRepository의 구현체는 DatabaseCartRepository이다

지금 구현하신 RepositoryModule과 같이 설정하는 구조를 활용하는 것도 좋습니다.

[tmdgh1592]

제가 생각을 잘못하고 있었네요...!
DatabaseCartRepository에 애노테이션을 추가하지 않아도 되도록 변경해보겠습니다.

class MainViewModel(
    private val productRepository: ProductRepository,
    @InMemoryCartRepositoryQualifier
    private val cartRepository: CartRepository,
) : ViewModel() {

위와 같은 구조로 변경해보겠습니다~

[woowahan-leah]

사실 모든 의존 관계에서 Qualifier가 필요하지는 않습니다. 실제 다른 DI 라이브러리를 학습하다보면 아시겠지만 Qualifier 개념은 후반부에 나오는 개념이에요. (== Qualifier를 몰라도 대부분의 의존성 주입은 가능하다는 의미입니다.)

[tmdgh1592]

사실 Hilt와 같은 구조를 따라가고 싶어서 Qualifier를 사용했습니다. (물론 Hilt는 컴파일 타임이지만요.. 😅)
ProductRepository의 서브타입이 더 늘어났을 때를 가정해서 미리 애노테이션을 추가하였는데,
지금 당장 필요해보이지 않아서 제거 하였습니다!

[woowahan-leah]

한 번 생성한 객체를 Cache 객체에 저장해두는 구조입니다.
지금 당장은 문제가 되지 않지만, 경우에 따라 캐싱없이 매번 새롭게 생성되어야 하는 객체들도 있을 것입니다.

바로 다음 미션에서 고민하게 되시겠네요!

[woowahan-leah]

실질적으로 clear하고 있지 않습니다.

    fun clear(): Cache {
        return copy(cache = mutableMapOf())
    }

[tmdgh1592]

방어적 복사만 하고 있고 실질적으로 제거는 하고 있지 않네요!
SubTypeConverter, Cache 클래스 모두 다음과 같이 변경하였습니다.

fun clear(): SubTypeConverter {
    converter.clear()
    return copy(converter = mutableMapOf())
}

[woowahan-leah]

현재의 DependencyInjector는 ViewModel Inject를 고려하고 설계되었지만, 사실 꼭 ViewModel에 의존할 필요는 없습니다.
모든 클래스를 커버할 수 있도록 ViewModel 의존을 걷어내신다면, 굳이 SubTypeConverter와 같은 클래스 없이도 재귀 주입을 구현할 수 있게 되실거예요.

요약: DI 라이브러리가 Android Library가 아니여도 된다.
이 코멘트는 큰 구조 변경을 요하기 때문에 참고만 해주세요! 지금 단계에서는 넘어가도 좋습니다~

[woowahan-leah]

DI 라이브러리가 제대로 작동하는지 검증하기 위한 시나리오가 많이 누락되었습니다.

현재의 테스트 코드에서는

1. Qualifier에 대한 검증이 이뤄지는 것처럼 보이지 않습니다. 실제로는 가짜 데이터로 검증을 하고 있을지 몰라도, 지금보다 더 다양한 시나리오가 나올 수 있을 것 같아요. 예를 들어 저라면 다음 시나리오 등을 테스트해볼 것 같습니다.

// 성공 케이스(인터페이스 1, 구현체 1)
class ConstructorTestViewModel(
    val fooDependency: FakeRepository,
)
// 실패 케이스(인터페이스 1, 구현체 2)
class ConstructorTestViewModel(
    val fooDependency: FakeRepository,
)
// 성공 케이스(인터페이스 1, 구현체 2)
class ConstructorTestViewModel(
    @FooQualifier
    val fooDependency: FakeRepository,
)
// 성공 케이스(인터페이스 1, 구현체 2)
class ConstructorTestViewModel(
    @FooQualifier
    val fooDependency: FakeRepository,
    @BarQualifier
    val barDependency: FakeRepository,
)

2. 재귀 주입도 테스트되고 있지 않습니다.
3. 필드 주입에서도 Qualifier를 사용할 수 있지 않을까요?
4. 개발자들이 굳이 가짜 테스트용 데이터를 확인하지 않더라도, 테스트 코드만으로 한눈에 어떤 값을 검증하려고 하는지 확인할 수 있게 개선해보면 어떨까요?

이외에도 누락된 시나리오가 있나요?

[tmdgh1592]

성공하는 테스트 케이스만 있다보니, 다시 봐도 프로덕션 코드를 제대로 검증하지 못하는 것 같았습니다.
말씀해주신 것처럼 조금 더 다양한 상황을 검증하는 테스트 케이스를 추가하여 추가 커밋하였습니다.

[tmdgh1592]

반달! 레아가 남겨주신 코멘트를 기반으로 추가 커밋하였습니다.
코드리뷰시 참고 부탁드려요 : )

[no1msh]

부나~ 코드 잘봤습니다. 허허
테스트 커버리지가 잔뜩 높아진게 보여 인상깊었습니다.

레아가 이미 많이 봐주시기도 하셔서
일단 레아대신 피드백 반영 확인해보았고,
추가로 제가 궁금한 점이나 생각해보시면 좋을 것들 조그마하게 적어보았습니다.

코멘트 또는 코드 수정 하셧다면 확인 후 바로 어프로브 누르도록 하겠습니다!

[no1msh]

coEvery는 every와 무엇이 다른가요?

[tmdgh1592]

coEvery는 이름 앞에 co가 붙은 것처럼 코루틴(suspend) 함수를 스터빙하기 위한 용도로 사용됩니다.
suspend 함수에 every를 사용하면 컴파일 에러가 발생하는 것을 확인할 수 있을 거예요 : )

[no1msh]

세심한 상수화 😎

[no1msh]

Suggested change

	fun List<CartProductEntity>.toDomain(): List<CartProduct> {
	return map {
	CartProduct(
	product = Product(
	name = it.name,
	price = it.price,
	imageUrl = it.imageUrl,
	),
	id = it.id,
	createdAt = it.createdAt,
	)
	}
	}
	fun List<CartProductEntity>.toDomain(): List<CartProduct> {
	return map { cartProductEntity ->
	CartProduct(
	product = Product(
	name = cartProductEntity.name,
	price = cartProductEntity.price,
	imageUrl = cartProductEntity.imageUrl,
	),
	id = cartProductEntity.id,
	createdAt = cartProductEntity.createdAt,
	)
	}
	}

---

조금만 봐도 it이 뭔지 유추할 수는 있지만, 지금과 같이 여러 인덴트가 들어간다면 더나은 가독성을 위해 cartProductEntity와 같은 explicit parameter를 사용하는건 어떨까요? 물론 큰차이가 느껴지지 않는다고 부나가 느끼신다면 무시하셔도 좋습니다.

[tmdgh1592]

좋은 의견이라고 생각해요~
코드를 보는데 유심히 들여다볼 정도로 복잡하지는 않지만, 명시적으로 작성해주면 좋겠다고 생각했어요.

참고로 저는 사실 대부분의 경우 아래와 같은 기준으로 explict paramter를 사용합니다.

• 하나의 파일에 코드가 길어져서 가독성이 떨어질 때
• 한 함수 내에서 depth가 길어질 때

[no1msh]

뷰모델은 레포지토리를 통해 그저 데이터를 관리하는데에만 관심사가 있습니다.

다르게 말하면 우리가 레포지토리 패턴을 사용한다는 가정하에 레포지토리의 데이터 소스로 Local이 오냐 Remote가 오냐는 레포지토리 내부 사정이라고 생각하는데 부나의 어노테이션은 데이터베이스, 즉 데이터 소스의 성격을 표시하고 있습니다.

부나가 생각하시기에 어떤가요?? 상관없을 것 같다면 반영하시지 않으셔도 좋습니다. 다만 부나의 의견은 궁금하니 코멘트 남겨주세요!

[tmdgh1592]

저도 반달과 동일한 생각이예요!
하지만 이 외에는 별다른 아이디어가 떠오르지 않아 위와 같은 구조로 설계하였습니다.

제가 아는 선에서 사실 Hilt도 위와 같은 형식으로 애노테이션을 지정해주고 있는 것으로 알고 있어요!
저도 이 부분에 영향을 많이 받아서 위처럼 작성하였습니다. 😅

참고 자료를 위해 링크를 함께 첨부해드려요 : )
Hilt 식별자

[no1msh]

의견 잘보았습니다. 부나!
4단계도 똑같이 홧팅입니다!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!