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/seminar7 basic #9

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

Feat/seminar7 basic #9

wants to merge 17 commits into from

Conversation

codingmy
Copy link
Contributor

@codingmy codingmy commented Dec 22, 2023

📌𝘐𝘴𝘴𝘶𝘦𝘴

📎𝘞𝘰𝘳𝘬 𝘋𝘦𝘴𝘤𝘳𝘪𝘱𝘵𝘪𝘰𝘯

  • ui state
  • databinding
  • viewModel 추가 구현
  • 코리 반영
  • singleton
  • 로그인, 가입 둘다 진행했슴니다

📷𝘚𝘤𝘳𝘦𝘦𝘯𝘴𝘩𝘰𝘵

Screen_Recording_20231222_192833_DoSoptTemplate.mp4

💬𝘛𝘰 𝘙𝘦𝘷𝘪𝘦𝘸𝘦𝘳𝘴

안봉이들 모두 8주차 동안 진짜진짜 수고 많았어!!!!!(빙봉들 진짜 👍👍 )
이태희 안팟장님 덕분에 정상에 올라왔습니다...진짜 짱....빛 그자체

@codingmy codingmy requested review from taeheeL and a team December 22, 2023 10:35
@codingmy codingmy self-assigned this Dec 22, 2023
@codingmy codingmy added ⭐ [FEAT] 새로운 기능 구현 ✅ [MOD] 코드 수정 및 내부 파일 수정 ➕ [ADD] 부수적인 코드 추가 및 라이브러리 추가, 새로운 파일 생성 ♻️ [REFACTOR] 코드 리팩토링 최민영 민영 labels Dec 22, 2023
Copy link
Member

@chanubc chanubc 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 3 to 11
object UserInfo {
var userInfoList = User(
nick = "",
self_description = "",
id = "",
pw = "",
mbti = "",
)
}
Copy link
Member

Choose a reason for hiding this comment

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

여기서 mapping함수를 추가하면 좋습니다!

Copy link
Contributor Author

@codingmy codingmy Jan 2, 2024

Choose a reason for hiding this comment

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

매핑함수를 공부한지 얼마 안돼서 일단 object에 mapping 함수를 만들어서 반영해봤는데...data class에서 정의하는게 더 적절한거겠죠...? 다시 옮겨보겠습니당 0489e44 아니다 일단 여기에서 매핑함수를 제대로 쓴게 맞나요?? 뭔가 다시 펼쳐보니까 잘쓴건지 긴가민가 하네용...!

Copy link
Member

Choose a reason for hiding this comment

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

제가쓴 mapping함수랑 클린아키텍쳐나 ob들이 사용했던 map함수의 목적이 달라서,,
ob들 코드를 따라가는게 맞는 것 같습니다! 전 단순 초기화랑 placeable를 용이하게 하기 위한 용도 였어요.

Comment on lines +43 to +62
///*
// ServicePool.authService.postLogin(RequestLoginDto(id, password))
// .enqueue(object : Callback<ResponseLoginDto> {
// override fun onResponse(
// call: Call<ResponseLoginDto>,
// response: Response<ResponseLoginDto>,
// ) {
// if (response.isSuccessful) {
// val data: ResponseLoginDto = requireNotNull(response.body()!!)
//
//
// val userId: Int = data.id
// loginSuccess.value = true
// toast("로그인 성공")
// val intent = Intent(context, MainHomeActivity::class.java)
// intent.putExtra("id", userId)
//
//
// startActivity(intent)
// } else {
Copy link
Member

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.

동의합니당!

Copy link

@m6z1 m6z1 left a comment

Choose a reason for hiding this comment

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

민영빙봉 8주동안 고생많아써요 🫶🫶🫶

Copy link
Contributor

@librarywon librarywon 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 +5 to +9
nick = "",
self_description = "",
id = "",
pw = "",
mbti = "",
Copy link
Contributor

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.

왜 꼭 디폴트 값을 주어야 할지 nonNull을 주면 안되는지 생각해보면 좋을 것 같아서요 답은 프로젝트의 성격에 따라 다릅니다 string의 기본값은 ""을 대부분 사용해요

Comment on lines 9 to 12
val username: String,
var username: String,

@SerialName("password")
val password: String,
var password: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

왜 val에서 var로 변경하였나요?

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 17 to 30
@POST("api/v1/members/sign-in")
fun postLogin(
fun postLogin(
@Body request: RequestLoginDto,
): Call<ResponseLoginDto>
): Response<ResponseLoginDto>

@POST("api/v1/members")
fun signUp(
fun postSignUp(
@Body request: RequestSignUpDto,
): Call<Unit>
): Response<Unit>

@GET("api/v1/members/{memberId}")
fun getUserInfo(
@Path("memberId") memberId: Int
): Call<ResponseUserDataDto>
): Response<ResponseUserDataDto>
Copy link
Contributor

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.

suspend가 안붙었는데 적용 안한거 같은데

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 Author

Choose a reason for hiding this comment

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

확인해봤는데 일단 안스에서는 다 누락없이 푸시하고 깃헙에도 올라간거 같은데 이 코리 창에서는 변경된 부분이 안보이네요...! 저번엔 됐었던거 같은데... 🤔🤔 혹시 깃헙 pr 올린 뒤에 커밋 올리면 이미 이전에 코리한 코드창에는 반영이 안되던가요...?일단 0ed0212 요 커밋을 봐주시죠~~!~! 코루틴 이렇게하면 이번엔 진짜로 적용된거 맞죠...???!

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 104 to 106
/*


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 20 to 21
private val _loginState = MutableStateFlow<LoginState>(LoginState.Loading)
val loginState: StateFlow<LoginState> = _loginState.asStateFlow()
Copy link
Contributor

Choose a reason for hiding this comment

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

찬우형 코드리뷰에서도 똑같은 리뷰를 달았는데 Ui State의 기본값을 init이나 loading 말고 다른 것을 주는 것이 어떨까요? 아래는 찬우형한테 해준 코리 복사해왔습니다!

처음 UiState의 기본 상태를 Loading으로 두는 것 같은데 init State상태를 추가해주시는 것이 좋을 것 같아요
loading State 은 실제로 서버와 통신을 진행하는 동안 스캘레톤 로딩을 보여주거나 로딩 화면을 보여줄 때를 주로 정의합니다.

Copy link
Contributor Author

@codingmy codingmy Jan 2, 2024

Choose a reason for hiding this comment

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

f237bd4 에서 Unstarted 상태를 추가해서 기본값은 Unstarted으로, 서버통신 시작시에 Loading으로 수정하도록해서 실제 서버통신 동안만 Loading값을 갖도록 수정해봤습니다!

val loginState: StateFlow<LoginState> = _loginState.asStateFlow()


fun checkLoginAvailableFromServer(id: String, password: String) {
Copy link
Contributor

Choose a reason for hiding this comment

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

개인 취향이지만 Available은 빼도 되지 않을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

안그래도 네이밍이 너무 길다고 느껴졌는데 그게 좋은거 같아요!!

@@ -49,7 +49,7 @@ class MainHomeActivity : AppCompatActivity() {
private fun clickBottomNavigation() {
var userId = intent.getIntExtra("id", -1)

mainHomeViewModel.userId=userId
// mainHomeViewModel.userId=userId
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 24 to +25
private val _navigateTo = MutableLiveData<Fragment>()
val navigateTo: LiveData<Fragment>
get() = _navigateTo
val navigateTo: LiveData<Fragment> get() = _navigateTo
Copy link
Contributor

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.

이 친구는 아마 하단 네비게이션 부분이 터치 인식됐을 때, viewModel에서 어떤 fragment를 보여줘야되는지 naviagetTo에 저장하고, activity에서 navigateTo값을 observe해서 fragment를 바꾸는 목적으로 사용했습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

viewModel에서 어떤 프레그먼트로 이동할지 알 필요가 있을까요?
프레그먼트 전환은 view랑 viewModel중에 누가 책임을 가지고 있어야할까요? MVVM에 대해서 공부해보시면 좋을 것 같습니다.

})
private fun myPageUserInfo() {
_userId.value = UserInfo.userInfoList.id
userPw.value = UserInfo.userInfoList.pw
Copy link
Contributor

Choose a reason for hiding this comment

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

얘도 _userPw 만들어서 처리하면 좋을 것 같아요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

근데 userPw는 이 뷰모델에서 User로 값 복붙 해주는거 밖에 안하는데 그래도 _userPw 만들어 놓는게 나을까요??

Copy link
Contributor

Choose a reason for hiding this comment

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

복붙밖에 안한다면 private로 만드는게 좋지 않을까요?

@@ -231,28 +226,15 @@ class SignUpActivity : AppCompatActivity() {
// return userInfoList
// }


private fun signUp() = with(binding) {
fun signUp() = with(binding) {
Copy link
Contributor

Choose a reason for hiding this comment

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

signUp() 함수가 반환값이 있지 않으면 일반적인 함수 선언형태로 변경해주시면 좋을 것 같습니다

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0ed0212 여기에서 같이 반영했습니당!!

toast("로그인 실패")
}

is LoginState.Loading -> {}
Copy link
Contributor

Choose a reason for hiding this comment

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

이 State는 어떤 역할을 하고 있는건가요?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
➕ [ADD] 부수적인 코드 추가 및 라이브러리 추가, 새로운 파일 생성 ⭐ [FEAT] 새로운 기능 구현 ✅ [MOD] 코드 수정 및 내부 파일 수정 ♻️ [REFACTOR] 코드 리팩토링 최민영 민영
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[7차 세미나] :
4 participants