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

Feat/#21 앱잼 대비 코드 리팩토링 #22

Open
wants to merge 31 commits into
base: develop-compose
Choose a base branch
from

Conversation

boiledEgg-s
Copy link
Member

@boiledEgg-s boiledEgg-s commented Jun 30, 2024

📌𝘐𝘴𝘴𝘶𝘦𝘴

📎𝘞𝘰𝘳𝘬 𝘋𝘦𝘴𝘤𝘳𝘪𝘱𝘵𝘪𝘰𝘯

  • 힐트 적용
  • 클린 아키텍처 적용
  • 멀티모듈 적용

📷𝘚𝘤𝘳𝘦𝘦𝘯𝘴𝘩𝘰𝘵

appjam.mp4

💬𝘛𝘰 𝘙𝘦𝘷𝘪𝘦𝘸𝘦𝘳𝘴

7주차엔 한게 없는데 머지가 안되서 일단 제쳐두고 앱잼 리팩토링 한거 머지합니다!!
이것저것 시도하고 파일 구조도 많이 바꿔서 커밋에 신경을 못썼어요ㅠ 반성 중입니다,,

기존 서버가 내려가는 바람에 로그인 버튼 누르면 바로 넘어가고 사용자 정보엔 더미 데이터를 넣어놨어요.. 그래도 친구 목록 불러오는 서버 통신은 잘 됩니다!!!

아 그리고 core는 일단 빼뒀습니다,, 일단 여기까지만ㅎㅎ

Copy link
Member

@leeeyubin leeeyubin left a comment

Choose a reason for hiding this comment

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

멀티모듈까지 섭렵한 석준오빠,, 최고다!!! 이제 터닝 작업도 들어갑시당~!!

Comment on lines +24 to +28

@Binds
@Singleton
abstract fun provideFollowerDataSource(followerDataSourceImpl: FollowerDataSourceImpl): FollowerDataSource

Copy link
Member

Choose a reason for hiding this comment

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

어노테이션은 @Binds인데 함수명은 provide로 되어있네요!
bind로 바꾸면 더 좋을 것 가타요!!

Copy link
Member Author

Choose a reason for hiding this comment

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

오 그렇네요,, 이름 지을때 항상 신경써야겠어요,,

Comment on lines 10 to +15
@SerialName("per_page")
val per_page:Int,
val perPage:Int,
@SerialName("total")
val total:Int,
@SerialName("total_pages")
val total_pages:Int,
val totalPages:Int,
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
Member

Choose a reason for hiding this comment

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

줄정렬해주시면 더 최고일것같아용! ex) id:Int -> id: Int

Copy link
Member 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 +67

override fun putUserIdInPreference(userId: String) = preferenceDataSource.setUserId(userId)
Copy link
Member

Choose a reason for hiding this comment

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

위 함수에서는 runCatching을 사용했는데, 여기서는 사용 안 해준 이유가 무엇인가용?

Copy link
Member Author

Choose a reason for hiding this comment

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

그러게요.. 이 코드 짤땐 예외처리할만한 부분이 없다고 생각했는데, 내용이 없으면 예외를 던지는 방향으로 코드를 짜는게 더 좋겠네요!!

Comment on lines +19 to +20
if (response.isSuccessful) {
with(response.body()?.data!!) {
Copy link
Member

Choose a reason for hiding this comment

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

!!는 null일 경우 앱을 강제 종료시킬 수 있기 때문에 사용을 지양해주는 게 좋아요!!!!!

Copy link
Member Author

Choose a reason for hiding this comment

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

이 부분을 dto 내에서 내장함수를 통해 entity로 변환하는 코드를 통해 수정할 수 있을 것 같네요!!
리드님 코드 참고하겠습니다!!

Comment on lines +30 to +37
}
} else {
throw Exception(
JSONObject(
response.errorBody()?.string().orEmpty()
).getString(MESSAGE)
)
}
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
Member Author

Choose a reason for hiding this comment

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

모든 레포지토리에서 response.isFailure()인 경우 예외를 던지고 있습니다!
뷰모델에서 runCatching을 통해 예외처리를 하기 위해 이렇게 구현해봤어요!!

Comment on lines +21 to +29
map {
FollowerResponseEntity(
id = it.id,
email = it.email,
firstName = it.firstName,
lastName = it.lastName,
avatar = it.avatar
)
}
Copy link
Member

Choose a reason for hiding this comment

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

이 부분이 dto에 있는 값을 data class로 바꾸는 작업인 것 같은데
dto 파일 안에 확장함수를 넣어두면 repositoryImpl 파일에서는 함수만 가져오는 방법도 있답니당

Copy link
Member Author

Choose a reason for hiding this comment

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

넵! 이 부분이랑 위 !! 사용한 부분이랑 같이 고쳐볼게요!

Comment on lines +56 to +61

implementation(libs.retrofit2.retrofit)
implementation(libs.serialization.json)
implementation(libs.retrofit2.serialization.converter)
implementation(platform(libs.okhttp.bom))
implementation(libs.okhttp)
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
Member Author

Choose a reason for hiding this comment

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

이 부분은 복붙해놓고 신경을 못썼네요,, 하핫,,
순수한 도메인 명심하겠습니다!!

Comment on lines +1 to +5
package com.sopt.now.compose.domain.repository

import com.sopt.now.compose.domain.entity.request.FollowerRequestEntity
import com.sopt.now.compose.domain.entity.response.FollowerResponseEntity
import retrofit2.Response
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 +56 to +58
private fun getFollowerList() = viewModelScope.launch {
Log.d("HomeViewModel", "getFollowerList()")
followerRepository.getFollowers(
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
Member Author

Choose a reason for hiding this comment

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

지운다 해놓고 항상 못 지웁니다ㅠㅠ
로그 왠만하면 쓰지 말아야겠네요,,

Comment on lines +48 to +51
)
).fold(
onSuccess = {
updateUiState(
Copy link
Member

Choose a reason for hiding this comment

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

fold는 어떤 기능을 하나요..?!

Copy link
Member Author

Choose a reason for hiding this comment

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

사실 코리조 리드 코드보고 습득한거라 잘 몰라요,, 이 부분은 잘 알아봐야겠네요!!

Copy link
Member

@arinming arinming left a comment

Choose a reason for hiding this comment

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

멀티모듈로 뚝딱 리팩토링 해버리는,, 많이 배우고 갑니당!!!!!

Comment on lines +5 to +15
@Qualifier
@Retention(AnnotationRetention.BINARY)
annotation class AUTH

@Qualifier
@Retention(AnnotationRetention.BINARY)
annotation class HEADER

@Qualifier
@Retention(AnnotationRetention.BINARY)
annotation class REQRES
Copy link
Member

Choose a reason for hiding this comment

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

Qualifier 적용하셨ㄱ군용!

Comment on lines +16 to +20
override suspend fun postLogin(request: RequestLoginDto): Response<ResponseLoginDto> =
authService.postLogin(request)

override suspend fun postSignUp(request: RequestSignUpDto): Response<ResponseSignUpDto> =
authService.postSignup(request)
Copy link
Member

Choose a reason for hiding this comment

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

SignIn보다 Login을 선호하시나욥?
사소하지만,,SignUp, Signup 통일이 안되어있습니당!

Copy link
Member Author

Choose a reason for hiding this comment

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

Login이 뭔가 더 익숙해서 사용합니다!!
단어 통일엔 신경 써야겠네요,,,

Comment on lines 10 to +15
@SerialName("per_page")
val per_page:Int,
val perPage:Int,
@SerialName("total")
val total:Int,
@SerialName("total_pages")
val total_pages:Int,
val totalPages:Int,
Copy link
Member

Choose a reason for hiding this comment

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

줄정렬해주시면 더 최고일것같아용! ex) id:Int -> id: Int

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.

3 participants