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

Week4 필수과제 구현 #8

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Week4 필수과제 구현 #8

wants to merge 8 commits into from

Conversation

angryPodo
Copy link
Contributor

@angryPodo angryPodo commented Nov 15, 2024

Related issue 🛠

Work Description ✏️

  • 유저 등록(회원가입) API 연동
  • 로그인 API 연동
  • 내 취미 조회 API 연동

Screenshot 📸

4.mov
4.2.mp4

Uncompleted Tasks 😅

  • 취미 조회 응답 잘 오는데 UI 반영이 안됨

  • List 변경하기

To Reviewers 📢

이번에 API 적용하면서 아직까지도 파일 구조 분리에 대해 이해가 가지 않는 부분이 많아서 미흡한것 같습니다.
뭔가 규모가 큰 개발을 진행하거나 앞으로 개발을 해가면서 로직 분리의 필요성을 느낄 타이밍이 오면 그때서야 제대로 이해 할 것 같아요ㅠ

@angryPodo angryPodo linked an issue Nov 15, 2024 that may be closed by this pull request
3 tasks
@angryPodo angryPodo requested review from a team, chattymin, tunaunnie, jihyunniiii and t1nm1ksun and removed request for a team November 15, 2024 14:58
@angryPodo angryPodo self-assigned this Nov 15, 2024
Copy link

@t1nm1ksun t1nm1ksun left a comment

Choose a reason for hiding this comment

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

저는 처음할때 서버통신이 정말 어려웠는데 에러코드 별로 분기처리도 잘하시고
DataSource로 토큰도 잘 가지고 계시네요 ㄷㄷ
고생하셨습니다!! 합세 빠이팅~
(여행갔다와서 리뷰 마저 더 달수도 ㅋㅋ)

Comment on lines +78 to +96
response.code() == 400 -> {
val errorMessage = when(response.body()?.code) {
"00" -> "요청이 유효하지 않습니다"
"01" -> "입력값이 8자를 초과했습니다"
else -> "회원가입에 실패했습니다"
}
_uiState.update { it.copy(errorMessage = errorMessage) }
}
response.code() == 409 -> {
_uiState.update { it.copy(errorMessage = "이미 존재하는 username입니다") }
}
else -> {
_uiState.update { it.copy(errorMessage = "회원가입에 실패했습니다") }
}
}
true
} catch (e: Exception) {
_uiState.update { it.copy(errorMessage = "네트워크 오류가 발생했습니다") }
} finally {
_uiState.update { it.copy(isLoading = false) }

Choose a reason for hiding this comment

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

이야 잘해놓으셨네요!!

val route: Any
val route: String

Choose a reason for hiding this comment

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

난 아직도 왜 해결된건지 이유를 모름 ㅋㅋ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

진짜 모름...왜됨..?

Copy link
Contributor

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 +12
data class BaseResponse<T>(
@SerialName("result")
val result: T? = null,
@SerialName("code")
val code: String? = null
)

Choose a reason for hiding this comment

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

BaseResponse 쓰는거 좋은데요??

Copy link
Contributor Author

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 43
class AuthLocalDataSource(private val context: Context) {
private val tokenKey = stringPreferencesKey("token")

suspend fun saveToken(token: String) {
context.dataStore.edit { preferences ->
preferences[tokenKey] = token
}
}

fun getToken(): Flow<String?> = context.dataStore.data.map { preferences ->
preferences[tokenKey]
}

suspend fun clearToken() {
context.dataStore.edit { preferences ->
preferences.remove(tokenKey)
}
}

companion object {
@Volatile
private var instance: AuthLocalDataSource? = null

fun getInstance(context: Context): AuthLocalDataSource {
return instance ?: synchronized(this) {
instance ?: AuthLocalDataSource(context.applicationContext).also {
instance = it
}
}
}
}
}

Choose a reason for hiding this comment

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

실제 구현부분은 AuthLocalDataSourceImpl 라고 따로 구현하는것도 좋습니다!!
나중에 모듈로 binding해주거나
val context = navController.context val userInfoLocalDataSource = UserInfoLocalDataSourceImpl(context)
이렇게 선언한걸 뷰모델에 넣어서 사용하기도 합니다
제 코드에 힐트 적용하고나서 예시로 보여드릴게요!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

네! 아직 의존성 주입이 왜 필요한지 왜 쓰이는지 겪어보지 않아서 덜 와닿는거 같아요. DI더 공부해보고 저도 물어보겠습니다!

Copy link
Contributor

@chattymin chattymin left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 :)

이번주부터 합세 시작이라고 들었는데 화이팅이에요!

@@ -17,26 +16,21 @@ import org.sopt.and.presentation.navigation.NavGraph
@Composable
fun MainScreen() {
val navController = rememberNavController()
var bottomNaviVisible by remember { mutableStateOf(false) }
var userEmail by remember { mutableStateOf("") }
var isLoggedIn by remember { mutableStateOf(false) }
Copy link
Contributor

Choose a reason for hiding this comment

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

변수명에서 조금 더 고민을 해보면 좋을 것 같습니다.
현재는 로그인을 한다면 바텀바가 보이고 있지만, 추후에는 조건이 달라질 수 있어요.

그렇기 때문에 isLoggedIn과 같이 조건이 적혀있는 변수명을 사용한다면 추후 조건이 변경되었을 때 변수명도 변경해야 하는 경우가 생깁니다

Comment on lines +21 to +25
data class MyPageUiState(
val hobby: String = "",
val isLoading: Boolean = false,
val errorMessage: String? = null
)
Copy link
Contributor

Choose a reason for hiding this comment

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

이 class가 ViewModel 내부에 존재하는 이유가 있을까요?

Comment on lines +27 to +31
init {
fetchMyHobby()
}

fun fetchMyHobby() {
Copy link
Contributor

Choose a reason for hiding this comment

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

뷰모델 내부에서만 호출한다면 private 접근제한자를 붙이면 더욱 안전하게 사용할 수 있어요

Comment on lines +73 to +97
} catch (e: Exception) {
_uiState.update {
it.copy(
errorMessage = "네트워크 오류가 발생했습니다",
isLoading = false
)
}
}
} else {
_uiState.update {
it.copy(
errorMessage = "로그인이 필요합니다",
isLoading = false
)
}
}
}
} catch (e: Exception) {
_uiState.update {
it.copy(
errorMessage = "토큰 조회에 실패했습니다",
isLoading = false
)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

catch를 할 때에는 Exception의 종류에 대해서 명확하게 작성해주시는게 좋습니다. 그래야 추후 오류를 파악하고 수정하기에 좋아요

그리고 모든 부분의 마지막에 isLoading = false라는 코드를 작성해주고 있는데 이는 finally로 빼는건 어떨까요? 중복 코드를 줄일 수 있을 것 같아요.

isError = uiState.errorMessage?.contains("이메일") == true
TextInputField(
value = uiState.username,
onValueChange = { signInViewModel.onUsernameChange(it) },
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onValueChange = { signInViewModel.onUsernameChange(it) },
onValueChange = signInViewModel::onUsernameChange,

이런 방법으로도 작성할 수 있어요. 컴포즈에서는 이러한 방법이 확실한 장점이 있는 반면, 단점도 있어요. 한번 찾아보시고 상황에 맞게 사용하시면 좋을 것 같네요

Choose a reason for hiding this comment

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

AuthRequest 파일 안에 SignUpRequest, SignInRequest 을 한번데 넣어서 관리하시는군요..! 나중에 유저 인증 관련 기능이 더 복잡해질 걸 생각하면, 이런 관리 방식은 굉장히 효율적인 것 같아요!!

Choose a reason for hiding this comment

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

AuthResponse 안에 응답 관련 클래스도 다 넣어두니 훨씬 깔끔하네요!!

if (token != null) {
try {
val response = ServicePool.authService.getMyHobby(token)
when {

Choose a reason for hiding this comment

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

와 분기 처리가 엄청 자세해서 거의 모범 코드 같아요... 보고 많이 배워갑니다!! 이번 과제 수정할 때 참고해볼게용

Copy link

@tunaunnie tunaunnie left a comment

Choose a reason for hiding this comment

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

늘 열심히 하시는 팀원이 같은 조에 계셔서 든든합니다👍 서버통신까지 높은 수준으로 완성해 보고자 노력하신 게 느껴져요!! 고생하셨습니당

Copy link
Contributor

@jihyunniiii jihyunniiii 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 +101 to +102
implementation("androidx.datastore:datastore-preferences:1.0.0")
implementation("androidx.datastore:datastore-preferences-core:1.0.0")
Copy link
Contributor

Choose a reason for hiding this comment

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

여기도 버전 카탈로그 적용해주시면 좋을 것 같네요!

preferences[tokenKey]
}

suspend fun clearToken() {
Copy link
Contributor

Choose a reason for hiding this comment

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

일반 함수와 suspend 함수의 차이는 무엇일까요?

authLocalDataSource = AuthLocalDataSource.getInstance(LocalContext.current)
)
),
modifier: Modifier = Modifier
Copy link
Contributor

Choose a reason for hiding this comment

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

Modifer는 최상단에 올려줍시다.
순서 : 필수 인자 -> Modifier (선택 인자 중에서 가장 처음이어야 함) -> 선택 인자

Comment on lines +27 to +29
init {
fetchMyHobby()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

init 블럭은 어떤 식으로 동작할까요?
어떤 로직을 담으면 좋고, 어떤 로직을 담지 않아야 할까요?

val route: Any
val route: String
Copy link
Contributor

Choose a reason for hiding this comment

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

무슨 문제가 있었는데 !!

Comment on lines +76 to +78
"01" -> "요청이 유효하지 않습니다"
"02" -> "로그인 정보가 올바르지 않습니다"
else -> "로그인에 실패했습니다"
Copy link
Contributor

Choose a reason for hiding this comment

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

요런 친구들도 상수화 해주시면 좋을 것 같네요!

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.

[FEAT] 4차 과제
5 participants