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

[리팩토링] 과제 리팩토링 - 아키텍처 + 힐트 적용 #19

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

Conversation

Hyobeen-Park
Copy link
Collaborator

Related issue 🛠

Work Video ✏️

To Reviewers❤️

드디어!!! 힐트 적용 성공했습니다아!!!
근데 왠지 모르겠지만 pr에 영상이 올라가지 않는 이슈가 발생해서 일단 사진으로 넣었어욤ㅎㅎ

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 +12 to +13

suspend fun getUserInfo(memberId: String): BaseResponse<ResponseUserInfoDto>
Copy link
Member

Choose a reason for hiding this comment

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

오 BaseResponse까지!! 좋네용

Comment on lines +9 to +11

class AuthRemoteDatasourceImpl : AuthRemoteDataSource {
private val authService = ServicePool.authService
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 +7 to +11

@Serializable
data class ResponseFriendsDto(
@SerialName("page")
val page: Int = 2,
Copy link
Member

Choose a reason for hiding this comment

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

DTO에서 2를 넣어준 이유가 있나용.,,,??
이 부분은 feature에서 넣어주는 게 분리 측면에서 좋은 것 같아요!!

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 +35 to +57
) {
fun toFriendEntity(): Friend =
Friend(
id = this.id,
email = this.email,
firstName = this.firstName,
lastName = this.lastName,
avatar = this.avatar
)
}

@Serializable
data class ResponseFriendsListSupportDto(
@SerialName("url")
val url: String,
@SerialName("text")
val text: String,
)

fun toFriendsList(): FriendsList =
FriendsList(
friendsList = this.data.map { it.toFriendEntity() }
)
Copy link
Member

Choose a reason for hiding this comment

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

함수를 두 개 만들어준 의도가 있나용...? 이렇게 하면 한 번 통신하는데 domain에서 두 개의 model이 생기는 등 불필요한 작업이 들어갈 것 같아서요!

Copy link
Collaborator Author

@Hyobeen-Park Hyobeen-Park Jul 3, 2024

Choose a reason for hiding this comment

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

원래는 friendsList를 반환해야겠다고 생각하고 만들다보니 함수가 두 개가 되었는데 흠,, 지금 생각해보니 model이 두 개가 생기는 문제도 있네요!! friendsList를 만들지 않고도 가능할 것 같아서 이부분 수정해볼게요!! 감사합니다❤️

Comment on lines +22 to +25
override suspend fun getUserInfo(memberId: String): Result<User> =
runCatching {
authRemoteDataSource.getUserInfo(memberId = memberId).data?.toUserEntity()!!
}
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 +25 to +27
init {
getFriendsInfo(2)
}
Copy link
Member

Choose a reason for hiding this comment

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

지금 여기서 page에 2를 넣어주고 있는 것 같은데, 그러면 위 DTO에서 2는 지워줘도 될 것 같습니당!

Comment on lines +30 to +32
_homeFriendState.value = UiState.Loading
viewModelScope.launch {
friendRepository.getFriendsList(page = page).onSuccess { friendsList ->
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 +194 to +195
val user = User(R.drawable.ic_baseline_visibility_white_24, id, nickname, phone)
signupViewModel.postSignup(user, pw)
Copy link
Member

Choose a reason for hiding this comment

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

나중에도 언급을 할 거긴 한데 pw처럼 축약어는 지양하는 게 좋을 것 같아요!
password로 표시하는 게 협업할 때도 찾기 편할 것 같습니당

Comment on lines +20 to +22
private var phone_pattern = "^01([0|1|6|7|8|9])-([0-9]{4})-([0-9]{4})$"
private var pw_pattern = "^(?=.*[a-zA-Z])(?=.*[0-9])(?=.*[!@#$%^&*])[a-zA-Z0-9!@#$%^&*]{8,12}$"

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로 둬도 괜찮을 것 같아요!

Comment on lines +37 to +42
contentDescription = "User Profile Image",
imageSize = 70.dp,
nickname = "터닝안드효빈",
phone = "❤️",
fontSize = 24.sp
)
Copy link
Member

Choose a reason for hiding this comment

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

터닝 개발할 때는 string 추출 해주세욤!

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