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

FEAT #7

Open
wants to merge 3 commits into
base: develop-compose
Choose a base branch
from
Open

FEAT #7

wants to merge 3 commits into from

Conversation

twogarlic
Copy link
Collaborator

@twogarlic twogarlic commented May 4, 2024

시연 영상

compose.mp4

viewmodel 적용되어있습니다

@t1nm1ksun
Copy link
Member

고생하셨습니다!

Copy link

@Sangwook123 Sangwook123 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 ! 어떻게 하면 좀 더 가독성 있는 코드를 짤 수 있을지에 대해서 생각해보시면 좋을 것 같습니다 ! 언제든지 궁금한 점 물어봐주세요

}
val client = OkHttpClient.Builder().addInterceptor(loggingInterceptor).build()
//
val retrofit: Retrofit by lazy {

Choose a reason for hiding this comment

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

by lazy는 어떤 기능을 하나요?

Text("회원가입")
}
Button(
onClick = {

Choose a reason for hiding this comment

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

onClick 내부 코드들 함수화 시켜주시면 좋을 것 같습니다. 이 버튼이 어떤 역할을 하는지 함수명으로 직관으로 알려주면 좋을 것 같아요

Text(
text = "Hello $name!",
modifier = modifier
fun Scaffold(Id: String?, Password: String?, Nickname: String?, Mbti: String?,City: String?, memberId: String?) {

Choose a reason for hiding this comment

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

이 함수명이 Scaffold인 이유가 뭘까요?

verticalArrangement = Arrangement.spacedBy(16.dp),
) {
when(selectedItem) {
0 -> {

Choose a reason for hiding this comment

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

0, 1, 2같은 Type Code는 클래스로 치환하거나 enum을 활용하시는게 가독성과 유지보수 측면에서 이점이 있을 것 같습니다

Copy link
Member

@0se0 0se0 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
Member

Choose a reason for hiding this comment

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

여기두 각 기능별로 한 줄씩 띄워주시면 읽기 더 편할 거 같아요!

Comment on lines 109 to 129
if (response.isSuccessful) {
val loginResponse = response.body()
if (loginResponse?.code == 200) {
val userId = response.headers()["location"]
Toast.makeText(
context,
"로그인 성공! ID는 $userId 입니둥",
Toast.LENGTH_SHORT,
).show()
val intent = Intent(context, MainActivity::class.java).apply {
putExtra("memberId", userId)
}
context.startActivity(intent)

} else {
Toast.makeText(context, "로그인 실패! 아이디와 비밀번호를 다시 확인해주세요.", Toast.LENGTH_SHORT).show()
}
} else {
Toast.makeText(context, "로그인에 실패했습니다!", Toast.LENGTH_SHORT).show()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

if (response.isSuccessful) 이렇게 응답 성공 조건문이 있으니까
그 안에서 if (loginResponse?.code == 200) 한번 더 검사하지 않아도 될 거 같아요!!

Comment on lines +105 to +108
if (response.isSuccessful) {
Toast.makeText(context, "비밀번호가 변경되었습니다! 다시 로그인 해주세요", Toast.LENGTH_SHORT).show()
context.startActivity(Intent(context, LoginActivity::class.java))
} else {
Copy link
Member

Choose a reason for hiding this comment

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

ResponseChangePasswordDto를 통해 받아온 메세지를 출력하는 방식도 좋을 거 같아요!

Copy link

@l2hyunwoo l2hyunwoo left a comment

Choose a reason for hiding this comment

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

안녕하세요 세영님. 안드로이드 파트 명예 OB 이현우입니다. 만나서 반갑습니다.

앞으로의 과제들에 대한 코드리뷰를 맡게 되었습니다. 고민해보시다가 궁금한 점이 있으시면 지식 공유방/디스코드 연락 해주세요. 대면/비대면 자유형식으로 추가 피드백 가능합니다.

PR 내에 작업 요구사항/기능(코드) 변경점/추가 이슈 등을 적어주셔야 다른 개발자 분들이 코드리뷰를 하실때 이 점들을 참고하시면서 진행할 수 있습니다. 이 점 먼저 반영해주시면 감사하겠습니다.

import retrofit2.http.PATCH
import retrofit2.http.POST

interface AuthService {
Copy link
Member

Choose a reason for hiding this comment

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

API명세서에 회원정보에 관한 내용을 이용해 회원정보를 가져올 수 있을것 같아요!

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.

5 participants