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

[스캇] 1단계 자동 DI 미션 제출합니다 #10

Merged
merged 9 commits into from
Sep 5, 2023

Conversation

chws0508
Copy link

@chws0508 chws0508 commented Sep 4, 2023

안녕하세요 링링 ^^ 스캇입니다.
이번에 링링이 저의 리뷰어가 되어주셔서 너무 영광입니다 :)

이번 자동 DI 미션은 Koin 구조를 한번 따라해보려고 하였습니다.
Koin 은 중앙 레지스트리 방식을 이용하고, KClass 타입을 Key로, 의존성 주입할 Instance 를 Value 로 하는 Map 형태로 의존성 주입 객체들을 저장하는 형식입니다.

자동 DI 커밋

수동 DI 커밋

테스트 커밋

요구사항

기능 요구 사항

생성자 주입 - 수동

  • 테스트하기 어렵다.
  • Repository 객체를 교체하기 위해 또다른 객체를 만들어 바꿔줘야 한다. 즉, ViewModel에 직접적인 변경사항이 발생한다.

생성자 주입 - 자동

  • ViewModel에서 참조하는 Repository가 정상적으로 주입되지 않는다.
  • Repository를 참조하는 다른 객체가 생기면 주입 코드를 매번 만들어줘야 한다.
    • ViewModel에 수동으로 주입되고 있는 의존성들을 자동으로 주입되도록 바꿔본다.
    • 특정 ViewModel에서만이 아닌, 범용적으로 활용될 수 있는 자동 주입 로직을 작성한다. (MainViewModel, CartViewModel 모두 하나의 로직만 참조한다)
    • 100개의 ViewModel이 생긴다고 가정했을 때, 자동 주입 로직 100개가 생기는 것이 아니다. 하나의 자동 주입 로직을 재사용할 수 있어야 한다.
  • 장바구니에 접근할 때마다 매번 CartRepository 인스턴스를 새로 만들고 있다.
    • 여러 번 인스턴스화할 필요 없는 객체는 최초 한 번만 인스턴스화한다. (이 단계에서는 너무 깊게 생각하지 말고 싱글 오브젝트로 구현해도 된다.)

선택 요구 사항

  • TDD로 DI 구현
  • Robolectric으로 기능 테스트
  • ViewModel 테스트
  • 모든 도메인 로직, Repository 단위 테스트

프로그래밍 요구 사항

  • 사전에 주어진 테스트 코드가 모두 성공해야 한다.
  • Annotation은 이 단계에서 활용하지 않는다.

@chws0508 chws0508 self-assigned this Sep 4, 2023
Copy link
Member

@RightHennessy RightHennessy left a comment

Choose a reason for hiding this comment

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

안녕하세요 ~ 스캇 🐛
스캇과 페어를 한게 엊그제 같은데.. 제가 스캇의 코드를 리뷰하다니 세월이 참 빠르네요 허허
코드를 잘 구현해서 제가 리뷰해줄 수 있는게 뭐가 있을까 ~ 싶지만 최선을 다해 리뷰해보았습니다 ㅎㅎ
코멘트 남긴 부분은 step2에서 반영 부탁드립니다 ~
image

Comment on lines 8 to 15
override fun onCreate() {
super.onCreate()
repositoryContainer = RepositoryContainer()
}

companion object {
lateinit var repositoryContainer: RepositoryContainer
}
Copy link
Member

Choose a reason for hiding this comment

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

companion object에서 바로 Repository Container를 생성하지 않고 onCreate에서 해준 이유가 궁금합니다.

Copy link
Author

Choose a reason for hiding this comment

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

Application 이 정상적으로 Oncreate 될 때, Repository Container 를 생성해준다 라는 명시하고 싶어서 이렇게 작성했습니다!


// TODO: Step2 - CartProductDao를 참조하도록 변경
class CartRepository {
class CartRepositoryImpl : CartRepository {
Copy link
Member

Choose a reason for hiding this comment

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

Impl이라는 키워드 대신 다른 키워드를 사용해보는 것은 어떤가요?
수업에서도 얘기가 나왔었지만, Impl은 다형성을 존중하지 못하는 느낌이 있다고 하더라고요 ~
참고로 공식문서에서는 기본 형태를 Default를 주로사용하는 것 같습니다 ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

저도 Defualt 문화에 동참해보겠습니다 ㅎㅎ

val cartRepository = ShoppingApplication.repositoryContainer.cartRepository

when {
isAssignableFrom(MainViewModel::class.java) ->
Copy link
Member

Choose a reason for hiding this comment

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

오.. isAssignableFrom이라는 함수.. 배워갑니다 (짱!)

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다~~

Comment on lines 33 to 34
val actual = cartRepository.getAllCartProducts()
val expected = listOf(Dummy.cartProducts[1])
Copy link
Member

Choose a reason for hiding this comment

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

개인적으로 이부분이 가독성이 떨어진다고 생각했는데요,
0번째 원소가 없어지면 1~마지막 일것이라고 생각했는데 cartProducts[1]이라서 다른 파일에서 가서 cartProducts가 무슨 리스트인지 확인을 해야했습니다.

확실히 파일을 분리한 스캇의 코드가 깔끔해 보이지만, 저는 가독성을 위해 테스트를 위한 dummy는 같은 테스트 파일 하단에 존재해야한다고 생각하는데 스캇의 생각은 어떠신가요?

Copy link
Author

@chws0508 chws0508 Sep 5, 2023

Choose a reason for hiding this comment

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

Dummy 파일의 세부 내용을 직접 확인해야하는 경우에는 확실히 링링의 말처럼 테스트 하단 파일에 존재하는 것이 맞다고 생각합니다! 하지만 Dummy 파일의 내용을 직접 확인하지 않아도 되고, 공통으로 쓰이는 데이터 같은 경우에는 파일을 따로 분리하여 공통으로 사용되게 하면 좋을 것 같네요!

그리고 다시 생각해보니, CartRepository 를 테스트하는 것인데, Dummy.cartProducts 를 이용하는 것도 잘못된 것 같네요!
수정한 파일 커밋입니다 !
커밋 링크

Comment on lines +49 to +52
inline fun <reified T : Any> get(): T {
val service = Injector.getService(T::class)
return service.instance as T
}
Copy link
Member

Choose a reason for hiding this comment

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

이건 어디 쓰이는 코드인거죠..?

Copy link
Author

Choose a reason for hiding this comment

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

아직은 쓰이지 않는 코드이지만, 나중에 Activity나 Fragment에서 최종적으로 의존성 주입을 할 때 사용할 코드입니다.

Copy link
Member

Choose a reason for hiding this comment

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

image

Copy link
Member

@RightHennessy RightHennessy left a comment

Choose a reason for hiding this comment

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

리뷰 완료했습니다 .
수고하셨습니다.
스텝 2에서 또 뵙겠습니다.

@RightHennessy
Copy link
Member

image
수고하셨습니다 ~
스텝2 하이팅 !

@RightHennessy RightHennessy merged commit 5ec0034 into woowacourse:chws0508 Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants