Skip to content

[Week6] 기존 과제 코루틴으로 변경하기 #12

Open
hyeminililo wants to merge 67 commits intomainfrom
week6
Open

[Week6] 기존 과제 코루틴으로 변경하기 #12
hyeminililo wants to merge 67 commits intomainfrom
week6

Conversation

@hyeminililo
Copy link
Collaborator

🫛 작업한 내용

  • week4 코드 리뷰 리팩토링
  • fetchUser(), Login(), Signup() 코루틴으로 변경

🫛 PR 포인트
아래 궁금하고 어려웠던 점 부분을 위주로 봐주시면 감사하겠습니다 🙇🏻‍♀️

📸 스크린샷

week6_251128_233718.mp4

🤔 궁금하고 어려웠던 점
일단 방금 ,, 영상을 촬영하다가 ,, fetchUser()가 제대로 동작하지 않고 있다는 것을 확인해버렸습니다 ,, 오류를 찾고 다시 추가 커밋을 하겠습니다 :)

📮 관련 이슈
#11

dquote> 기존 Intent로 넘겨주던 부분을 User 객체로 형성해 데이터 전달
Lazycolumn으로 홈스크린 구현
PassordVisualTransformation 적용 안 한 버그 수정
기존 아이디, 비밀번호 입력 후, 닉네임, 생일 등 입력 불편함 개선
기존 ProfileList로 관리하던 유저 정보를 viewModel을 이용해 적용
y축을 기준으로 돌아가는 애니메이션 구현헀는데, 누를 때마다 front, back으로 바로 변하지 않은 버그 발생
Copy link
Member

@sonms sonms left a comment

Choose a reason for hiding this comment

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

항상 열심히 하시는 모습이 너무 보기 좋아요! 이번 합세에서 배운 내용을 토대로 api연결법을 잘 습득해 오신 것 같아 더 좋습니다 :)
조금 더 읽고 추가적으로 리뷰 달도록 하겠습니다!

@POST("api/v1/auth/login")
suspend fun login(
@Body request: RequestLoginDto
): Response<ResponseSuccessDto<LoginDataDto>>
Copy link
Member

Choose a reason for hiding this comment

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

이번 합세에서 배웠을 지 모르겠지만 BaseResponse 를 사용해 봅시다~ wrapping이 너무 많이 되어있네요!

import kotlinx.serialization.SerialName
import kotlinx.serialization.Serializable

enum class Status { ACTIVE }
Copy link
Member

Choose a reason for hiding this comment

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

혹시 따로 이렇게 1개의 status를 data에서 만들어 준 이유가 있을까요?
이런 상태는 ui에서 사용될 거 같아요
서버에서 받은 내용을 가공하여 내려주는 역할은 dto가 아닌것 같습니다!
7차 세미나에서 배운대로 간단 변환의 책임이나 가공은 어디가 좋을 지 생각해보시면 좋을 것 같아요!

contentColor = Color.White
),
) {
Text(textMessage, modifier)
Copy link
Member

Choose a reason for hiding this comment

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

사용하지 않는 text의 modifier 는 지워도 괜찮을 것 같아요!

Comment on lines +40 to +44
enum class CardState(val imageResId: Int) {
Front(com.sopt.dive.R.drawable.card_front_image),
Back(com.sopt.dive.R.drawable.card_image)
}

Copy link
Member

Choose a reason for hiding this comment

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

이것도 한 파일에서 사용되기 보다는 분리하는게 더 좋아 보여요!

Choose a reason for hiding this comment

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

저도 이런 타입을 나타내는 enum class 와 같은 코드는 type 패키지 안에 분리하는거 같아요 ~!

@hyeminililo hyeminililo requested review from jyvnee, seungjae708 and seungjunGong and removed request for oilbeaneda and sohee6989 November 30, 2025 11:26
Copy link

@jyvnee jyvnee left a comment

Choose a reason for hiding this comment

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

수고햇다아~~~~~ 🤩🤩🤩

@Composable
fun LoginScreen(
userViewModel: UserViewModel,
onLoginSuccess: () -> Unit,
Copy link

Choose a reason for hiding this comment

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

onLoginSuccess는 어디서 사용하고 있나요??


@Composable
fun ProfileScreen(paddingValues: PaddingValues, userViewModel: UserViewModel) {
val user by userViewModel.currentUser.collectAsState()
Copy link

Choose a reason for hiding this comment

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

여기 collectAsState 사용하셨네요!!! collectAsStateWithLifecycle 이랑 차이 배웠는데!

} else null

val emailError =
if (ageText.isNotBlank() && !InputValidators.isValidEmail(emailText)) {
Copy link

Choose a reason for hiding this comment

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

ageText 검사하고 있다!

Comment on lines +139 to +151
if (
idError == null && pwError == null && nameError == null && emailError == null && ageError == null &&
idText.isNotBlank() && pwText.isNotBlank() && nameText.isNotBlank() && emailText.isNotBlank() && ageText.isNotBlank()
) {
val request =
RequestSignupDto(idText, pwText, nameText, emailText, ageText.toInt())

userViewModel.signUpUser(request)
Toast.makeText(context, "회원가입 완료! 로그인 화면으로 돌아갑니다.", Toast.LENGTH_SHORT).show()
onSignUpComplete()
} else {
Toast.makeText(context, "모든 정보를 입력해주세요", Toast.LENGTH_SHORT).show()
}
Copy link

Choose a reason for hiding this comment

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

이런 로직 처리 뷰모델에서 안 하고 Screen에서 하는 이유가 있나요?? 그리고 회원가입 성공 여부 없이 로그인 화면으로 가고 있는데 성공/실패 응답을 확인해야 하지 않을까요..??

Copy link

@seungjunGong seungjunGong left a comment

Choose a reason for hiding this comment

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

고생하셨습니다ㅏ ~~ 이전 커밋까지 이번 pr 에 올라가게 된거 같은데 기존에 dev 브랜치에 merge 하셨던거 같아요 이번에 main 브랜치로 설정되어 있어서 ! 혹시 의도한것이 아니라면 dev 브랜치로 머지되도록 수정해야할거 같네요 ㅎ.ㅎ

alias(libs.plugins.kotlin.android)
alias(libs.plugins.kotlin.compose)
alias(libs.plugins.kotlin.serialization)
id("kotlin-parcelize")

Choose a reason for hiding this comment

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

p2: 현재 방식은 버전을 명시하지 않고 사용하기에 프로젝트 버전 관리를 하기 어려워요 이친구도 versions.toml 에 plugins 에 정의해서 사용해 보는 것두 추천드립니다 ㅎ

kotlin-parcelize = { id = "org.jetbrains.kotlin.plugin.parcelize", version.ref = "kotlinParcelize" }

singleLine = true,
keyboardOptions = keyboardOptions,
visualTransformation = visualTransformation

Choose a reason for hiding this comment

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

아주 사소한거긴 한데 이런 줄바꿈은 없애는게 좋습니다 ㅎ

Comment on lines +40 to +44
enum class CardState(val imageResId: Int) {
Front(com.sopt.dive.R.drawable.card_front_image),
Back(com.sopt.dive.R.drawable.card_image)
}

Choose a reason for hiding this comment

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

저도 이런 타입을 나타내는 enum class 와 같은 코드는 type 패키지 안에 분리하는거 같아요 ~!


@Composable
fun HomeScreen(paddingValues: PaddingValues, viewModel: MainViewModel = viewModel()) {
val homeProfileList by viewModel.homeProfileList.collectAsState()

Choose a reason for hiding this comment

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

지난 6주차 세미나 때 배운 내용 중 하나인 collectAsState 와 collectAsStateWithLifecycle 을 비교 해보시고 뭘 적용하면 좋을지 알아보는 것두 좋을거 같습니다 ~

Comment on lines +41 to +49
Text(text = "ID: ${user?.id}", fontSize = 20.sp)

Text(text = "PW: ${user?.pw}", fontSize = 20.sp)

Text(text = "Name: ${user?.name}", fontSize = 20.sp)

Text(text = "Email: ${user?.email}", fontSize = 20.sp)

Text(text = "Age: ${user?.age}", fontSize = 20.sp)

Choose a reason for hiding this comment

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

현재는 각 Text 컴포넌트마다 개별적으로 null 체크를 하고 있는데
이 방식보다는 상위 레벨에서 user 대한 null 체크를 먼저 수행한 뒤
하위 UI를 그리는 방식이 더 명확하고 안전합니다 !

Spacer(modifier = Modifier.height(40.dp))

Button(
onClick = { userViewModel.logoutUser() },

Choose a reason for hiding this comment

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

이처럼 단순한 함수 호출이라면 람다를 새로 생성하기보다는 함수 참조를 그대로 전달하는 편이 더 좋습니다.
람다는 호출 시마다 람다 객체를 생성하기 때문에 함수 참조에 비해 불필요한 리소스를 더 사용하게 돼요 !!
Ex. userViewModel::logoutUser,

Suggested change
onClick = { userViewModel.logoutUser() },
onClick = userViewModel::logoutUser,

userViewModel = userViewModel,
onSignUpComplete =
{
navController.popBackStack(Route.Login.path, inclusive = false)

Choose a reason for hiding this comment

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

화면의 스택을 제거하는 방법으로 popBackStack 과 navigateUp 이 있는데요 ! 이 둘의 차이는 무엇일까요?

Comment on lines +25 to +27
companion object {
private const val USER = "user"
}

Choose a reason for hiding this comment

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

그거 아세요 ~ companion object 는 클래스의 맨 아래에 배치하는게 컨벤션이라고 합니다 저도 이번에 알았어요 ㅋㅎㅋㅋㅎ

Copy link

@seungjae708 seungjae708 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다~ 이번 주에 배운 아키텍처를 고민해서 같이 적용해보면 좋을것 같아요!
앱잼까지 화이팅 !

): Response<ResponseSuccessDto<ResponseUserDto>>

@GET("api/v1/users/{id}")
fun fetchUserInfo(

Choose a reason for hiding this comment

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

suspend 키워드 빠진 거 같아요!


@Composable
fun UserProfileCard(user: HomeProfileInfo, modifier: Modifier = Modifier) {
var isFollowing by remember { mutableStateOf(false) }

Choose a reason for hiding this comment

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

상태를 직접 관리하고 있는데 상위 컴포넌트로 호이스팅해주는 건 어떨까요?

import com.sopt.dive.ui.components.UserProfileCard

@Composable
fun HomeScreen(paddingValues: PaddingValues, viewModel: MainViewModel = viewModel()) {

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants