feat: 프로필 이미지 변경 및 타임라인 닉네임 표시 #538 #603
Conversation
s6m1n
force-pushed
the
feature/#538-member-profile-image-change
branch
from
08c1ed4 to
feae3c7
Compare
너무 좋은 것 같아요! 프로필 정보를 불러오는 요청이 마이페이지에만 국한되지 않아서 적절하다고 생각합니다.
이 의견도 동의합니다!
이 부분도 좋은 의견이에요! 사실 새로운 계정을 생성한다는 의미에서는 빙티의 말대로 "SignUp"이라는 이름이 더 적합한 것 같아요. 저희가 지금과 같이 서비스의 로그인 기능을 구현한 이유는, 사용자에게 회원가입 같은 귀찮은 절차를 없애고, 바로 서비스를 시작할 수 있도록 제공하기 위함이었어요. 리니와 함께 다른 서비스의 회원 등록 절차를 참고했을 때에도 "카카오로 로그인하기", "Login with Google" 과 같이 "로그인"을 강조하는 것을 보았어요. 아마도 저희가 생각한 것과 비슷한 이유일거라고 추측했습니다! 그래서 회원가입(SignUp)을 나타내는 표현보다는, 지금의 이름을 유지하거나 다른 적절한 이름을 짓는 것이 좋다고 생각합니다! 추가로 마지막 의견을 보고 빙티의 의견이 조금 헷갈리는데, interface MemberProfileRepository {
// MemberProfile을 GET
suspend fun getMemberProfile(): ResponseResult<MemberProfile>
// MemberProfile의 profileImageUrl을 POST
suspend fun changeProfileImage(profileImageFile: MultipartBody.Part): ResponseResult<ProfileImageResponse>
}
interface LoginRepository {
suspend fun fetchTokenWithRecoveryCode(recoveryCode: String): ResponseResult<String>
}
interface SignUpRepository {
suspend fun signUpWithNickname(nickname: String): ResponseResult<String>
} |
There was a problem hiding this comment.
너무 고생 많으셨어요 빙티!! 빠르게 구현 완료해주셨네요...! 쵝오~~ 😁
드디어 사용자들이 프로필 이미지를 변경할 수 있게 되겠군요...! 👍
또 코드에서도 많은 고민과 개선의 흔적이 보였습니다!! 전체적으로 깔끔해지고, 또 세세한 부분도 신경쓴 것이 느껴졌습니다!
몇 가지 질문들과 개선하면 좋을 부분 코멘트로 남겨놓았어요~ 천천히 확인 부탁드려요!
그리고 도메인 모델인 MemberProfile이 Activity에 나타나있는 것이 아쉬워서, 어떻게하면 좋을까 고민해보고 있어요. 빙티도 혹시 고민 중이라면, 잠시 뒤 도서관에서 같이 이야기해봅시다...!
| @@ -11,7 +12,32 @@ class UserInfoPreferencesManager(context: Context) { | |||
|
|
|||
| suspend fun getToken(): String? { | |||
| return withContext(Dispatchers.IO) { | |||
| mUserInfoPrefs.getString(TOKEN_KEY_NAME, "") | |||
| mUserInfoPrefs.getString(TOKEN_KEY_NAME, EMPTY_STRING) | |||
There was a problem hiding this comment.
빈 문자열도 상수화 처리를 해주셨네요!
저는 예전에 빈 문자열에 대해서도 상수화를 하면 가독성이 떨어질 수 있다는 리뷰를 받은 적이 있었습니다. 빈 문자열인 경우에는 ""로 두는게 더 명시적이라고 느껴져서, 빈 문자열은 따로 처리하지 않아요.
빙티는 혹시 이 의견에 대해서 어떻게 생각하시나요...! 변경을 원하는 부분은 아니고, 순수하게 빙티의 의견이 궁금합니다!
There was a problem hiding this comment.
이 부분은 빈 문자열을 EMPTY_STRING이라는 이름으로 상수화해서 가독성이 떨어진다고 느껴질 수도 있을 것 같아요~
만약 상수화 하는 방향으로 간다면 좀 더 명시적인 이름으로 변경해봐도 좋을 것 같습니다!
ex) 값이 없는 상태를 뜻하는 단어, defValue라는 것을 뜻하는 단어 등
There was a problem hiding this comment.
상수화 할래 말래?
""는 꽤 일반적인 문자열이라 현재 프로젝트 곳곳에서 쓰이고 있습니다.
따라서 개인적인 선호 + 유지보수의 용이성을 위해,
UserInfoPreferencesManager 내에서 getString의 반환값이 null인 경우를 처리하기 위해 사용되는 것들을 묶어 상수화 했습니다!
호두는 이런 경우를 고려하여도 ""로 두는 게 더욱 명시적이라고 생각하시는지요?.?
답은 없기 때문에 저희 셋이 이야기를 나눠보며 가장 보기 편한 걸로 하면 좋을 것 같아요!~
상수 위치 논의
또한 ""는 MemberProfile 클래스의 isValid에서 Empty 여부를 검사해 유효성을 검사하는 로직과 연관되어 있습니다.
data class MemberProfile(
val profileImageUrl: String? = null,
val nickname: String,
val uuidCode: String,
) {
fun isValid() = uuidCode.isNotEmpty() && nickname.isNotEmpty()
}그래서 const val DEFAULT_STRING = ""를 MemberProfile이 들고 있도록 하고 UserInfoPreferencesManager에서 가져다쓰는 게 더 적절하지 않나 생각이 드네요!
(참고로 UserInfoPreferencesManager 클래스에는 이미 import com.on.staccato.domain.model.MemberProfile 가 있습니다.)
상수 이름 추천
값이 없는 상태를 뜻하는 단어, defValue라는 것을 뜻하는 단어 등
또다시 천하제일 작명대회가 열렸습니다.
만약 상수화를 한다면 어떤 이름이 적절할까요?
EMPTY_STRING, DEFAULT_STRING 저는 이 두 개밖에 안 떠오르는데요. 나름 괜찮지 않나..ㅋ.ㅋ
| @@ -106,7 +106,7 @@ class PhotoAttachFragment : BottomSheetDialogFragment(), PhotoAttachHandler { | |||
| if (context is OnUrisSelectedListener) { | |||
| uriSelectedListener = context | |||
| } else { | |||
| throw RuntimeException() | |||
| throw IllegalStateException("Activity or Fragment must implement OnUrisSelectedListener") | |||
There was a problem hiding this comment.
IllegalStateException으로 변경하면서 적절한 예외와 에러 메시지를 나타내게 되었네요! 👍
| @HiltViewModel | ||
| class SharedViewModel | ||
| @Inject | ||
| constructor(private val myPageRepository: MyPageRepository) : ViewModel() { |
There was a problem hiding this comment.
SharedViewModel도 Hilt를 적용해주셨네요~ 👍
| @@ -177,7 +178,7 @@ class MemoryUpdateViewModel | |||
| context: Context, | |||
| uri: Uri, | |||
| ): Job { | |||
| val thumbnailFile = convertMemoryUriToFile(context, uri, name = MEMORY_FILE_NAME) | |||
| val thumbnailFile = convertMemoryUriToFile(context, uri, IMAGE_FORM_DATA_NAME) | |||
There was a problem hiding this comment.
Util에 IMAGE_FORM_DATA_NAME를 정의해서 사용했군요!
Activity마다 있던 같은 값의 동반객체가 제거되어서 더욱 깔끔해지고 좋은 것 같습니다!
| <variable | ||
| name="viewModel" | ||
| type="com.on.staccato.presentation.main.viewmodel.MapsViewModel" /> |
There was a problem hiding this comment.
데이터바인딩한 MapsViewModel은 사용되지 않고 있는 것 같아요...! 혹시 따로 추가하신 이유가 있을까요??
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:layout_marginEnd="12dp" | ||
| android:layout_marginBottom="12dp" | ||
| android:contentDescription="@string/mypage_update_profile_image_description" | ||
| android:onClick="@{()->menuHandler.onProfileImageChangeClicked()}" |
There was a problem hiding this comment.
직접 실행해서 터치해보니, 아이콘 크기가 작아서 터치가 잘 되지 않았어요!
그래서 터치가 쉽도록 프로필 변경 이미지 Handler를 프로필 이미지 View에 연결하거나, 아이콘의 터치 영역을 조금 더 넓히는 것이 좋아보여요! (사실 이 작업은 제가 했어야 했습니다...🥲)
추가로 궁금한 점이 있어요! 혹시 handler를 프로필 이미지가 아닌 아이콘에 연결한 이유는, 나중에 프로필 이미지를 터치했을 때 이미지를 확대해서 보여주는 화면 구현을 생각해서인가요??
There was a problem hiding this comment.
그래서 터치가 쉽도록 프로필 변경 이미지 Handler를 프로필 이미지 View에 연결하거나, 아이콘의 터치 영역을 조금 더 넓히는 것이 좋아보여요!
아이콘에 18dp의 padding을 주어 터치 영역을 넓혔습니다!
추가로 궁금한 점이 있어요! 혹시 handler를 프로필 이미지가 아닌 아이콘에 연결한 이유는, 나중에 프로필 이미지를 터치했을 때 이미지를 확대해서 보여주는 화면 구현을 생각해서인가요??
아뇨 딱히 그런 기능을 의도하진 않았습니다! 여유가 된다면 추가해도 좋을 것 같네요.
단순히 제가 사용자라면 아이콘을 눌러 수정하는 기능을 기대할 것이라고 생각했기 때문이에요..!
| @@ -50,17 +50,16 @@ | |||
| tools:src="@drawable/icon_member" /> | |||
|
|
|||
| <ImageView | |||
| android:id="@+id/iv_mypage_profile_image_icon" | |||
| android:id="@+id/iv_mypage_profile_image_change_icon" | |||
There was a problem hiding this comment.
id 이름에 change가 없었군요...?? 🥲
고쳐주셔서 감사해요!
| import com.on.staccato.databinding.ActivityMypageBinding | ||
| import com.on.staccato.domain.model.MemberProfile |
There was a problem hiding this comment.
Activity 이지만, domain의 MemberProfile 모델이 import되어있어요...! 현재 구현 상 어쩔 수 없지만, View와 Model 사이의 의존성이 생겨 아쉽습니다.
UiModel을 적용해보는건 어떨까 했지만, SharedPreferences에 접근하여 데이터를 불러오고 설정하기 때문에, UiModel을 도입하는 것도 불필요하다고 느껴졌습니다.
적어도 import 만이라도 없애고 싶은데, 혹시 따로 생각해놓은 개선 방법이 있으신가요...!
| fun startWithResultLauncher( | ||
| context: Context, | ||
| activityLauncher: ActivityResultLauncher<Intent>, | ||
| ) { | ||
| Intent(context, MyPageActivity::class.java).apply { | ||
| activityLauncher.launch(this) | ||
| } | ||
| } |
There was a problem hiding this comment.
ActivityResultLauncher 메서드 설정 좋습니다!! 👍
| import com.on.staccato.databinding.ActivityMainBinding | ||
| import com.on.staccato.domain.model.MemberProfile |
There was a problem hiding this comment.
여기도... 😿
지금 생각 중인 구현이 있는데, 혹시 괜찮다면 코드를 공유해볼까 합니다!
There was a problem hiding this comment.
아래에 코멘트로 관련 의견을 남겨두었어요...!
이렇게 한다면 View와 Model 간의 의존성을 분리시키고, SharedPreferences의 활용성을 높일 수 있을 것 같습니다!
There was a problem hiding this comment.
View와 Model 간의 의존성을 분리시키고, SharedPreferences의 활용성을 높일 수 있을 것 같습니다!
저도 이와 관련하여 MainActivity와 UserInfoPreferencesManager에 대한 의견을 남길 부분이 있는데, 몸 상태가 나아지면 생각을 정리해서 공유하겠습니다..! 죄송합니다 ㅠㅠ
➡️ 이와 관련된 코멘트 남겨뒀습니당! #603 (comment)
| override fun onProfileImageChangeClicked() { | ||
| if (!photoAttachFragment.isAdded) { | ||
| photoAttachFragment.show(fragmentManager, PhotoAttachFragment.TAG) | ||
| } | ||
| } | ||
|
|
||
| override fun onUrisSelected(vararg uris: Uri) { | ||
| val imageFile = convertMemoryUriToFile(this, uris.first(), IMAGE_FORM_DATA_NAME) | ||
| myPageViewModel.changeProfileImage(imageFile) | ||
| } | ||
|
|
There was a problem hiding this comment.
initStartView가 로직 순서 상 제일 먼저 실행되어서 초기화 작업을 해주고 있으니까, 해당 오버라이드 메서드들이 initStartView 보다 아래에 위치하는게 좋을 것 같아요! 순서만 옮겨주시길 요청드려요!
|
빙티가 현재
위의 문제점을 해결하기 위해서는, MVVM 아키텍처를 따르고, 추상화와 의존성 주입을 활용하여 의존 관계를 최대한 끊어내는 것이 방법이 될 것 같아요. 그래서 아래 구현을 제안해봅니다!
1.
|
| @@ -11,7 +12,32 @@ class UserInfoPreferencesManager(context: Context) { | |||
|
|
|||
| suspend fun getToken(): String? { | |||
| return withContext(Dispatchers.IO) { | |||
| mUserInfoPrefs.getString(TOKEN_KEY_NAME, "") | |||
| mUserInfoPrefs.getString(TOKEN_KEY_NAME, EMPTY_STRING) | |||
There was a problem hiding this comment.
mUserInfoPrefs 변수에 m 키워드를 붙이신 이유가 있으신가요? 혹시 m이 멤버 변수를 뜻한다면 m 키워드는 제거해도 괜찮을 것 같아서요..! 이 부분은 예전에 호두가 작업했던 부분이라 호두에게 여쭤봅니다😄
There was a problem hiding this comment.
혹시 m이 멤버 변수를 뜻한다면 m 키워드는 제거해도 괜찮을 것 같아서요..!
맞아요! 멤버 변수의 member를 뜻합니다! 클래스 내부에서만 사용되는 private 필드인 경우, 멤버 변수라는 의미로 member를 붙이는 컨벤션입니다.
다만 자바에서 주로 활용되는 컨벤션이니, 코틀린 코드와는 어울리지 않은 것 같네요!
해나의 의견대로 제거하는 것이 좋을 것 같습니다! 👍
| suspend fun getMemberProfile(): MemberProfile = | ||
| MemberProfile( | ||
| profileImageUrl = getProfileImageUrl() ?: EMPTY_STRING, | ||
| nickname = getNickname() ?: EMPTY_STRING, | ||
| uuidCode = getRecoveryCode() ?: EMPTY_STRING, | ||
| ) |
There was a problem hiding this comment.
data class MemberProfile(
val profileImageUrl: String? = null,
val nickname: String,
val uuidCode: String,
) 현재 저희의 MemberProfile은 nickname과 uuidCode가 nullable일 수 없어요!
getNickname(), getRecoveryCode() 에서 nullable 하지 않은 상태로 반환해주는 건 어떨까요?
| suspend fun getMemberProfile(): MemberProfile = | |
| MemberProfile( | |
| profileImageUrl = getProfileImageUrl() ?: EMPTY_STRING, | |
| nickname = getNickname() ?: EMPTY_STRING, | |
| uuidCode = getRecoveryCode() ?: EMPTY_STRING, | |
| ) | |
| suspend fun getMemberProfile(): MemberProfile = | |
| MemberProfile( | |
| profileImageUrl = getProfileImageUrl() ?: EMPTY_STRING, | |
| nickname = getNickname(), | |
| uuidCode = getRecoveryCode(), | |
| ) |
There was a problem hiding this comment.
sharedPreference의 getString 메서드 반환 타입이 Nullable이라 아래처럼 수정해주었습니다!
override suspend fun getMemberProfile(): MemberProfile =
MemberProfile(
profileImageUrl = getProfileImageUrl(),
nickname = getNickname(),
uuidCode = getRecoveryCode(),
)
private suspend fun getProfileImageUrl(): String? {
return withContext(Dispatchers.IO) {
userInfoPrefs.getString(PROFILE_IMAGE_URL_KEY_NAME, null)
}
}
private suspend fun getNickname(): String {
return withContext(Dispatchers.IO) {
userInfoPrefs.getString(NICKNAME_KEY_NAME, null) ?: EMPTY_STRING
}
}
private suspend fun getRecoveryCode(): String {
return withContext(Dispatchers.IO) {
userInfoPrefs.getString(RECOVERY_CODE_KEY_NAME, null) ?: EMPTY_STRING
}
}| private suspend fun getNickname(): String? { | ||
| return withContext(Dispatchers.IO) { | ||
| mUserInfoPrefs.getString(NICKNAME_KEY_NAME, null) | ||
| } | ||
| } | ||
|
|
||
| private suspend fun getRecoveryCode(): String? { | ||
| return withContext(Dispatchers.IO) { | ||
| mUserInfoPrefs.getString(RECOVERY_CODE_KEY_NAME, EMPTY_STRING) | ||
| } | ||
| } |
There was a problem hiding this comment.
| private suspend fun getNickname(): String? { | |
| return withContext(Dispatchers.IO) { | |
| mUserInfoPrefs.getString(NICKNAME_KEY_NAME, null) | |
| } | |
| } | |
| private suspend fun getRecoveryCode(): String? { | |
| return withContext(Dispatchers.IO) { | |
| mUserInfoPrefs.getString(RECOVERY_CODE_KEY_NAME, EMPTY_STRING) | |
| } | |
| } | |
| private suspend fun getNickname(): String { | |
| return withContext(Dispatchers.IO) { | |
| mUserInfoPrefs.getString(NICKNAME_KEY_NAME, EMPTY_STRING) ?: EMPTY_STRING | |
| } | |
| } | |
| private suspend fun getRecoveryCode(): String { | |
| return withContext(Dispatchers.IO) { | |
| mUserInfoPrefs.getString(RECOVERY_CODE_KEY_NAME, EMPTY_STRING) ?: EMPTY_STRING | |
| } | |
| } |
|
|
||
| private suspend fun getNickname(): String? { | ||
| return withContext(Dispatchers.IO) { | ||
| mUserInfoPrefs.getString(NICKNAME_KEY_NAME, null) |
There was a problem hiding this comment.
getNickname() 메서드에서만 defValue를 EMPTY_STRING이 아닌 null로 설정하신 이유가 있으신가용?
| @@ -21,8 +47,36 @@ class UserInfoPreferencesManager(context: Context) { | |||
| } | |||
| } | |||
|
|
|||
| suspend fun setMemberProfile(memberProfile: MemberProfile) { | |||
| setProfileImageUrl(memberProfile.profileImageUrl ?: "") | |||
There was a problem hiding this comment.
이 부분도 EMPTY_STRING을 사용하지 않으신 이유 있으실까용? 🧐
| <ImageView | ||
| android:id="@+id/iv_main_mypage" | ||
| android:layout_width="40dp" | ||
| android:layout_height="40dp" | ||
| android:clickable="true" | ||
| android:contentDescription="@string/main_mypage_btn_description" | ||
| android:focusable="true" | ||
| android:onClick="@{() -> handler.onMyPageClicked()}" | ||
| app:layout_constraintStart_toStartOf="parent" | ||
| app:layout_constraintTop_toTopOf="parent" | ||
| bind:coilCircleImageUrl="@{memberProfile.profileImageUrl}" | ||
| bind:coilPlaceHolder="@{@drawable/icon_member}" /> | ||
| </LinearLayout> | ||
| </androidx.constraintlayout.widget.ConstraintLayout> |
There was a problem hiding this comment.
ConstraintLayout을 활용하고 있기 때문에 LinearLayout은 사용하지 않아도 될 것 같아요~
아래와 같이 구현하면 ImageView만 있어도 동일하게 동작합니다!!
| <ImageView | |
| android:id="@+id/iv_main_mypage" | |
| android:layout_width="40dp" | |
| android:layout_height="40dp" | |
| android:clickable="true" | |
| android:contentDescription="@string/main_mypage_btn_description" | |
| android:focusable="true" | |
| android:onClick="@{() -> handler.onMyPageClicked()}" | |
| app:layout_constraintStart_toStartOf="parent" | |
| app:layout_constraintTop_toTopOf="parent" | |
| bind:coilCircleImageUrl="@{memberProfile.profileImageUrl}" | |
| bind:coilPlaceHolder="@{@drawable/icon_member}" /> | |
| </LinearLayout> | |
| </androidx.constraintlayout.widget.ConstraintLayout> | |
| <ImageView | |
| android:id="@+id/iv_main_mypage" | |
| android:layout_width="40dp" | |
| android:layout_height="40dp" | |
| android:layout_marginStart="10dp" | |
| android:layout_marginTop="10dp" | |
| android:clickable="true" | |
| android:contentDescription="@string/main_mypage_btn_description" | |
| android:focusable="true" | |
| android:onClick="@{() -> handler.onMyPageClicked()}" | |
| app:layout_constraintStart_toStartOf="parent" | |
| app:layout_constraintTop_toTopOf="parent" | |
| bind:coilCircleImageUrl="@{memberProfile.profileImageUrl}" | |
| bind:coilPlaceHolder="@{@drawable/icon_member}" | |
| tools:src="@drawable/icon_member" /> |
There was a problem hiding this comment.
사실 LinearLayout은 테두리를 주기 위함입니다!
너무 미세한 차이이긴 하지만요~..
어떤 게 더 맘에 드시나요?
| 테두리O | 테두리X |
|---|---|
 |
 |
 |
 |
There was a problem hiding this comment.
icon_member_with_border.xml는 삭제해도 될 것 같아요! icon_member와 동일한 아이콘입니다🙂
| setRecoveryCode(memberProfile.uuidCode) | ||
| } | ||
|
|
||
| suspend fun setProfileImageUrl(url: String) { |
There was a problem hiding this comment.
url은 null일 수 있으니 파라미터 타입을 nullable로 설정하면 더 명시적일 것 같아요!
| suspend fun setProfileImageUrl(url: String) { | |
| suspend fun setProfileImageUrl(url: String?) { |
| import com.on.staccato.databinding.ActivityMainBinding | ||
| import com.on.staccato.domain.model.MemberProfile |
There was a problem hiding this comment.
View와 Model 간의 의존성을 분리시키고, SharedPreferences의 활용성을 높일 수 있을 것 같습니다!
저도 이와 관련하여 MainActivity와 UserInfoPreferencesManager에 대한 의견을 남길 부분이 있는데, 몸 상태가 나아지면 생각을 정리해서 공유하겠습니다..! 죄송합니다 ㅠㅠ
➡️ 이와 관련된 코멘트 남겨뒀습니당! #603 (comment)
| @@ -11,7 +12,32 @@ class UserInfoPreferencesManager(context: Context) { | |||
|
|
|||
| suspend fun getToken(): String? { | |||
| return withContext(Dispatchers.IO) { | |||
| mUserInfoPrefs.getString(TOKEN_KEY_NAME, "") | |||
| mUserInfoPrefs.getString(TOKEN_KEY_NAME, EMPTY_STRING) | |||
There was a problem hiding this comment.
이 부분은 빈 문자열을 EMPTY_STRING이라는 이름으로 상수화해서 가독성이 떨어진다고 느껴질 수도 있을 것 같아요~
만약 상수화 하는 방향으로 간다면 좀 더 명시적인 이름으로 변경해봐도 좋을 것 같습니다!
ex) 값이 없는 상태를 뜻하는 단어, defValue라는 것을 뜻하는 단어 등
| private fun loadMemberProfile() { | ||
| lifecycleScope.launch { | ||
| val cachedMemberProfile = getCachedMemberProfile(this) | ||
| if (cachedMemberProfile.isValid()) { | ||
| sharedViewModel.setMemberProfile(cachedMemberProfile) | ||
| } else { | ||
| sharedViewModel.fetchMemberProfile() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private suspend fun getCachedMemberProfile(scope: CoroutineScope): MemberProfile { | ||
| return scope.async { | ||
| userInfoPrefsManager.getMemberProfile() | ||
| }.await() | ||
| } | ||
|
|
||
| private fun observeMemberProfile() { | ||
| sharedViewModel.memberProfile.observe(this) { memberProfile -> | ||
| lifecycleScope.launch { | ||
| userInfoPrefsManager.setMemberProfile(memberProfile) | ||
| } | ||
| binding.memberProfile = memberProfile | ||
| } | ||
| } |
There was a problem hiding this comment.
UserInfoPreferencesManager는 SharedPreferences를 활용해 데이터를 저장하고 가져오는 등의 역할을 담당하고 있습니다.
data layer 네이밍 컨벤션 가이드에 따르면, SharedPreferences를 사용하는 DataSource에 대해 네이밍을 언급하고 있는 것을 확인할 수 있습니다. 따라서 ~Manager라는 이름보다는 ~Repository 또는 ~DataSource(추상화에 따라 결정)로 네이밍하는 것이 더 명시적이라고 생각해요!
이제, UserInfoPreferencesManager를 ~Repository 또는 ~DataSource로 생각하고 현재 MainActivity를 살펴보겠습니다. 개인적으로 MainActivity에서 사용자의 정보를 불러오거나 저장하기 위해 UserInfoPreferencesManager의 함수를 직접 호출하는 것이 어색하게 느껴졌습니다.
이러한 문제점을 해결하기 위해 UserInfoPreferencesManager의 이름을 ~Manager가 아닌 ~Repository 또는 ~DataSource(추상화에 따라 결정)로 변경하고, MainActivity에서UserInfoPreferencesManager의 함수를 호출하는 부분을 ViewModel에서 처리하도록 변경하는 건 어떨까요? 🧐
cache된 MemberProfile은 ViewModel이 관리
ViewModel 내에서 cachedMemberProfile의 유효성을 검증하고 그 결과에 따라 MemberProfile set 또는 fetch
또한 UserInfoPreferencesManager 에서 Preferences라는 단어는 제거하면 어떨까요?
data layer 네이밍 컨벤션 가이드에 따르면, SharedPreferences를 datastore로 마이그레이션하는 등의 상황을 고려해 구현 세부사항을 이름에 포함하지 않는 것을 권장하고 있어요!
There was a problem hiding this comment.
MainActivity에서 사용자의 정보를 불러오거나 저장하기 위해 UserInfoPreferencesManager의 함수를 직접 호출하는 것이 어색하게 느껴졌습니다.
또한 UserInfoPreferencesManager 에서 Preferences라는 단어는 제거하면 어떨까요?
저도 해나의 의견에 동의합니다!
다만 현재 UserInfoPreferencesManager 클래스의 경우 해당 PR에 따라 Application에서 인스턴스로 만들어 앱 전역에서 사용할 수 있는 구조인데요..
위 제안에 따라 SharedPreference를 DataSource로 변경한다면 Application.sharedPreference처럼 직접 접근할 수 없고, ViewModel의 Repository를 통해서만 데이터를 CRUD 할 수 있는 구조로 변경해야겠네요.
제가 맞게 이해한 걸까요?.?
There was a problem hiding this comment.
위 제안에 따라 SharedPreference를 DataSource로 변경한다면 Application.sharedPreference처럼 직접 접근할 수 없고, ViewModel의 Repository를 통해서만 데이터를 CRUD 할 수 있는 구조로 변경해야겠네요.
제가 맞게 이해한 걸까요?.?
네~ 맞습니다😄
There was a problem hiding this comment.
해당 코멘트와 호두의 코멘트에 대한 부분 반영 완료했습니다!
변경 내용
SharedPreference를 MyPageLocalDataSource로 추상화하고
MyPageDefaultRepository에서 로컬 데이터와 서버 데이터를 적절히 처리하도록 했습니다.
class MyPageDefaultRepository
@Inject
constructor(
private val myPageLocalDataSource: MyPageLocalDataSource,
private val myPageRemoteDataSource: MyPageRemoteDataSource,
) : MyPageRepository {
// 로컬 or 서버에서 프로필 불러오기
override suspend fun getMemberProfile(): ApiResult<MemberProfile> {
val localProfile = myPageLocalDataSource.getMemberProfile() // 로컬 데이터를 불러온다
return if (localProfile.isValid()) { // 로컬 데이터가 유효하면
Success(localProfile) // 로컬 데이터 사용
} else { // 아니면
syncMemberProfile() // 서버 데이터 사용
}
}
// 로컬 프로필을 서버 프로필과 동기화
private suspend fun syncMemberProfile(): ApiResult<MemberProfile> {
return myPageRemoteDataSource.loadMemberProfile().handle { // 서버 프로필을 불러온다
val serverProfile = it.toDomain()
myPageLocalDataSource.updateMemberProfile(serverProfile) // 로컬 프로필을 업데이트 한다
serverProfile
}
}
// 로컬 & 서버 프로필 이미지 업데이트
override suspend fun changeProfileImage(profileImageFile: MultipartBody.Part): ApiResult<String?> =
myPageRemoteDataSource.updateProfileImage(profileImageFile).handle { // 서버 프로필 이미지를 업데이트 한다
val imageUrl = it.profileImageUrl
myPageLocalDataSource.updateProfileImageUrl(imageUrl) // 로컬 프로필 이미지를 업데이트 한다
imageUrl
}
}이로써 메인 화면과 마이페이지 화면의 Activity에서 SharedPreferences에 직접 접근하지 않고 ViewModel의 Repository를 통해 데이터를 읽고 쓸 수 있게 되었습니다.!
+)
Activity 이지만, domain의
MemberProfile모델이 import되어있어요...! 현재 구현 상 어쩔 수 없지만, View와 Model 사이의 의존성이 생겨 아쉽습니다.
UiModel을 적용해보는건 어떨까 했지만, SharedPreferences에 접근하여 데이터를 불러오고 설정하기 때문에, UiModel을 도입하는 것도 불필요하다고 느껴졌습니다.
@Junyoung-WON 의 코멘트 중 위 내용에 대한 질문입니다!
MemberProfile은 도메인 모델 객체 입니다.
저는 도메인 모델이 '도메인의 핵심 개념과 규칙을 객체로 표현한 것', '도메인을 개념적으로 표현한 것'이기에 특정 계층에 종속되지 않고 여러 곳에서 보편적으로 사용될 수 있다고 생각했습니다.
Activity에서 도메인 모델을 참조하는 것을 View와 Model 간의 의존성이 발생했다고 여겨 무조건 지양하는 게 맞을까? 하는 의문이 드는데, 호두는 어떤 이유로 참조해선 안된다고 생각하시는지 궁금합니다!
# Conflicts: # android/Staccato_AN/app/src/main/java/com/on/staccato/presentation/memorycreation/viewmodel/MemoryCreationViewModel.kt # android/Staccato_AN/app/src/main/java/com/on/staccato/presentation/memoryupdate/viewmodel/MemoryUpdateViewModel.kt # android/Staccato_AN/app/src/main/java/com/on/staccato/presentation/staccatocreation/viewmodel/StaccatoCreationViewModel.kt # android/Staccato_AN/app/src/main/java/com/on/staccato/presentation/staccatoupdate/viewmodel/StaccatoUpdateViewModel.kt
s6m1n
force-pushed
the
feature/#538-member-profile-image-change
branch
from
75bd753 to
ee371e3
Compare
Test Results 33 files 33 suites 6s ⏱️ Results for commit ee371e3. |
🌻Test Coverage Report
|
|
refactor: ApiResult의 제네릭 타입 제약 제거 커밋을 확인해주세요! 사용자 프로필이 없는 경우 ApiResult<String?>를 지원해야 함
interface MyPageRepository {
suspend fun getMemberProfile(): ApiResult<MemberProfile>
suspend fun changeProfileImage(profileImageFile: MultipartBody.Part): ApiResult<String?>
}
interface MyPageApiService {
// 기타 생략 ...
@Multipart
@POST(PROFILE_IMAGE_CHANGE_PATH)
suspend fun postProfileImageChange(
@Part imageFile: MultipartBody.Part,
): ApiResult<ProfileImageResponse>
}만약 사용자 프로필이 없다면, ProfileImageResponse의 profileImageUrl 값으로 null이 들어옵니다. @Serializable
data class ProfileImageResponse(
@SerialName("profileImageUrl") val profileImageUrl: String? = null,
)따라서 ApiResult의 T가 nullable 타입인 String?을 지원할 수 있도록 Any 타입 제약을 제거하였습니다. |
⭐️ Issue Number
🚩 Summary
화면별 변경 내용
타임라인
마이페이지
스타카토
주요 리팩토링
SharedPreference에 MemberProfile 정보 저장
기존에는 SharedPreference에서 사용자의 token만 저장하고 있었으며,
이번 PR에서 MemberProfile 도메인 모델의 아래 세 프로퍼티를 추가로 저장하도록 수정했습니다.
앱을 최초로 실행해 캐시된 멤버 프로필이 없는 상태에서의 흐름 요약
토큰을 SharedPreference에 저장한다.멤버 프로필을 SharedPreference에 저장한다.RESULT_OK일 때 SharedPreference의 멤버 프로필을 불러와 viewModel에 set하는 ActivityResultLauncher를 설정한다.프로필 이미지를 SharedPreference에 저장한다.RESULT_OK)를 수행한다.RESULT_OK이므로 메인 화면에서는 변경된 프로필이 보여진다.🙂 To Reviewer
멤버 관련 Repository 네이밍 수정 제안 (feat.백엔드)
MyPageRepository -> MemberProfileRepository
기존의 MyPageRepository는 아래 두 메서드를 갖고 있습니다.
기존에는 두 메서드 모두 마이페이지 화면에서 호출되었습니다.
그러나 멤버 프로필을 SharedPreference에 캐시로 저장하도록 리팩터링 하는 과정에서
getMemberProfile()의 호출 시점이 메인 화면으로 바뀌었습니다.이에 따라 MyPageRepository라는 이름이 부적절하다고 생각하였고, 해당 레포지토리에서
MemberProfile관련 데이터를 다룬다는 의미를 살려 MemberProfileRepository로 변경하는 것을 제안합니다.MemberRepository -> LoginRepository
또한, 기존의 MemberRepository는 아래와 같습니다.
복구 코드로 토큰을 불러오는 로직이 아이디와 비밀번호로 기존의 계정을 불러오는 '로그인'의 로직과 비슷해,
LoginRepository에 있는 게 더 적절할 것 같다는 생각이 듭니다.
LoginRepository -> SignUpRepository
또한 현재 LoginRepository의 loginWithNickname은 닉네임을 입력해 새 계정을 생성한다는 로직을 수행하므로,
로그인 보다는 오히려 회원가입(SignUp)에 가깝다고 생각합니다.
이 부분에 대해 안드로이드 내에서 1차로 논의해보고, 모두 동의한다면 백엔드에게 전달해 URL 변경을 요청해봐도 좋을 것 같습니다.
📋 To Do
글자 수 규칙 정책 논의 & 확정