Skip to content

🚀 4단계 - GitHub(인기 저장소) #79

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

Open
wants to merge 8 commits into
base: hyemdooly
Choose a base branch
from

Conversation

hyemdooly
Copy link

혁님 안녕하세요~! 3단계 리뷰 반영하여 4단계 미션 구현하였습니다!
선택사항도 함께 구현하였는데요.

(선택사항) domain 패키지를 만들어 비즈니스 로직을 캡슐화한다.
저장소의 Star 개수가 50개 이상인지 판단하는 로직도 도메인 레이어에 포함될지 스스로 판단하고 PR 설명에 적는다.

처음에는 50개 이상이면 HOT을 보여준다는 것이 뷰의 관심사이기에, 뷰에서 판단하면 된다고 생각했습니다. 그래서 처음에는 뷰의 로직으로 작성했는데요.
만약 도메인 계층을 가지고 다른 플랫폼으로 옮겼을 때도 동일한 결과를 보장해야하지 않을까, 뷰는 최대한 멍청해야하지 않을까? 생각이 들어서 도메인 계층으로 생각을 바꾸어 리팩터링 하였습니다.
이 부분에 대한 혁님의 생각도 궁금합니다. 😄

Copy link
Member

@malibinYun malibinYun left a comment

Choose a reason for hiding this comment

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

4단계 미션 고생많으셨어요~
제가 너무 늦었지요.. 심심한 사과의 말씀 올립니다.
미션을 잘 구현해주셔서 경험담 위주로 코멘트를 남겼어요.
더 시도하고 싶으신 게 있으시다면 반영 후 리뷰 요청을, 이만 마무리 짓고 싶으시다면 메시지 남겨주세요 :)

Comment on lines +35 to +36
return flow {
val repositories = getRepositoryUseCase(ORGANIZATION)
Copy link
Member

Choose a reason for hiding this comment

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

검색 결과를 유동적으로 변경해야할때 변경점이 커질 거라 예상돼요.
첫 값은 savedstatehandle을 활용한다던지, 혹은 다른 검색 결과를 받을 수 있도록 확장할 수 있도록 구성해보는 건 어떨까요?

Comment on lines +45 to +48
RepositoryListEvent.ShowSnackBar(
msgRes = R.string.repository_list_fetch_error,
actionLabelRes = R.string.retry
)
Copy link
Member

Choose a reason for hiding this comment

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

Viewmodel은 안드로이드 프레임워크에 조금은 자유로워서 유닛테스트가 가능한데요,
R값이 있는 순간부터 다른 테스트 도구의 도움 없이는 유닛테스트를 하기가 어려워져요.
그래서 저는 최대한 유닛 테스트를 어렵게 하는 요소를 뷰모델에 집어넣지 않으려고 해요.

Comment on lines +7 to +17
fun interface GetRepositoriesUseCase {
suspend operator fun invoke(organization: String): List<Repository>
}

class DefaultGetRepositoriesUseCase(
private val githubRepoRepository: GithubRepoRepository,
) : GetRepositoriesUseCase {
override suspend operator fun invoke(organization: String): List<Repository> {
return githubRepoRepository.getRepositories(organization).map { it.toDomain() }
}
}
Copy link
Member

Choose a reason for hiding this comment

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

UseCase의 인터페이스라.... 옛날부터 생각해봤던 것이긴 한데 정말 보일러플레이트가 많은 코드라 저는 안하는 게 낫겠다 라고 결론지었는데요,
이 방법을 택한 이유가 있으셨나요?

Copy link
Member

Choose a reason for hiding this comment

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

그러고보니 .toDomain을 보니,
도메인은 GithubRepoRepository를 통과해서 반환될 때부터 도메인이어야 자연스럽단 생각이 드네요~

Comment on lines +11 to +13
) {
val isHot: Boolean = stars >= HOT_STANDARD

Copy link
Member

Choose a reason for hiding this comment

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

처음에는 50개 이상이면 HOT을 보여준다는 것이 뷰의 관심사이기에, 뷰에서 판단하면 된다고 생각했습니다. 그래서 처음에는 뷰의 로직으로 작성했는데요.
만약 도메인 계층을 가지고 다른 플랫폼으로 옮겼을 때도 동일한 결과를 보장해야하지 않을까, 뷰는 최대한 멍청해야하지 않을까? 생각이 들어서 도메인 계층으로 생각을 바꾸어 리팩터링 하였습니다.
이 부분에 대한 혁님의 생각도 궁금합니다. 😄

저도 참 고민이 많았는데요,
저도 똑같은 생각의 흐름으로 코드를 작성했어요.

뷰에 보여주는 것이라는 것은 맞으나, HOT이라는 딱지를 붙여주는 기획은 결국 도메인이라고 생각해요.
뷰의 요소는 HOT을 어디에 어떻게 보여줄 것이냐의 로직이란 생각이 들어 저도 도메인에 isHot을 놓는게 적절하다 생각했어요.

백엔드에서 HOT을 뷰의 요소로 볼 것이냐? 라고 했을때, 저는 아니라고 생각했어요.
오히려 HOT인 조건을 백엔드에 박아 넣는게, 핫픽스나 다른 배포로 하여금 더 유동적으로 대처할 수 있는 값이라 생각이 들어서요.
백엔드가 연결된다면, isHot도 응답으로 보내주지 않을까 하는 개인적인 생각이 있었어요 :)

Comment on lines 30 to +48
fun RepositoryItem(
repository: RepositoryEntity,
repository: Repository,
modifier: Modifier = Modifier
) {
RepositoryItem(
fullName = repository.fullName,
description = repository.description,
stars = repository.stars,
isHot = repository.isHot,
modifier = modifier
)
}

@Composable
fun RepositoryItem(
fullName: String,
description: String,
stars: Int,
isHot: Boolean,
Copy link
Member

Choose a reason for hiding this comment

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

오~ 저도 컴포즈 컴포넌트 구성할 때 이런 방식으로 구성하는데, 팁을 드리기도전에 이렇게 구성해주셨네요

원형 컴포넌트는 최대한 원시값들을 활용하고, 도메인이 들어가게 편의성을 제공하는 컴포넌트를 따로 구성하는 형태를 전 선호해요.
이러면 재활용할 때 정말 유용하더라구요.

Comment on lines +69 to +70
}
Spacer(modifier = Modifier.weight(1f))
Copy link
Member

Choose a reason for hiding this comment

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

VerticalDivider, HorizontalDivider 이런건 만들어주면서 VerticalSpacer 이런건 안만들어주더라구요
전 회사에서 VerticalSpacer, HorizontalSpacer 만들어서 사용하는데, Modifier 매번 안만들어서 정말 편해요. 나중에 만들어서 잡솨보세요~

ex)

~~~
VerticalSpacer(height = 8.dp)

음 다 쓰고 보니 이건 weight 걸려있네요
이것도 만들면 좋겠는데요? WeightedSpacer 같은걸 필요할때 써먹어야겠군요

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