[Week4] 4주차 필수 과제 #12

hyeeum · 2024-11-14T18:08:31Z

Related issue 🛠

closed [Feature] 4주차 필수 과제 #11

Work Description ✏️

[유저 등록]
- 회원가입 페이지에 진행해주세요.
- 기존에 이메일, 비밀번호를 받는 구조에서 username, password, hobby를 입력하는 구조로 변경해주세요.
- username, password, hobby가 조건을 만족하지 않는 경우 (8자 이상인 경우) 조건을 만족하지 않았다는 것을 화면에 표시해주세요. (방식은 자율입니다.)

[로그인]
- 로그인 페이지에 진행해주세요.
- 기존 이메일, 비밀번호를 받는 구조에서 username, password를 받는 구조로 변경해주세요.
- 로그인 실패, 성공 시에 알맞은 안내 문구를 유저에게 제시해주세요. (방식은 자율입니다.)

[내 취미 조회]
- MY 페이지에 진행해주세요
- 기존에 이메일을 표시했던 부분에 내 취미가 표시되도록 해주세요.

Screenshot 📸

이름 오류	비밀번호 오류	취미 오류	정상 회원가입

로그인 오류	정상 로그인

취미 불러오기

To Reviewers 📢

네이밍 붙이는게 참어려운 것 같아요 , signup signin을 data layer코드에 넣는게 맞는지 뭔가 하면서도 이건 아닌 것 같다는 느낌이 드는데 다른 분들의 코드를 많이 참고하면서 더 좋은 네이밍을 알아보도록 하겠습니다 : - )
그리고 서버통신은 한 번 하면 감이 잡히기는하는데 체화를 하는 과정이 참 오래 걸리는 것 같아요 패키지를 분리하는 과정에서 다시 한 번 아키텍처 정리를 할 수 있었습니다..! 까먹지않도록 계속 봐야할 것 같네요

greedy0110

네트워크 연결 과제 고생 많으셧습니다 혜음님, 간단히 코멘트 남겨두었어요.

greedy0110 · 2024-11-16T09:01:10Z

app/src/main/java/org/sopt/and/data/di/NetworkModule.kt

+        return OkHttpClient.Builder()
+            .addInterceptor(loggingInterceptor)
+            .addInterceptor(authInterceptor)
+            .build()


릴리즈에는 필요없는 interceptor이므로 디버그 앱에만 추가해주심이 어떨까요

Suggested change

return OkHttpClient.Builder()

.addInterceptor(loggingInterceptor)

.addInterceptor(authInterceptor)

.build()

val builder = OkHttpClient.Builder()

if (BuildConfig.DEBUG) builder.addInterceptor(loggingInterceptor)

builder.addInterceptor(authInterceptor)

return builder.build()

꼼꼼하게 봐주셔서 감사합니다! 릴리즈 디버그 구분해서 사용할 생각은 못했네요!

greedy0110 · 2024-11-16T09:05:31Z

app/src/main/java/org/sopt/and/feature/my/MyViewModel.kt

+        viewModelScope.launch {
+            wavveRepository.getHobby().onSuccess { hobbyEntity ->
+                _state.value = _state.value.copy(
+                    hobby = hobbyEntity.hobby
+                )
+            }.onFailure { }


실패하면 사용자는 아무런 피드백을 받지 못하고 빈 화면을 보게 될 것 같습니다.
앱을 사용하는 사람 입장으로 생각하면 피하고 싶은 상황일 것 같아요.
최소한 토스트로 문제가 생겼음을 보여주거나 뭔가 문제가 있으니 재시도 하라는 화면이 있으면 좋을 것 같습니다.

클라이언트 개발자로서 클라이언트를 고려하지 않은 점이 두드러지는 것 같네요 :( 해당 사항고려해서 적절한 이벤트를 추가하도록 하겠습니다

greedy0110 · 2024-11-16T09:08:34Z

app/src/main/java/org/sopt/and/sharedpreference/User.kt

+import javax.inject.Singleton
+
+@Singleton
+class User @Inject constructor(


모듈이 분리된 이 시점에서 이 객체는 다분히 data 혹은 repository 수준에서 관리될 친구로 보이네요.

data로 관리한다면 UserToken 정보를 디바이스에 저장하는 객체로 변모할 것이고 (현재 프로젝트 구조 상 이름은 UserDataSource, TokenDataSource ?)

repository로 관리한다면 UserToken 정보를 저장하고, 필요한 시점에 초기화 하는 인터페이스를 제공하는 객체로 활용되겠네요. (현재 프로젝트 구조에 맞는 이름은 아마도 UserRepository? AuthRepository? SignRepository)

User 를 어떻게 분류해야할지 고민이 있었는데 말씀해주신 내용 바탕으로 고민해보고 수정해보겠습니다,감사합니다! : )

kamja0510

역시 혜음.. 저같은 사람도 코드를 알아볼 수 있게 짜셨네요(mvi는 살짝 이해못함 ㅎ)
항상 코드리뷰 시간이 아니라 보면서 공부하는 느낌

kamja0510 · 2024-11-17T08:07:01Z

app/src/main/java/org/sopt/and/data/di/NetworkModule.kt

+    @Provides
+    fun provideLoggingInterceptor(): HttpLoggingInterceptor {
+        return HttpLoggingInterceptor { message ->
+            Log.d("Retrofit2", "CONNECTION INFO -> $message")


세미나에서 배운것은 이 중괄호 블럭이 없는데 이 블럭이 추가되면서 어떤 추가적인 정보를 볼 수 있나요?

사실 이 블럭 안에 코드가 없어도 로그캣에 인터셉터 내용이 잘 출력되지만 조금 더 로그 잘보이게 하려고 로그 커스텀 했어요 ㅋㅋ

kamja0510 · 2024-11-17T08:08:19Z

app/src/main/java/org/sopt/and/data/dto/request/RequestSignUpDto.kt

+
+@Serializable
+data class RequestSignUpDto(
+    @SerialName("username")


변수명과 json key명이 같은데도 SerialName 어노테이션을 굳이 써야하나요?

저는 사실 그냥 습관적으로 사용했었는데요. 최근에 새롭게 알게 된 사실이 있어요.
추후에 클라에서 암호화해서 서버통신을 하게된다면 어노테이션이 없을 경우 서버가 해석을 못한다고 하더라고요!!
함께 알아가시면 좋을 것 같네요...신기방구

kamja0510 · 2024-11-17T08:09:19Z

app/src/main/java/org/sopt/and/data/dto/response/ResponseUserHobbyDto.kt

+    @SerialName("hobby")
+    val hobby: String
+){
+    fun toEntity() = ResponseHobbyEntity(


신기한 문법들

kamja0510 · 2024-11-17T08:13:11Z

app/src/main/java/org/sopt/and/feature/search/SearchNavigation.kt


 fun NavController.navigateSearch(
    navOptions: NavOptions
 ) {
-    navigate(Search, navOptions)
+    navigate(org.sopt.and.feature.search.Search, navOptions)


오잉 왜 갑자기 경로 다 줘야되는걸로 바뀌었나요

어 그러게요 이거 뭐야

kamja0510 · 2024-11-17T08:40:30Z

app/src/main/java/org/sopt/and/feature/signin/SignInState.kt

+    val password: String = "",
+    var isPasswordVisible: Boolean = false,
+) {
+    val isButtonEnabled: Boolean = username.isNotEmpty() && password.isNotEmpty()


이 줄이 왜 중괄호 블럭으로 따로 빠져 있나요? 저 username이랑 password를 쓰려면 따로 빼야하는 건가요?

username password를 사용하기 위해서 뺀 건 아니고, username password가 바뀔때마다 isButtonEnabled도 함께 값이 변경되어야하므로 따로 뺀 거랍니다! 뷰모델에서 작성하는 방법도 있긴한데 저는 간단한 로직이라서 State안에다 뒀어요!

kamja0510 · 2024-11-17T08:43:44Z

app/src/main/java/org/sopt/and/feature/signup/SignUpViewModel.kt

@@ -72,9 +94,8 @@ class SignUpViewModel @Inject constructor() : ViewModel() {
    }

    companion object {
-        private const val MIN_PASSWORD = 8
-        private const val MAX_PASSWORD = 20
+        private const val MIN_SIGNUP_LENGTH = 1


와 저는 상수화도 안하고 최소값1로 두지도 않았네요 혼나겠다

kamja0510 · 2024-11-17T08:45:31Z

app/src/main/java/org/sopt/and/sharedpreference/User.kt

+@Singleton
+class User @Inject constructor(
+    @ApplicationContext private val context: Context
+) {
    private val sharedPreferences: SharedPreferences =


호옥시 datastore 안쓰시고 sharedPreferences 쓰시는 이유가 있나요!

아뇨 ㅠㅠ 딱히 이유없어요... 사실 datastore안써봄

kamja0510 · 2024-11-17T08:47:02Z

app/src/main/res/values/strings.xml

사소한 부분들도 string들을 다 바꾸셨네요!

kamja0510 · 2024-11-17T08:51:38Z

app/src/main/java/org/sopt/and/feature/my/MyRoute.kt

 ) {
+    val state by viewModel.state.collectAsStateWithLifecycle()
+
+    LaunchedEffect(true) {


이런거 보면 Route를 쓰는게 좋을 거 같다는 생각이 드네요

HAJIEUN02

고생많았서요 혜음이~~ 클린아키텍처 적용을 해서 그런지 저랑 비슷한 부분이 많아서 보기 편했습니다 ㅎㅎ 유스케이스도 한번 사용해보셔요!

HAJIEUN02 · 2024-11-17T09:39:50Z

app/build.gradle.kts

+
+    // Network
+    implementation(platform(libs.okhttp.bom))
+    implementation(libs.okhttp)


okhttp랑 retrofit처럼 같은 카테고리인 친구들은 각각 libs.versions.toml 내부에서 bundles로 묶어서 한번에 디펜던시 추가해줄 수 있서요! 그러면 더 보기 깔끔할 듯합니당

적용해보겠습니다링~!!!

HAJIEUN02 · 2024-11-17T09:40:25Z

app/src/main/AndroidManifest.xml

        tools:targetApi="31">
        <activity
-            android:name=".main.MainActivity"
+            android:name=".feature.main.MainActivity"


오호 ui가 아니라 feature로 패키지를 구성하셨군요
새로운 네이밍 좋네요

저는 지난 플젝때 이렇게 했어서 자연스럽게 이렇게 네이밍을 했어요

HAJIEUN02 · 2024-11-17T09:42:36Z

app/src/main/java/org/sopt/and/data/di/NetworkModule.kt

+    @Provides
+    fun provideLoggingInterceptor(): HttpLoggingInterceptor {
+        return HttpLoggingInterceptor { message ->
+            Log.d("Retrofit2", "CONNECTION INFO -> $message")


로그는 PR 올릴 때 삭제해주시거나 Timber로 바꿔주시면 좋을 것 같아요!!

팀버녀석 사용해볼게요!!

HAJIEUN02 · 2024-11-17T10:19:37Z

app/src/main/java/org/sopt/and/data/dto/request/RequestSignInDto.kt

+    val password: String,
+)
+
+fun RequestSignInEntity.toDto() = RequestSignInDto(


요거는 엔티티를 디티오로 바꾸는 거니까 엔티티가 주체잖아요!! 그래서 저라면 엔티티에 해당함수를 구현할 것 같아요

그렇네요!! 꼼꼼한 리뷰 너무 감사해요 :)

구현하려다가 생각난건데 이렇게 되면 domain단에서 data를 알고있다는건데 클린 아키텍처의 목적에 부합하지 않아지는 것 아닐까요?? 이와 관련해서는 어떻게 생각하시는지도 궁금합니다!

1971123-seongmin

고생하셨습니다. 항상 잘하셔서 리뷰라기보단 많이 배워가는 것 같아요.

1971123-seongmin · 2024-11-17T14:16:20Z

app/src/main/java/org/sopt/and/data/datasourceimpl/WavveDataSourceImpl.kt

+    override suspend fun getUserHobby(): BaseResponse<ResponseUserHobbyDto> =
+        wavveService.getUserHobby()
+
+}


깔끔 그 자체에요. 코드 너무 좋아요

1971123-seongmin · 2024-11-17T14:16:55Z

app/src/main/java/org/sopt/and/data/di/DataSourceModule.kt

+    @Binds
+    @Singleton
+    abstract fun bindsDataSource(myDataSourceImpl: WavveDataSourceImpl): WavveDataSource
+}


데이터스토어 모듈을 따로 만드셨네요. 저도 분리해야겠어요.

1971123-seongmin · 2024-11-17T14:19:32Z

app/src/main/java/org/sopt/and/data/dto/response/ResponseSignInDto.kt

+    @SerialName("token")
+    val token: String
+){
+    fun toEntity() = ResponseSignInEntity(


objcect에 Mapper를 모아두는 방식을 안쓰고 데이터 클래스에 바로 정의하신 이유 알려주실 수 있나요??

사실 이게 습관이 된 것도 있기도하고 뭔가 파일이 더 많아지는 게 싫어서 ㅠㅠ 의 이유도 있습니다

1971123-seongmin · 2024-11-17T14:21:22Z

app/src/main/java/org/sopt/and/feature/signup/SignUpNavigation.kt


 import androidx.navigation.NavController
 import androidx.navigation.NavGraphBuilder
 import androidx.navigation.compose.composable
 import kotlinx.serialization.Serializable
-import org.sopt.and.main.MainTabRoute
+import org.sopt.and.feature.main.MainTabRoute

 fun NavController.navigateSignUp() {
    navigate(SignUp)


컴포즈도 보통 이렇게 네비게이션 함수를 확장으로 두고 쓰나요??

저는 이전 플젝에서 이렇게 사용했어서 사용하긴한건데 다른 분들은 어떻게 생각하는지 궁금하네요..
개인적으로는 한 파일에서 관리해줄 수 있으니까 편하다고 느껴지기는 한 것 같아요..!

1971123-seongmin · 2024-11-17T14:21:59Z

app/src/main/java/org/sopt/and/feature/signup/SignUpState.kt

+    val hobby: String = "",
+    var isPasswordVisible: Boolean = false,
+) {
+    val isButtonEnabled: Boolean = username.isNotEmpty() && password.isNotEmpty()


이렇게 하면 버튼 활성화/비활성화를 관리할 때 훨씬 편할까요??

사람 취향에 따라 viewmodel에서 관리하거나 state에서 관리할 수 있는데 저는 간단한 로직이라서 따로 함수 호출없이 이렇게 관리하는게 편한 것 같아요!

1971123-seongmin · 2024-11-17T14:23:19Z

app/src/main/res/values/strings.xml

    <string name="not_valid_input">유효하지 않은 입력입니다.</string>

    <!--signin-->
    <string name="signin">로그인</string>
-    <string name="signin_placeholder">이메일 주소 또는 아이디</string>
+    <string name="signin_username_placeholder">이름</string>
+    <string name="signin_password_placeholder">비밀번호</string>
    <string name="find_id">아이디 찾기</string>
    <string name="reset_password">비밀번호 재설정</string>
    <string name="join">회원가입</string>


string 이름들이 정말 깔끔하고 직관적이네요..

jihyunniiii

역시 참 깔끔하네요! 고생하셨습니다! 우리 혜음이 합세도 파이팅!

jihyunniiii · 2024-11-19T19:58:18Z

app/src/main/java/org/sopt/and/data/datasourceimpl/WavveDataSourceImpl.kt

+import javax.inject.Inject
+
+class WavveDataSourceImpl @Inject constructor(
+    val wavveService: WavveService


private를 붙여줘도 좋을 듯,,!! 합니다

저 맨날 이런 접근제어자를 제대로 활용못하는 것 같아요 ㅠ 좀 적용해봐야겠습니다

jihyunniiii · 2024-11-19T19:59:49Z

app/src/main/java/org/sopt/and/data/dto/response/BaseResponse.kt

+
+

요기는 2줄 공백이당 크크

jihyunniiii · 2024-11-19T20:04:37Z

app/src/main/java/org/sopt/and/feature/signin/SignInViewModel.kt

 import javax.inject.Inject

 @HiltViewModel
-class SignInViewModel @Inject constructor() : ViewModel() {
+class SignInViewModel @Inject constructor(
+    private val user:User,


정렬이 제대로 안 된 것 같숨다

그리고 여기 user를 매개변수로 넣어주는 이유가 뭔가요? ㅜ 찾고 싶은데 졸려서 그런가 안 보여잉 ㅠㅠㅠ

jihyunniiii · 2024-11-19T20:10:31Z

app/src/main/java/org/sopt/and/sharedpreference/User.kt

+import javax.inject.Singleton
+
+@Singleton
+class User @Inject constructor(


아 위에 있는 user가 이러한 역할을 하는 친구였군요!
승민님 리뷰처럼 조금 더 직관적으로 네이밍하는 게 필요할 것 같아요!
id랑 비밀번호 등을 저장하는 user data class인 줄 알았서요

말씀주신대로 네이밍을 수정해보도록 할게요!

hyeeum added 21 commits November 8, 2024 14:04

Feature #11: network setting

148c3d2

Chore #11: extract strings

eca37a7

Chore #11: feature package align

75519a4

Chore #11: feature package align

13cbd4f

Feature #11: di package

265203e

Feature #11: dto package

84616e6

Feature #11: entity package

b5b6e0f

Feature #11: WavveDataSource

084407f

Feature #11: WavveDataSourceImpl

41840c9

Feature #11: WavveRepository

6c47389

Feature #11: WavveRepositoryImpl

37d3db6

Feature #11: WavveService

6db3300

Chore #11: rename App to WavveApplication

a3176cf

Chore #11: change email,password to token

450ed5e

Feature #11: MyState

4538e9b

Feature #11: MyViewModel

db02568

Chore #11: delete signUpEmail,signUpPassword parameters

f68a47e

Chore #11: edit parameter email to username

9712368

Chore #11: edit parameter email to username and add hobby

52c7573

Chore #11: connect signup api

5748826

Chore #11: rename function,class name

765cb5f

hyeeum added the enhancement New feature or request label Nov 14, 2024

hyeeum requested review from HAJIEUN02, kamja0510 and 1971123-seongmin November 14, 2024 18:08

hyeeum self-assigned this Nov 14, 2024

greedy0110 reviewed Nov 16, 2024

View reviewed changes

kamja0510 approved these changes Nov 17, 2024

View reviewed changes

HAJIEUN02 approved these changes Nov 17, 2024

View reviewed changes

1971123-seongmin approved these changes Nov 17, 2024

View reviewed changes

jihyunniiii reviewed Nov 19, 2024

View reviewed changes

hyeeum added 7 commits December 4, 2024 21:51

Chore #11: apply OkHttpClient builder to debug condition

ba00844

Chore #11: add common failure string

595a2fa

feature #11: add MySideEffect

48bb5af

chore #11: add onFailure event

2972f5f

chore #11: apply bundleto version catalog

86dca27

chore #11: delete space

d2c8c11

chore #11: make Service to private

995174b

hyeeum merged commit fe53ce4 into develop Dec 4, 2024

hyeeum deleted the week4 branch December 4, 2024 15:19

[Week4] 4주차 필수 과제 #12

[Week4] 4주차 필수 과제 #12

Conversation

hyeeum commented Nov 14, 2024 • edited Loading

Related issue 🛠

Work Description ✏️

Screenshot 📸

To Reviewers 📢

greedy0110 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamja0510 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HAJIEUN02 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1971123-seongmin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jihyunniiii left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyeeum commented Nov 14, 2024 •

edited

Loading