Conversation
- 서버 API 미구현으로 아직 온전한 기능 구현 상태는 아님
- 동아리가 클럽으로 되어 있어서 데이터를 변환하지 못하는 버그였음.
- Getter 추가 - JOB 직무로 변경 woowacourse-teams#42
| val activities: List<Activity> | ||
| ) { | ||
| companion object { | ||
| fun from(activitiesApiModels: List<ActivitiesApiModel>): List<Activities> = |
There was a problem hiding this comment.
데이터 모델이 ApiModel에 의존해서는 안된다고 생각합니다. apiModel.toExternal() 혹은 apiModel.toDomain()이런 식으로 바꾸는 게 좋을 것 같습니다. 만약 로컬 데이터 저장소에서 로그인 데이터를 불러온다면 Login.from(entity: LoginEntity) 이렇게 새로 만들어줘야 하기 때문입니다. 즉, 데이터 소스 모델에 의해 데이터 모델이 변경되면 안된다고 생각합니다.
정적 팩토리 메서드는 해당 클래스 혹은 그 하위 타입을 반환하는 것이 좋다고 생각합니다! 리스트처럼 다른 타입으로 변환하는 일은 다른 Activities 외부에서 하는 것이 좋지 않을까요? 이처럼 다른 타입으로 변환하는 일을 Activities에 포함한다면 Activities 클래스가 하는 일이 많아질 거라고 생각합니다.
There was a problem hiding this comment.
정리하자면,
이후에 같은 데이터 모델에 대해 여러 타입으로 변환될 일이 생길 수 있기 때문에,
변환을 모두 데이터 모델에 맡기면 한 클래스가 하는 일이 많아질 수 있고, 기존 클래스의 변경을 최소화하자는 말씀으로 이해하고 리뷰 반영하겠습니다.
| val name: String, | ||
| ) { | ||
| companion object { | ||
| fun from(activitiesApiModel: List<ActivityApiModel>): List<Activity> = |
There was a problem hiding this comment.
위의 ApiModel 및 정적 팩토리 메서드 리뷰와 마찬가지입니다.
| val isRegistered: Boolean, | ||
| ) { | ||
| companion object { | ||
| fun from(apiModel: LoginApiModel): Login = Login( |
There was a problem hiding this comment.
위의 ApiModel 리뷰와 마찬가지입니다.
| @SerialName("activityIds") | ||
| val activityIds: List<Int>, | ||
| ) { | ||
| companion object { |
There was a problem hiding this comment.
이것처럼 ApiModel이 데이터 모델에 의존해야 한다고 생각합니다.
| } | ||
|
|
||
| private fun fetchActivities(): Job = viewModelScope.launch { | ||
| when (val activitiesApiModel = activityRepository.getActivities()) { |
There was a problem hiding this comment.
리포지터리에서 ApiModel을 반환하나요?
There was a problem hiding this comment.
apiResult를 apiModel로 잘못 작성하였네요..!
activitiesResult로 변경하였습니다.
| var isSelected: Boolean = false, | ||
| ) { | ||
| companion object { | ||
| fun from(activities: List<Activity>): List<ActivityUiState> = |
There was a problem hiding this comment.
위의 정적 팩토리 메서드 리뷰와 동일합니다.
There was a problem hiding this comment.
List는 T가 아니기 때문에 적절한 정적 팩토리 메서드 패턴이 아니라는 의미로 이해했습니다.
코드 수정 완료하였습니다.
| val activities: List<ActivityUiState>, | ||
| ) { | ||
| companion object { | ||
| fun from(tags: List<Activities>): List<ActivitiesUiState> = tags.map(::from) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
|
||
| class TokenRepositoryImpl(context: Context) : TokenRepository { | ||
| private val preference: SharedPreferences = | ||
| context.getSharedPreferences("kerdy", Context.MODE_PRIVATE) |
There was a problem hiding this comment.
SharedPreference 도 싱글톤으로 만들어서 의존성으로 사용하면 어떨까요?
There was a problem hiding this comment.
SharedPreferenceContainer를 만들어서 주입받도록 변경하였습니다.
앞으로는 다른 곳에서도 Preference를 공통으로 사용할 수 있습니다.
| setOnCheckedChangeListener { _, _ -> viewModel.toggleTagSelection(jobTag) } | ||
| } | ||
|
|
||
| override fun onClick(view: View) { |
There was a problem hiding this comment.
그냥 buttonClickListener 에 한번에 구현하는 것이 개인적으로 가독성이 좋아보여요
There was a problem hiding this comment.
현재 클릭할 뷰가 하나이므로, 스캇 말씀처럼 View.OnClickListener를 구현하는 것보다 바로 setOnClickListener에 구현하는 것이 가독성이 좋아 보입니다!
수정 완료하였습니다 : )
| private val activityRepository: ActivityRepository, | ||
| private val memberRepository: MemberRepository, | ||
| ) : ViewModel() { | ||
| val nameUiState: MutableLiveData<String> = MutableLiveData() |
There was a problem hiding this comment.
이 변수는 naeUiState 로 하고, 나머지는 UiState 안붙인게 통일성에 안맞는 것 같아요. 이것도 그냥 name 으로 하는것이 좋을 것 같네요.
There was a problem hiding this comment.
감사합니다! nameUiState는 깜빡하고 프리픽스를 제거하지 않았었네요 😭
수정 완료하였습니다~
| block: ActivityTag.() -> Unit | ||
| ): ActivityTag = requireContext().chipOf(block) | ||
|
|
||
| fun Context.chipOf( |
There was a problem hiding this comment.
이건 안쓰이는것 같은데 지워도 될 것 같아요!
#️⃣연관된 이슈
📝작업 내용
스크린샷 (선택)
예상 소요 시간 및 실제 소요 시간
💬리뷰 요구사항(선택)