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

네트워크 모델 변환 함수 생성 #312

Closed
wants to merge 12 commits into from

Conversation

plgafhd
Copy link
Collaborator

@plgafhd plgafhd commented Jul 7, 2024

변환 함수를 생성하는 과정에서
예상했던 것보다 나중에 Revisit 해야 할 부분이 많아서,
네트워크 모델 변환 함수 생성/데이터베이스 모델 변환 함수 생성/확장 함수 생성 세 PR로 나누게 되었다.

이 PR에서는

  • 일단 core:model에 생성한 모델을 기반으로 변환 함수를 만든다.
  • 이전에 모듈을 옮기는 과정에서 (옮겨야 하는데 옮기지 않은/안 옮겨도 되는데 옮긴) 모델들을 수정한다.

@plgafhd plgafhd requested a review from a team as a code owner July 7, 2024 16:54
@plgafhd plgafhd changed the base branch from refactor/multi-module-base-branch to eastshine2741/create-model July 8, 2024 12:10
Comment on lines -14 to 17
// temp test
implementation(project(":core:network"))
implementation(project(":core:database"))
implementation(project(":core:model")) // TODO : revisit
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이게 implementation이면 생기는 문제점

core:model <- core:network <- core:data
NIA에서도 그렇고 우리도 다 완성되면 이런 의존성이 생긴다.
그러면, core:data는 core:network에 의존하는 것 만으로도 core:model의 data class나 함수들을 가져올 수 있다.
그런데, api가 아니라 implementation으로 가져오면 그렇게 못한다.
지금 해놓은 것처럼 core:model도 명시적으로 가져와야 한다.
나중에 필요한 종속 구성에 맞춰 바꿔야 한다.

Copy link
Collaborator Author

@plgafhd plgafhd Jul 9, 2024

Choose a reason for hiding this comment

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

실제로 찾아보니까 (Developers 문서)
api와 implementation의 차이가 딱 저거로 구분되는 듯 한데...
설명만 봤을 때는 api를 썼을 때의 장점은 딱히 없어보인다. (빌드 시간의 관점에서)
장점이 있다면 프로그램을 짤 때 있는 장점 정도..? (위 코멘트가 implementation의 문제점 = api의 장점)
그럼에도 NIA는 위 코멘트 같은 경우 뿐만 아니라 모듈간 종속에 있어서 거의 api를 쓰고 있다.
근데 그렇게 하면 '빌드 시간 개선'이라는 처음 목적에 어긋나지는 않나 싶기도 한다..

Copy link
Collaborator

@eastshine2741 eastshine2741 Jul 9, 2024

Choose a reason for hiding this comment

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

아하 빌드속도가 다르구나..

일단 빌드 시간이 늘어나진 않을듯? core:model같은건 결국 feature에까지 전달되어야 하는 클래스들을 가지니, build.gradle.kts(:core:network)에서 api(projects.core.model)하지 않더라도 온갖 모듈에서 implementation(projects.core.model)을 해줘야 할 거고, 그럼 컴파일되는 경우의 수는 동일할 것 같아서

api를 썼을 때의 장점은 딱히 없어보인다

내가 생각한 장점은 이래

  • 여러 곳에서 implementation 쓰는 것보다 의존성 그래프가 단순해진다
  • 중복코드(implementation)를 줄일 수 있다

Copy link
Collaborator

@eastshine2741 eastshine2741 Jul 9, 2024

Choose a reason for hiding this comment

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

NIA는 위 코멘트 같은 경우 뿐만 아니라 모듈간 종속에 있어서 거의 api를 쓰고 있다.

NIA가 모듈 간 종속에 있어서 api를 쓰는 경우도 있는데, core 끼리만 적당히 쓰는 듯..? feature 모듈에서 다른 모듈에 의존할 때에는 무조건 implementation만 씀. core:model처럼 온갖 곳에서 쓰이는 모듈은 확실히 api로 의존되는 경우가 많고, core:designsystem같은건 implementation으로 의존되는 경우가 많네.

'빌드 시간 개선'이라는 처음 목적에 어긋나지는 않나 싶기도 한다

core:model처럼 온갖 곳에서 쓰이는 모듈은, 수정 시 빌드 시간이 길어진다는 단점을 피할 수 없는 것 같기도..

Copy link
Collaborator

Choose a reason for hiding this comment

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

core:model <- core:network 인데, core:network 에서 외부에 공개하는 부분(예: API의 응답 타입)에 core:model 이 반드시 포함되고 있으니까 api 를 쓰지 않으면 안 될 거야

Copy link
Collaborator

Choose a reason for hiding this comment

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

core:network 에서 core:model 을 api 로 들고 있지 않고 implementation 으로만 들고 있으면, core:network 를 참조하는 다른 모듈에서 core:network랑 core:model 을 둘 다 종속성으로 줘야 하는데, 이러면 굳이 다른 모듈에 core:model의 종속성을 명시해 줘야 하니까 불필요하지

Copy link
Collaborator

Choose a reason for hiding this comment

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

NIA를 기준으로 따라가자

Comment on lines 14 to 16
buildingNameEng = this.buildingNameEng ?: "",
coordinate = this.locationInDMS.toExternalModel(), // TODO : locationInDMS랑 locationinDecicmal이 내려오고 있다..
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이거 locationInDMS랑 locationInDecimal 중 DMS 쓰는게 맞는건가?
(예를 들어 locationInDMS = (37.46073, 126.95452)면 locationInDecimal = (489941.0, 1100367.0))
뒤에거가 어디 쓰이냐에 따라 둘다 필요할 수도 있을 듯 단순히 비율 차이는 아닌 듯 해서

Copy link
Collaborator Author

@plgafhd plgafhd Jul 11, 2024

Choose a reason for hiding this comment

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

현재 locationInDMS만 쓰고 있으므로,
그대로 두고 주석만 지우면 될 듯

Comment on lines 5 to 10

fun ColorDto.toExternalModel() = LectureColor(
foregroundString = this.fgRaw ?: "#000000",
backgroundString = this.bgRaw ?: "#FFFFFF",
)
// TODO : 검은 바탕에 흰 글씨를 default로 해놨는데, 추후 revisit
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ColorDto의 도메인 모델인 LectureColor를 non-nullable로 하는 과정에서 디폴트 값이 필요했는데,
일단 그 값을 검은 바탕에 흰 글씨로 해놓음

Comment on lines +5 to +13

// TODO : 논의 중, RemoteConfig라는 model 자체가 필요 없다는 얘기도 있었다.
fun RemoteConfigDto.toExternalModel() = RemoteConfig(
friendsBundleSrc = reactNativeBundleSrc?.src?.get("android"),
vacancyNotificationBannerEnabled = vacancyBannerConfig.visible,
sugangSNUUrl = vacancyUrlConfig.url,
settingPageNewBadgeTitles = settingsBadgeConfig.new,
disableMapFeature = disableMapFeature ?: false,
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

RemoteConfig라는 model 자체가 필요 없다는 얘기가 있었던 이유
RemoteConfig는 그 내부에서 값을 처리해서 기본 타입 (String 이런거)의 값만 외부로 주기 때문에,
똑같이 기본 타입의 값만 외부로 주는 레포지토리를 생각해보면
레포지토리는 네트워크 모델을 쓸 수 있기 때문..

Copy link
Collaborator

Choose a reason for hiding this comment

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

첫 번째 RemoteConfig는 새로 만드는 model 말한거고
두 번째 RemoteConfig는 지금 있는 리모트컨피그 받아오고 가공하는 클래스 말한거지?ㅋㅋㅋㅋ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 그러네 맞아 ㅋㅋㅋㅋㅋ

@plgafhd
Copy link
Collaborator Author

plgafhd commented Jul 8, 2024

TagDto 관련

현재 tag 목록을 가져오는 과정 (내가 이해한게 맞다면...)

  • 시간표를 선택했을 때 만약 학기가 바뀌면 그 학기에 맞게 tag 목록을 가져와야 한다.
  • 이때 GetTagListResults 응답이 내려온다. 여기에는 태그의 type별로 (ex: 학년, 분류, 학점..) 선택할 수 있는 tag name들이 List로 내려온다.
  • 그러면 이거를 LectureSearchRepository에서 하나씩 읽어서 TagDto (type, name) 형태로 List를 만든다.

그래서 core:network의 model에는 TagDto가 있는게 아니라, GetTagListResults가 있다. 그리고 core:model에 Tag라는 data class를 넣을 예정이다. 그러면, GetTagListResults -> Tag 변환 함수는 필요 없는거 아닐까..?

Comment on lines 4 to 6

abstract class OriginalLecture(
data class OriginalLecture(
override val id: String,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

얘랑 TimetableLecture abstract로 되어 있길래 바꿨어

Comment on lines 14 to 19

fun LectureDto.toExternalModel(): Lecture {
return when (this.color == ColorDto()) { // TODO : revisit (true이면 OriginalLecture, false이면 TimeTableLecture 반환)
true -> OriginalLecture(
id = this.id, // TODO : 모지 이거 id랑 lecture_id 중에 뭐가 필요한거지..?
classification = this.classification,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이 파일에서 revisit 하고 싶은 점

  1. 어떤 도메인 모델로 바꿀지 (OriginalLecture, TimetableLecture) 결정하는 로직이 저게 괜찮은가
  2. id와 lecture_id를 이해 못해서 일단 type이 맞는대로 끼워넣어 놓음..
  3. 더 뭔가(ui, viewmodel, repository 등)를 만들기 전에 잘 작동하는지 테스트 할 수 있으면 좋을 듯

Copy link
Collaborator

Choose a reason for hiding this comment

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

나도 헷갈려서 전에 딱 정리해둠
https://wafflestudio.slack.com/archives/C0PAVPS5T/p1718805215296379?thread_ts=1718796714.629209&cid=C0PAVPS5T

어떤 API의 응답이냐에 따라 원본 강의 id가 다른 필드에 내려와

  • POST /v1/search_query에서는 _id
  • GET /v1/tables/{tableId}, GET /v1/tables/recent에서는 lecture_id

POST /v1/search_query에서는 lecture_id 자체가 내려오지 않음. 강의 id라는 게 2개씩이나 필요하지 않은 상황이니까.

즉, lecture_id가 null이라면 OriginalLecture라고 할 수 있다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Comment on lines +66 to +68
5 -> Day.SATURDAY
else -> Day.SUNDAY
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이것도 enum Day에 혹시 다른 값이 내려올 때를 대비해서 Day.Default 이런거 추가할까?

Copy link
Collaborator

Choose a reason for hiding this comment

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

음 난 없어도 된다고 생각해
Notification type같은 건 나중에 다른 값이 추가될 여지가 충분한데, 이건 요일이다보니 다른 값이 추가되진 않을거고
오히려 안 쓰이는 케이스가 하나 더 생김으로써 불필요한 예외처리가 많아질 수 있을 듯..?

예를 들어 요일피커같은거에서 when(Day형 변수) 문을 쓴다고 하면, Day.Default에 대한 동작을 정의하지 않으면 컴파일 에러날거야. when 문은 모든 케이스에 대해 처리해야 하기 때문. 이 때 Day.Default에 대한 동작을 정하기가 애매하단거지

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

음 그러네 쓰는 곳에서 애매하겠구나

Comment on lines 76 to 81
),
place = Place(
name = this.place,
building = null
)
)
Copy link
Collaborator Author

@plgafhd plgafhd Jul 8, 2024

Choose a reason for hiding this comment

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

이 부분에서 의문이 생겼는데, (슬랙)
만약 이렇게 하게 된다면
나중에 강의 상세 들어갈 때 v1/building를 쏘고, GetBuildingsResponse가 내려올텐데 Building.kt에 있는 LectureBuildingDto.toExternalModel()를 이용할 수 있다.
그렇게 해서 Building을 얻어서 저 place의 building 자리를 채울 수 있다.

@plgafhd
Copy link
Collaborator Author

plgafhd commented Jul 8, 2024

우리 TableTheme/BuiltInTheme/CustomTheme 얘기했던게
변환 함수가 필요 없고 network model도 필요 없는거 맞지..?

Comment on lines 16 to 18
totalCredit = this.totalCredit ?: 0L,
theme = // TODO : 이 부분에서 문제 발생
isPrimary = this.isPrimary,
Copy link
Collaborator Author

@plgafhd plgafhd Jul 8, 2024

Choose a reason for hiding this comment

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

이거 논의할때 theme으로 색깔 얻는거 ui에서 그릴거니까 거기로 옮기자고 했는데 (context가 무진장 많이 필요)
그렇게 하니까 TimeTable의 변환 함수를 못만드네... 이거 어떻게 하지

지금 theme 자리에 BuiltInTheme/CustomTheme 둘 중 하나가 와야 하고 만약 CustomTheme인 경우... 어떻게 하지?
일단 생각한 방법은 CustomTheme에서 colors라는 필드를 아예 없애기?
어차피 ui에서 바꿀거면 필요 없기도 하고

Copy link
Collaborator

Choose a reason for hiding this comment

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

으아아악 CustomTheme이라는 모델에 colors는 무조건 있어야 자연스러울거같은데

theme 관련된건 내가 커스텀테마 feature모듈 만들면서 또는 그 전에 리비짓할게...
일단 제공테마만 생각한다고 하면 theme: Int로 두고 TableDto.theme 그대로 가져오면 될거야

Copy link
Collaborator

Choose a reason for hiding this comment

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

이거 내가 해서 푸시해둘게

Copy link
Collaborator

Choose a reason for hiding this comment

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

푸시했어
c2226c7

@plgafhd plgafhd changed the title :core:data 변환 함수 생성 네트워크 모델 변환 함수 생성 Jul 9, 2024
@plgafhd
Copy link
Collaborator Author

plgafhd commented Jul 9, 2024

추가 의문: 데이터베이스 모델 <-> 도메인 모델 변환 함수는 필요한건가..?

Comment on lines 8 to 18
fun TableDto.toExternalModel() = TimeTable(
id = this.id,
courseBook = CourseBook(
semester = this.semester,
year = this.year,
),
title = this.title,
lectureList = this.lectureList.map { it.toTimetableLecture() },
totalCredit = this.totalCredit ?: 0L,
theme = this.theme
isPrimary = this.isPrimary,
Copy link
Collaborator

@eastshine2741 eastshine2741 Jul 9, 2024

Choose a reason for hiding this comment

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

확장함수들에 this 없으면 더 깔끔할것같아
컴파일러가 다 알아서 this 있는걸로 생각해주니깐

Copy link
Collaborator

Choose a reason for hiding this comment

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

어 근데 이제보니 NIA는 toExternalModel이 아니라 asExternalModel이네ㅋㅋㅋ 그게그거겠지머

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 this 안쓰는게 오히려 좋은건가 쓰는게 좋을거일줄 알았어.. ㅋㅋㅋㅋ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

어 근데 이제보니 NIA는 toExternalModel이 아니라 asExternalModel이네ㅋㅋㅋ 그게그거겠지머

헐 ㅋㅋㅋㅋㅋ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

832104d
this 제거

@eastshine2741
Copy link
Collaborator

eastshine2741 commented Jul 9, 2024

우리 TableTheme/BuiltInTheme/CustomTheme 얘기했던게
변환 함수가 필요 없고 network model도 필요 없는거 맞지..?

엉 변환함수는 ThemeDto에 메서드로 들어있어서 그것만 data 모듈로 옮겨놨어 f4fa74d

pull 땡겨줘

@eastshine2741
Copy link
Collaborator

eastshine2741 commented Jul 9, 2024

추가 의문: 데이터베이스 모델 <-> 도메인 모델 변환 함수는 필요한건가..?

필요하지? 데이터베이스는 다른 계층이고, 두 모델은 다른 모델이니까
우리가 도메인 모델 설계한거는 우리가 개발할 때 편하도록 설계한거고, DB나 shared preference, datastore 등에 저장할 때에 편한 형식은 또 다를 수 있으니 변환 함수가 있어야할거야

NIA도 보면 asEntity() 이런 변환함수들 있네

@eastshine2741
Copy link
Collaborator

GetTagListResults -> Tag 변환 함수는 필요 없는거 아닐까..?

나 이해를 잘 못했는데ㅋㅋㅋㅋ... TagDto -> Tag 가 필요없고 GetTagListResults -> Tag가 필요하다는 뜻이야?

@plgafhd
Copy link
Collaborator Author

plgafhd commented Jul 9, 2024

데이터베이스 모델 <-> 도메인 모델 변환 함수
이거 나도 거의 필요하다고 생각하긴 했는데,
우리가 쓰는 database는 사실 NIA의 datastore인데 이름을 잘못 붙였던거에 가깝고,
NIA에서 datastore 쓸 때는 딱히 변환 함수가 없는 것 같아서 의문이 잠깐 생겼었어

@plgafhd
Copy link
Collaborator Author

plgafhd commented Jul 9, 2024

나 이해를 잘 못했는데ㅋㅋㅋㅋ... TagDto -> Tag 가 필요없고 GetTagListResults -> Tag가 필요하다는 뜻이야?

내 생각대로면 GetTagListResults -> Tag도 필요 없을 것 같아

  1. 지금 저 변환 과정이 레포지토리에서 이루어진다.
  2. 지금 구조대로라면, GetTagListResults -> List<Tag>는 가능하지만, GetTagListResults -> Tag는 어차피 불가능하다.

이런 이유로..? 모델을 따로 두는 것까지는 의미가 있지만
변환 함수를 두는 것에는 의미가 없을 것 같아서

@eastshine2741
Copy link
Collaborator

eastshine2741 commented Jul 10, 2024

GetTagListResults -> List는 가능하지만, GetTagListResults -> Tag는 어차피 불가능하다.

아아아 이해했어 흠 그러게 TagDto라는게 따로 없어서 레포지토리의 메서드에서 List만 리턴하면 되는구나
변환함수 따로 없어도될듯??

@plgafhd
Copy link
Collaborator Author

plgafhd commented Jul 11, 2024

546cfab
논의 결과에 따라 수정

  1. Place라는, String과 Building을 묶는 data class를 아예 없앤다.
  2. PlaceTime은 Place 타입의 place가 아닌 String 타입의 placeName을 갖는다.

@eastshine2741
Copy link
Collaborator

eastshine2741 commented Jul 12, 2024

굿굿 내일 다같이 다시 보고 머지하자 수고했당

placeName = place
)

// TODO : 제대로 되는지 테스트 해보면 좋을 듯
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 테스트는 어떤 테스트 얘기한거야? 제대로 변환되는지 테스트?

간단한거면 여기다 fun main() {} 만들고 돌려볼 수 있어

…plete-mappers

# Conflicts:
#	core/model/src/main/java/com/wafflestudio/snutt2/core/model/data/lecture/OriginalLecture.kt
Copy link
Collaborator

@JuTaK97 JuTaK97 left a comment

Choose a reason for hiding this comment

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

궁금한점) 네트워크 모델 변환 함수는 core:network 에 있어야 하는 거 아닌가? 그래서 이름도 toExternalModel 인거고.

@plgafhd
Copy link
Collaborator Author

plgafhd commented Jul 16, 2024

나도 그거 찾아봤었는데

NIA 기준으로
네트워크 모델 (core:network) -> 데이터베이스 모델 (core:database) 변환함수는 core:data에
데이터베이스 모델 (core:database) -> 도메인 모델 (core:model) 변환함수는 core:data 또는 core:database에
이렇게 있더라구
그래서 오잉..? 기준이 뭐지? 싶었는데 그냥 core:data에 놓는게 기본이고, 데이터베이스는 편하게 같이 놓은 느낌? 정도로 생각했어
data가 repository를 갖고 있다 + network/model/database에 다 의존한다 라는 특성 때문에
중간 다리 역할로 적합하다고 생각하기도 했고
위치가 큰 상관이 없는 느낌도 약간 있기는 해

@JuTaK97
Copy link
Collaborator

JuTaK97 commented Jul 16, 2024

나도 그거 찾아봤었는데

NIA 기준으로 네트워크 모델 (core:network) -> 데이터베이스 모델 (core:database) 변환함수는 core:data에 데이터베이스 모델 (core:database) -> 도메인 모델 (core:model) 변환함수는 core:data 또는 core:database에 이렇게 있더라구 그래서 오잉..? 기준이 뭐지? 싶었는데 그냥 core:data에 놓는게 기본이고, 데이터베이스는 편하게 같이 놓은 느낌? 정도로 생각했어 data가 repository를 갖고 있다 + network/model/database에 다 의존한다 라는 특성 때문에 중간 다리 역할로 적합하다고 생각하기도 했고 위치가 큰 상관이 없는 느낌도 약간 있기는 해

우리는, core:network 의 API들은 네트워크 모델을 반환하고 core:database 는 데이터베이스 모델을 반환하는 걸로 가 보자
core:network 는 네트워크 통신에 집중하고, 번역은 사용하는 쪽에서 하는 게 좋을 것 같기 때문

@JuTaK97
Copy link
Collaborator

JuTaK97 commented Jul 16, 2024

그런데 우리 core:model 아직 하나도 안 만든거 아니야?
예전에 얘기했던 건, 우선 현재 app에 있는 네트워크랑 저장소를 core:network, core:database 로 교체하기 위해서, app에 있는 레포지토리들(추후 core:data로 분리될 부분)에서 네트워크 모델이랑 데이터베이스 모델을 app에 있는 구 모델들로 매핑하는 함수를 임시로 만들기로 했던 걸로 기억하는데

@plgafhd plgafhd closed this Jul 20, 2024
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.

None yet

3 participants