-
Notifications
You must be signed in to change notification settings - Fork 8
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
[AN/USER] 유저 어플 클린아키텍쳐 적용 (#650) #651
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다!! 코멘트 확인해 주세요 👍
android/festago/app/build.gradle.kts
Outdated
implementation(project(":common")) | ||
implementation(project(":data")) | ||
implementation(project(":domain")) | ||
|
||
// android | ||
implementation("androidx.core:core-ktx:1.10.1") | ||
implementation("androidx.appcompat:appcompat:1.6.1") | ||
implementation("com.google.android.material:material:1.9.0") | ||
implementation("androidx.constraintlayout:constraintlayout:2.1.4") | ||
implementation(project(":presentation")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app -> common
, app -> domain
의존성이 왜 필요한지 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금은 직접적인 의존은 하고 있지 않지만 그래프와 같이 의존성을 구상하고 설계 했다는 것을 보여주려고 추가를 하였습니다.
현재는 의존하고 있지 않으니 가지치기 해두겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
common 모듈에 어떤 코드가 위치하게 되는건지 잘 이해하지 못했어요! analytics 로깅 관련 코드가 이곳으로 옮겨진 것 같은데 현재 presentation 레이어에서만 로그를 남기고 있으니 presentation에서만 필요한 코드 아닌가요??
app -> common
, data -> common
의존이 왜 필요한지 궁금!
이후에 data나 app에서 common 모듈을 사용하게 되는걸까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
�현재 analytics는 presentation만 사용되고 있지만 팀내에서 모든 레이어에서 로깅하기로 했기 때문에 모든 모듈에서 접근 가능한 모듈을 작성을 해야 했습니다!
현재 app, domain, data, presentation 모듈 모두 접근 가능한 모듈에 배치하시 위해서는 domain 레이어가 가장 적합하다고 판단이 되었지만 analytics는 안드로이드 의존성을 가지고 있습니다. 그래서 저희는 domain 레이어는 코틀린 라이브러리로 두기로 하여서 별도의 모듈이 필요하다고 생각하였습니다.
dependencies { | ||
// hilt | ||
implementation("com.google.dagger:hilt-android:2.44") | ||
testImplementation(project(":data")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testImplementation 제거해도될듯합니다~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
common 모듈 내 di 패키지로 옮기는건 어떤가요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
페스타고 코드 리뷰를 오랜만에 해서 어색하네요,,모듈 분리 고생많으셨습니다!
별거없지만 궁금한점 코멘트로 남겼습니다 확인해주세요!
-
Presentation, Data 가 공통적으로 사용하는 것을 둘 다 아는 어딘가에 위치시켜야 하는데 Domain 은 안드로이드 의존성이 없어서 Common 모듈을 만드셨군요? 이해했습니다.
-
splash 화면에서 applicationContext 로 카카오 sdk 초기화해도 문제 없을 것으로 예상됩니다. 좋아요!
-
Data 레이어도 분리할 수 있겠지만 안드로이드 의존성이 있게 두는게 좋다고 생각들어요.
-
app → Common, app → domain 없애자는 해시의 의견에도 동의합니다.
import org.junit.Rule | ||
import org.junit.Test | ||
import java.time.LocalDateTime | ||
import java.time.format.DateTimeFormatter | ||
|
||
@HiltAndroidTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UI 테스트에 이 어노테이션을 추가해주어야 정상적으로 동작하나요?
그 이유도 궁금하네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hilt Activity must be attached to an @HiltAndroidApp Application. Did you forget to specify your Application's class name in your manifest's <application />'s android:name attribute?
다음과 같은 에러가 발생해서 대처 방안으로 작성 하였는데 이부분은 깊게 알고 있지 않아서 좀 더찾아보고 코멘트 달겠습니다!
com.festago.festago.presentation.fcm.NotificationManager( | ||
this | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
한 줄에 작성이 안되는 걸까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
com.festago.festago.presentation.fcm.NotificationManager( | |
this | |
) | |
NotificationManager(this) |
파일 이동시 다음과 같이 변환되는 문제들이 있어서 모두 수정했다고 생각했는데 짜잘한게 남았네요 반영하겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
리뷰 반영 확인했습니다! 커밋 단위가 작업 별로 분리되어있고 메세지가 직관적이라 바로 이해할 수 있었어요! 고생많았습니다ㅎㅎ
📌 관련 이슈
✨ PR 세부 내용
모듈 분리
Common 모듈
App 모듈의 Kakao
Data의 안드로이드 라이브러리
경로