Week6 필수,심화 과제 #10
Week6 필수,심화 과제 #10
Conversation
kamja0510
requested review from
HAJIEUN02,
jihyunniiii,
hyeeum and
1971123-seongmin
| ) | ||
| } | ||
|
|
||
| fun toSignInRequestDto(signInInformationEntity: SignInInformationEntity) = SignInRequestDto( |
| password = signInInformationEntity.password | ||
| ) | ||
|
|
||
| fun toSignUpRequestDto(signUpInformationEntity: SignUpInformationEntity) = SignUpRequestDto( |
| import kotlinx.serialization.Serializable | ||
|
|
||
| @Serializable | ||
| data class GetMyHobbyResponseDto( |
There was a problem hiding this comment.
Dto 이름을 HTTP method를 활용해서 짓기보다는 해당 데이터 클래스가 나타내는 핵심만 딱 활용해주심 좋을 것 같아요! MyHobbyResponseDto 이런 식으루,,
| ) : GetMyHobbyRepository { | ||
| override suspend fun getMyHobby(): Result<MyHobbyEntity> = | ||
| runCatching { | ||
| getMyHobbyDataSource.getMyHobby().result?.let { Mapper.toMyHobbyEntity(it) }!! |
There was a problem hiding this comment.
!!는 강제 not null 처리를 해주는 것이기 때문에 null이 들어오는 경우 발생할 예외처리를 해주면 더 좋을 것 같아요
There was a problem hiding this comment.
runCathcing의 onFailure를 사용해서 null에 대한 처리를 해주시거나 이 부분에 관련된 코드들을 다시 하나씩 보시고 null이 발생하지 않게 하면 더 좋은 방법이 될 수 있을 것 같아요.
| fun create(): SignInRepositoryImpl { | ||
| return SignInRepositoryImpl( | ||
| SignInDataSource( | ||
| ServicePool.userService |
| class GetMyHobbyUseCase( | ||
| private val getMyHobbyRepository: GetMyHobbyRepository | ||
| ) { | ||
| suspend operator fun invoke(): Result<MyHobbyEntity> = |
There was a problem hiding this comment.
네 쓰기전에 알아보고 쓰긴 했는데 왜 usecase에는 invoke를 써야하는지는 모르겠숩니다
| import org.sopt.and.domain.usecase.SignInUseCase | ||
| import org.sopt.and.presentation.signin.SignInViewModel | ||
|
|
||
| class SignInViewModelFactory : ViewModelProvider.Factory { |
|
| class GetMyHobbyDataSource( | ||
| private val userService: UserService | ||
| ) { | ||
| suspend fun getMyHobby(): GetMyHobbyResponseDto = userService.getMyHobby() |
There was a problem hiding this comment.
아키텍처를 적용해보는 것도 좋긴 한데요. 개인적으로 우려가 됩니다.
첫 주차에 봤던 재민 님의 코드가 저는 더 좋은 코드인 것처럼 느껴집니다. 재민님의 의도가 더 잘 보이는 코드 같아요.
지금은 조금 이해하기 어려운 코드가 된 것 같습니다.
이런 구조는 유지보수성을 높이기 보다 경직성을 높인다고 보입니다.
get /user/my-hobby 라는 api 하나를 화면에서 호출하기 위해서 너무나 너무나 너무나도 많은 단계가 필요해요.
api 명세는 UserService 에 정의하구요.
이를 호출해주는 DataSource는 GetMyHobbyDataSource 클래스에 정의합니다.
이 DataSource는 GetMyHobbyRepository 인터페이스의 실제 구현체인 GetMyHobbyRepositoryImpl 을 생성하기 위해서 넘겨집니다.
이렇게 만든 Repository를 GetMyHobbyUseCase 생성자에 넣어주고요.
최종적으로 ViewModel은 GetMyHobbyUseCase 인스턴스를 주입받아서 사용합니다.
여기에 등장한 객체만 UserService, GetMyHobbyDataSource, GetMyHobbyRepositoryImpl, GetMyHobbyRepository, GetMyHobbyUseCase 이렇게 총 5가지 인데요. 객체가 너무 많아보입니다.
간단한 기능 하나를 추가하는데 작성해야 하는 객체가 많다는 건 그만큼 작업할 때 공수가 많이 들어간다는 의미가 됩니다.
그렇다면 그 공수를 들여서 얻는 가치가 명확해야 합니다. 지금은 그 가치가 불명확한 상황으로 보여요.
저는 DataSource는 제외하고 Repository만 구성해보시는 걸 추천드려 볼게요.
Repository같은 경우에도 interface - implmentation 나누지 마시고 interface 없이 implementation만 구성해보시는 걸 추천드려볼게요.
There was a problem hiding this comment.
좋은 리뷰 너무 감사합니다
한번만 더 오래 고민해보겠습니다.. 저도 구현하면서 이게 맞나 라는 생각이 들었습니다
말씀하신 어떤 가치가 있을 수 있는지를 생각해보겠습니다
There was a problem hiding this comment.
안녕하세요. 안드로이드 파트장 배지현입니다.
먼저, 바쁘실텐데도 불구하고 매주 정성스러운 리뷰 남겨주셔서 감사합니다.
저도 단순히 아키텍처를 도입한 코드가 좋은 코드가 아니며 팀 상황이나 프로젝트 볼륨에 맞춰 아키텍처 도입을 결정하고 이를 바탕으로 구현된 코드가 의미가 있다고 생각해요.
그렇기 때문에 세미나 때 아키텍처를 다루고 이에 대한 과제를 내면서 그냥 단순히 아키텍처적인 지식을 전달하기보다는 아키텍처의 필요성이과 각 객체의 책임을 이해시키려고 노력을 많이 했던 것 같아요.
특히, 과제에서는 '단순히 아키텍처를 적용하라'에서 끝이 아니라 어떤 아키텍처를 고르고, 왜 골랐는지, 그리고 각 구성요소들의 책임을 무엇이라고 생각하고 왜 필요한지를 충분히 고민하고 코드를 작성할 수 있도록 다양한 장치들을 마련했어요.
하지만 과제의 볼륨이 아키텍처의 필요성을 느낄만한 볼륨이 아니기 때문에 아키텍처를 도입하게 되면서 '아 이 정도 수준의 볼륨에 아키텍처를 적용하는 것이 과연 옳은가? 더 많은 리소스가 드는 것이 아닌가?' 하는 생각이 드는 것이 어쩌면 당연한 일이라고 생각해요.
그럼에도 제가 이러한 과제를 낸 이유는 이러한 고민을 하는 것 자체가 파트원 친구들의 성장에 도움이 된다고 판단했기 때문이에요.
어떠한 기술을 몰라서 쓰지않는 것과 그 기술에 대해 잘 알고 있지만 프로젝트의 볼륨이나 팀 상황을 판단했을 때 사용하지 않는 것이 옳다고 생각해서 사용하지 않는 것이 다르고, 파트원 친구들이 배우고 있는 단계라는 것을 생각했을 때 세미나에서 다양한 기술을 다루고 그 기술의 필요성을 이해시키는 것, 그리고 그 기술을 실제 프로젝트에 어떻게 적용할 수 있을지를 알려주는 것이 파트원 친구들의 성장에 도움이 된다고 생각했거든요.
그래서 실제 서비스를 구현하는 것이 아닌 배움을 위한 과제이기에 실제였다면 해당 기술을 적용시키는 것이 더 비효율적일지라도 이를 해보게 하는 것 그리고 이를 통해 비효율성을 직접 느껴 아키텍처 도입에 대한 고민을 하게 하는 것이 파트원 친구들에게 더 의미가 있을 것이라 판단하여 이러한 과제를 내게 되었어요. (비효율성을 느끼지 못하더라도 아키텍처에 대한 고민을 하는 시간을 마련해주는 것 자체가 의미가 있다고 판단했습니다.)
저도 파트장이 처음이기에 그리고 아직 배우고 있는 입장이기에 이러한 방향으로 파트를 이끌어나가는 것이 맞는지, 이러한 방법이 파트원 친구들에게 정말로 도움이 될지 아직도 고민이 많네요. ㅜㅜ
승민님의 리뷰를 읽어 보니 과제가 끝난 후 discussion을 통해 과제에 대한 생각과 과제 의도를 공유하는 것보다 오히려 과제 의도를 사전에 설명하는 것이 파트원 친구들이 더 충분한 고민을 하게 만드는 것 같아요.
그리고 이러한 의도를 명예 OB 분들에게 공유드려야 더 의미있는 리뷰 시간이 될 것 같고요.
앞으로 더 나은 파트를 만들어 나갈 수 있도록 파트원 친구들이 더 많이 성장할 수 있도록 조금 더 많이 고민해보고 노력해보겠습니다.
항상 좋은 리뷰 남겨주셔서 정말 감사합니다!
There was a problem hiding this comment.
1주차 재민님의 코드를 봤을 때, 코드를 고민하고 작성하신게 눈에 보였었어요.
코틀린과 안드로이드 문법은 잘 모르신다고 하셨지만 코드를 통해서 구현하고자 하는게 뭔지 의도가 잘 보이는 코드였습니다. 좋은 코드라고 생각해요.
그런데 이번에 리뷰에서는 뭔가에 쫓기는 느낌을 받았었습니다. 아직 소화되지 않은 아키텍처를 급하게 적용해보는 느낌을 지울 수 없었어요. 그리고 그렇게 코드를 작성하는 것이 좋지 않다고 믿습니다.
본인이 납득할 수 있는 수준에서 코드를 작성해야 작성하는 사람도 읽는 사람도 납득할 결과물이 나올 거라고 생각해요.
혹시 쫓기면서 작성하신 코드는 아닌지 그렇다면 너무 쫓기지 마시고 본인이 이해한 만큼 코드를 고민하고 작성하는게 좋겠다고 생각해서 리뷰를 남겼었습니다.
이 리뷰가 @jihyunniiii 님의 세미나 방향과 대치되거나 의도를 잘 못 캐치한 리뷰가 될 수 있는걸 알고 있었는데요.
엄청나게 많은 라이브러리, 아키텍쳐 등 새로운 지식을 접하는 과정에서
좋은 코드란 좋은 라이브러리, 기술, 아키텍처일 것이라고 생각하거나 이런 걸 사용해야 좋은 코드라고 생각할 수 있을 것 같아서 노파심에 리뷰를 남겨보았습니다.
| }.onSuccess { response: GetHobbyResponseDto -> | ||
| response.result?.let { setMyHobby(it.hobby) } | ||
| getMyHobbyUseCase().onSuccess { myHobbyEntity: MyHobbyEntity -> | ||
| setMyHobby(myHobbyEntity.myHobby) | ||
| }.onFailure { } |
There was a problem hiding this comment.
오류처리는 다양한 방법으로 가능한데 제가 아는 제일 심플한 방법은 Failure 람다 내부에서 status code 별로 처리해주는 것..? 사실 저는 실패 분기처리를 꼼꼼하게 해본 적이 없어서.. 다른 분들의 노하우도 궁금하네요
저번에 예인언니 코드 염탐했는데 되게 꼼꼼히 하시더라구요 한 번 참고해보시긜...!
| Button( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .height(50.dp), |
There was a problem hiding this comment.
아마 합세때 리드분이 잘 알려주셨을텐데 혹시나 해서 남겨두어요!
추후 기기의 글자 대응을 하려면 height로 고정 dp 주는 것보다 padding으로 크기 조절하는 것이 좋습니다!
폰트 크기 키우면 글자가 잘려요
|
|
||
| @Composable | ||
| fun SignInBtn( | ||
| signIn: (String, String) -> Unit, |
There was a problem hiding this comment.
signIn 이라는 매개변수만 보고서는 어떤 역할을 하는지 유추하기 힘든 것 같아요!
| } else if (signUpResponseEntity.code == SignUpFailureCase.FAILURE_DUPLICATE_USERNAME.errorCode | ||
| && signUpResponseEntity.status == SignUpFailureCase.FAILURE_DUPLICATE_USERNAME.statusCode | ||
| ) { |
There was a problem hiding this comment.
onFailure에는 람다의 인자로 Throwable이 오더라고요.. 그래서 400대의 status code를 처리할 방법을 모르겠숩니다 ㅠ
There was a problem hiding this comment.
Throwable이 HttpException인지를 판단해서 HttpException이면 error Body나 status code값을 가져올 수 있어요.
UiState.Error(
message = if (exception is HttpException) {
exception.response()?.errorBody()
?.byteString()?.utf8()?.let { errorBodyJson ->
Json.decodeFromString<NullableBaseResponse<Unit>>(errorBodyJson).message
}
} else {
exception.message
},
code = (exception as? HttpException)?.response()?.code()
)
예전에 작성한 코드라 썩 좋은 코드같지는 않지만 참고해보시면 좋을 것 같네요 ~
코드 전체가 궁금하시면 아래 링크 참고하셔요!
https://github.com/TeamPINGLE/PINGLE-ANDROID/blob/develop/app/src/main/java/org/sopt/pingle/presentation/ui/main/home/HomeViewModel.kt
| import org.sopt.and.domain.usecase.GetMyHobbyUseCase | ||
| import org.sopt.and.presentation.myinfo.MyInfoViewModel | ||
|
|
||
| class MyInfoViewModelFactory : ViewModelProvider.Factory { |
There was a problem hiding this comment.
뷰모델 팩토리!! 저는 사실 바로 Hilt를 사용해서 뷰모델 팩토리를 사용을 안해봤습니다..
기회가 된다면 저도 공부해보겠습니다.
이 부분 관련해서 Hilt라는 것도 한번 찾아 보시면 좋을 것 같아요
There was a problem hiding this comment.
하나의 뷰모델 팩토리를 통해 관리해도 좋을 것 같네요 ~
| ) : GetMyHobbyRepository { | ||
| override suspend fun getMyHobby(): Result<MyHobbyEntity> = | ||
| runCatching { | ||
| getMyHobbyDataSource.getMyHobby().result?.let { Mapper.toMyHobbyEntity(it) }!! |
There was a problem hiding this comment.
runCathcing의 onFailure를 사용해서 null에 대한 처리를 해주시거나 이 부분에 관련된 코드들을 다시 하나씩 보시고 null이 발생하지 않게 하면 더 좋은 방법이 될 수 있을 것 같아요.
| request | ||
| ) | ||
| ) | ||
| )!! |
There was a problem hiding this comment.
이 부분도 널에 대한 처리나 널이 발생하지 않게 해주시면 더 좋을 것 같아요.
| return GetMyHobbyRepositoryImpl( | ||
| GetMyHobbyDataSource( | ||
| ServicePool.userService | ||
| ) |
There was a problem hiding this comment.
fun create(
userService: UserService
) 이런식으로 외부에서 서비스를 주입받으면 나중에 서비스를 다른 서비스로 교체하기도 쉽고 코드의 역할이 더 명확할 것 같아요.
ex) create 함수는 단순히 객체를 만드는 부분 / 어떤 서비스를 사용할지는 호출하는 쪽에서 결정하는 식으로..
There was a problem hiding this comment.
음! 좋은 의견인거 같네요! 이해했습니다
| class GetMyHobbyDataSource( | ||
| private val userService: UserService | ||
| ) { | ||
| suspend fun getMyHobby(): GetMyHobbyResponseDto = userService.getMyHobby() |
There was a problem hiding this comment.
안녕하세요. 안드로이드 파트장 배지현입니다.
먼저, 바쁘실텐데도 불구하고 매주 정성스러운 리뷰 남겨주셔서 감사합니다.
저도 단순히 아키텍처를 도입한 코드가 좋은 코드가 아니며 팀 상황이나 프로젝트 볼륨에 맞춰 아키텍처 도입을 결정하고 이를 바탕으로 구현된 코드가 의미가 있다고 생각해요.
그렇기 때문에 세미나 때 아키텍처를 다루고 이에 대한 과제를 내면서 그냥 단순히 아키텍처적인 지식을 전달하기보다는 아키텍처의 필요성이과 각 객체의 책임을 이해시키려고 노력을 많이 했던 것 같아요.
특히, 과제에서는 '단순히 아키텍처를 적용하라'에서 끝이 아니라 어떤 아키텍처를 고르고, 왜 골랐는지, 그리고 각 구성요소들의 책임을 무엇이라고 생각하고 왜 필요한지를 충분히 고민하고 코드를 작성할 수 있도록 다양한 장치들을 마련했어요.
하지만 과제의 볼륨이 아키텍처의 필요성을 느낄만한 볼륨이 아니기 때문에 아키텍처를 도입하게 되면서 '아 이 정도 수준의 볼륨에 아키텍처를 적용하는 것이 과연 옳은가? 더 많은 리소스가 드는 것이 아닌가?' 하는 생각이 드는 것이 어쩌면 당연한 일이라고 생각해요.
그럼에도 제가 이러한 과제를 낸 이유는 이러한 고민을 하는 것 자체가 파트원 친구들의 성장에 도움이 된다고 판단했기 때문이에요.
어떠한 기술을 몰라서 쓰지않는 것과 그 기술에 대해 잘 알고 있지만 프로젝트의 볼륨이나 팀 상황을 판단했을 때 사용하지 않는 것이 옳다고 생각해서 사용하지 않는 것이 다르고, 파트원 친구들이 배우고 있는 단계라는 것을 생각했을 때 세미나에서 다양한 기술을 다루고 그 기술의 필요성을 이해시키는 것, 그리고 그 기술을 실제 프로젝트에 어떻게 적용할 수 있을지를 알려주는 것이 파트원 친구들의 성장에 도움이 된다고 생각했거든요.
그래서 실제 서비스를 구현하는 것이 아닌 배움을 위한 과제이기에 실제였다면 해당 기술을 적용시키는 것이 더 비효율적일지라도 이를 해보게 하는 것 그리고 이를 통해 비효율성을 직접 느껴 아키텍처 도입에 대한 고민을 하게 하는 것이 파트원 친구들에게 더 의미가 있을 것이라 판단하여 이러한 과제를 내게 되었어요. (비효율성을 느끼지 못하더라도 아키텍처에 대한 고민을 하는 시간을 마련해주는 것 자체가 의미가 있다고 판단했습니다.)
저도 파트장이 처음이기에 그리고 아직 배우고 있는 입장이기에 이러한 방향으로 파트를 이끌어나가는 것이 맞는지, 이러한 방법이 파트원 친구들에게 정말로 도움이 될지 아직도 고민이 많네요. ㅜㅜ
승민님의 리뷰를 읽어 보니 과제가 끝난 후 discussion을 통해 과제에 대한 생각과 과제 의도를 공유하는 것보다 오히려 과제 의도를 사전에 설명하는 것이 파트원 친구들이 더 충분한 고민을 하게 만드는 것 같아요.
그리고 이러한 의도를 명예 OB 분들에게 공유드려야 더 의미있는 리뷰 시간이 될 것 같고요.
앞으로 더 나은 파트를 만들어 나갈 수 있도록 파트원 친구들이 더 많이 성장할 수 있도록 조금 더 많이 고민해보고 노력해보겠습니다.
항상 좋은 리뷰 남겨주셔서 정말 감사합니다!
| import org.sopt.and.domain.model.SignUpResponseEntity | ||
| import retrofit2.Response | ||
|
|
||
| object Mapper { |
There was a problem hiding this comment.
Mapper를 object로 관리하시는 이유가 있나요?
| SignUpResponseEntity( | ||
| no = it.no, | ||
| status = signUpResponseDto.code(), | ||
| code = signUpResponseDto.body()!!.code |
There was a problem hiding this comment.
!!는 NPE를 발생시킬 수 있으니 사용을 지양합시다!
| import org.sopt.and.data.datasource.GetMyHobbyDataSource | ||
| import org.sopt.and.data.repositoryimpl.GetMyHobbyRepositoryImpl | ||
| import org.sopt.and.data.service.ServicePool | ||
| import org.sopt.and.domain.model.MyHobbyEntity |
There was a problem hiding this comment.
domain은 순수한 Kotlin, java 코드로 이루어져야 합니다.
data와 presentation layer에 대한 의존성도 없어야 하고요!
| import org.sopt.and.data.datasource.SignInDataSource | ||
| import org.sopt.and.data.repositoryimpl.SignInRepositoryImpl | ||
| import org.sopt.and.data.service.ServicePool | ||
| import org.sopt.and.domain.model.SignInInformationEntity | ||
| import org.sopt.and.domain.model.SignInResponseEntity |
There was a problem hiding this comment.
마찬가지에요! 의존성을 잘 고민해봅시다 ~
| import org.sopt.and.domain.usecase.GetMyHobbyUseCase | ||
| import org.sopt.and.presentation.myinfo.MyInfoViewModel | ||
|
|
||
| class MyInfoViewModelFactory : ViewModelProvider.Factory { |
| import org.sopt.and.domain.usecase.GetMyHobbyUseCase | ||
| import org.sopt.and.presentation.myinfo.MyInfoViewModel | ||
|
|
||
| class MyInfoViewModelFactory : ViewModelProvider.Factory { |
There was a problem hiding this comment.
하나의 뷰모델 팩토리를 통해 관리해도 좋을 것 같네요 ~
| } else if (signUpResponseEntity.code == SignUpFailureCase.FAILURE_DUPLICATE_USERNAME.errorCode | ||
| && signUpResponseEntity.status == SignUpFailureCase.FAILURE_DUPLICATE_USERNAME.statusCode | ||
| ) { |
There was a problem hiding this comment.
Throwable이 HttpException인지를 판단해서 HttpException이면 error Body나 status code값을 가져올 수 있어요.
UiState.Error(
message = if (exception is HttpException) {
exception.response()?.errorBody()
?.byteString()?.utf8()?.let { errorBodyJson ->
Json.decodeFromString<NullableBaseResponse<Unit>>(errorBodyJson).message
}
} else {
exception.message
},
code = (exception as? HttpException)?.response()?.code()
)
예전에 작성한 코드라 썩 좋은 코드같지는 않지만 참고해보시면 좋을 것 같네요 ~
코드 전체가 궁금하시면 아래 링크 참고하셔요!
https://github.com/TeamPINGLE/PINGLE-ANDROID/blob/develop/app/src/main/java/org/sopt/pingle/presentation/ui/main/home/HomeViewModel.kt
Related issue 🛠
Work Description ✏️
Uncompleted Tasks 😅
To Reviewers 📢