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/#9] 4주차 필수 과제 구현 #11

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

Conversation

boiledEgg-s
Copy link
Contributor

Related issue 🛠

Work Description ✏️

  • 유저 등록, 로그인, 내 취미 조회 api 연결

Screenshot 📸

  • 로그인

    week4_signin.mp4
  • 회원가입

    week4_signup.mp4
  • 취미 조회

Uncompleted Tasks 😅

  • 도전, 심화 과제
  • 키보드 관리 (아직도 안함)

To Reviewers 📢

  • 수정된 파일 수 많은건 초반에 패키지 이동을 몇번 해서 그렇습니다ㅎㅎ 겁먹지 마세요~
  • Call 사용은 거의 안해봐서 아키텍처에 적용하고 의존성 관리하는게 좀 어려웠네요,,
    어설픈 부분 있으면 코멘트 남겨주세요😊

@boiledEgg-s boiledEgg-s added the enhancement New feature or request label Nov 5, 2024
@boiledEgg-s boiledEgg-s requested a review from a team November 5, 2024 08:38
@boiledEgg-s boiledEgg-s self-assigned this Nov 5, 2024
@boiledEgg-s boiledEgg-s requested review from jm991014, kangyein9892, jihyunniiii and serioushyeon and removed request for a team November 5, 2024 08:38
Copy link

@kangyein9892 kangyein9892 left a comment

Choose a reason for hiding this comment

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

Room까지 알차게 사용해서 Repository 많이 만드느라 고생하셨습니다!!!! 짱짱

Comment on lines +11 to +13
fun postSignUp(
request: SignUpRequestDto
): Call<BaseResponse<SignUpResponseDto>>

Choose a reason for hiding this comment

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

To reviewer에 원래는 call을 안쓰셨다고 했던 것 같은데 원래 하셨던 방법이 뭐였나요?!
그리고 이번에는 특별히 call을 쓰신 이유가 있을까요? (suspend 적용 바로 안하시고...!!)
datasource 부분까지 call을 사용하신 이유가 있을까요?!

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.

앞서나기 싫어서 Call 사용했습니다ㅎㅎ (근데 진짜에요)


@Serializable
data class BaseResponse<T>(
@SerialName("result")

Choose a reason for hiding this comment

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

서버랑 이름이 같다면 굳이 @SerialName 안붙여도 되는 걸로 알고 있는데 붙이신 이유가 있을까요...!? 보기 편함을 위해서...??!

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 +6 to +9
fun User.toSignInRequest(): SignInRequestDto = SignInRequestDto(
userName = this.userName,
password = this.password
)

Choose a reason for hiding this comment

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

따로 mapper 패키지와 파일을 만들어서 사용하는 방법도 한번에 보기 편하고 찾기 편하니까 좋은 방법인 것 같네요...!

Comment on lines +20 to +50
override suspend fun getMyHobby(token: String): Result<Hobby> = runCatching {
suspendCoroutine { continuation ->
userDataSource.getMyHobby(token)
.enqueue(object : Callback<BaseResponse<MyHobbyResponseDto>> {
override fun onResponse(
call: Call<BaseResponse<MyHobbyResponseDto>>,
response: Response<BaseResponse<MyHobbyResponseDto>>
) {
if (response.isSuccessful) {
response.body()?.result?.hobby?.let {
val hobby = Hobby(it)
Log.d("MyHobby", it)
continuation.resume(hobby)
}
} else {
continuation.resumeWithException(Exception())
}
}

override fun onFailure(
call: Call<BaseResponse<MyHobbyResponseDto>>,
response: Throwable
) {
Log.d("MyHobby", "fail")
continuation.resumeWithException(response)
}
})
}
}

}
Copy link

@kangyein9892 kangyein9892 Nov 15, 2024

Choose a reason for hiding this comment

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

제가 잘 알지못해서 그런걸수도 있지만.... 혹시 repositoryImpl에서 이 작업을 한 이유가 있을까요? 제가 볼때는 데이터를 가져와서 처리해주는 것뿐이고 도메인로직같지는 않아서요...!! datasource에서 진행해도 괜찮지 않을까요?
그리고 continuation.resume continuation.resumeWithException이 뭔가요??! 궁금합니다 ㅎㅎ
(그리고 Log 다썼으면 지워주시와요 ~~)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • 제 개인적인 생각으로 DataSource라는건 단순히 저장소로부터 데이터를 불러오는 작업만 수행해야 한다고 생각합니다!
    성공 여부 확인이라던가 기타 엔티티 매핑 작업 같은건 레포지토리와 유즈케이스 같은 도메인층에서 작업해주는게 옳다고 생각해서 이렇게 구현을 했습니다!

  • continuation의 경우 제 디스커션 과제로 대신 답할게요! 여기로

  • 로그 확인!

Copy link

@serioushyeon serioushyeon left a comment

Choose a reason for hiding this comment

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

역시 흠잡을게 없는 코드네요! 고생하셨습니다~

class AuthDataSourceImpl @Inject constructor(
private val userService: AuthService
): AuthDataSource {
override fun postSignUp(request: SignUpRequestDto): Call<BaseResponse<SignUpResponseDto>> = userService.signUp(request)

Choose a reason for hiding this comment

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

요기도 뭔가 줄 분리하면 좋을 것 같아요!

}
continuation.resume(
SignInResponse(
message = R.string.signin_snackbar_fail

Choose a reason for hiding this comment

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

아 string으로 가져오는게 아니라 resId로 받아올 수 있군요! 이걸 생각 못했네..

Copy link

@jm991014 jm991014 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 +11 to +13
fun postSignUp(
request: SignUpRequestDto
): Call<BaseResponse<SignUpResponseDto>>

Choose a reason for hiding this comment

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

저도 궁금합니다~!!

} else {
response.errorBody()?.run {
val body = JSONObject(string())
Log.d("SignIn", "code : ${body.getString("code")}")

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 +18 to 20
fun clearToken() {
token = ""
}
Copy link
Contributor

Choose a reason for hiding this comment

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

sharedPreferences를 clear 시켜주는 것 대신 토큰만 clear 해준 이유가 있는지 궁금해요!
또, sharedPreference에서 remove 함수를 사용해 줄 수도 있습니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

그냥 필요한 부분만 지우는게 좋지 않을까 싶어서 저렇게 했습니다!
remove에 대해선 자세히 알아보고 적용할게요~

Comment on lines 26 to +27


Copy link
Contributor

Choose a reason for hiding this comment

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

2줄 공백이다 ㅋㅋ

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 +11 to +13
fun postSignUp(
request: SignUpRequestDto
): Call<BaseResponse<SignUpResponseDto>>
Copy link
Contributor

Choose a reason for hiding this comment

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

suspend 다음주에 알려줄 거야 ㅜㅜ 앞서나가지마잉 ㅋㅋ

@InstallIn(SingletonComponent::class)
object RetrofitModule {

@Provides
Copy link
Contributor

Choose a reason for hiding this comment

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

@provides@BINDS의 차이는 뭘까용?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Provides는 클래스의 객체를 직접 생성해야 할 때 사용합니다. 주로 외부 라이브러리 클래스의 객체를 생성하고 의존성 그래프에 제공하기 위해 사용하죠!

@Binds는 구현체를 인터페이스에 바인딩하기 위해 사용한다고 합니다.
즉, 인터페이스와 그 구현체를 연결하고 인스턴스를 생성하여 그 인스턴스를 인터페이스로 사용할 수 있도록 해주는 것이죠!


@Provides
@Singleton
fun providesHomeService(
Copy link
Contributor

Choose a reason for hiding this comment

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

UserService를 받아오는데 이름은 HomeService인 이유가 있나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

어라라,, 클래스 이름 바꿀때 모듈 이름을 못바꿨나보네요,,

import retrofit2.http.Body
import retrofit2.http.POST

interface AuthService {
Copy link
Contributor

Choose a reason for hiding this comment

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

Service를 분리하시는 기준이 있는지 궁금해요!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

외부 저장소가 여러 개인 경우 저장소 기준으로 나누고, 그 하위에선 기능별로 나누는 것 같아요!
예를 들면 회원가입 로그인과 같이 사용자의 정보를 확인하는 서비스, 마이페이지와 같이 사용자의 정보를 불러오는 서비스, 홈화면에서 사용자에게 표시할 여러 정보를 불러오는 서비스 등으로 나눕니다!

@@ -0,0 +1,8 @@
package org.sopt.and.domain.entity
Copy link
Contributor

Choose a reason for hiding this comment

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

domain 레이어에는 안드로이드 종속성 없이 순수 코틀린 언어로만 작성되어야 하죠?
근데 StringRes 어노테이션을 사용하면서 안드로이드와 종속성이 생겼어요.
어떻게 수정하면 좋을지 생각해보면 좋을 것 같아요!

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 +74 to +77
/**
* 4주차 이후로 쓸모가 없어진 코드
* 만든게 아까워서 전시용으로 놔둠
*/
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 +87 to +88
## retrofit
## Retrofit
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
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEAT] 4주차 과제 (필수)
5 participants