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] 6주차 과제 #10

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

[feat] 6주차 과제 #10

wants to merge 4 commits into from

Conversation

t1nm1ksun
Copy link
Member

Related issue 🛠

Work Description ✏️

  • Repository Pattern 사용
  • MVVM Architecture Pattern 사용
  • UseCase사용
  • Hilt 적용

Screenshot 📸

변경사항 없습니다

Uncompleted Tasks 😅

  • N/A

To Reviewers 📢

  • X

@t1nm1ksun t1nm1ksun added the feat Feature label Nov 28, 2024
@t1nm1ksun t1nm1ksun requested a review from a team November 28, 2024 10:09
@t1nm1ksun t1nm1ksun self-assigned this Nov 28, 2024
@t1nm1ksun t1nm1ksun requested review from chattymin, tunaunnie, jihyunniiii and angryPodo and removed request for a team November 28, 2024 10:10
Copy link

@chattymin chattymin left a comment

Choose a reason for hiding this comment

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

역시 깔끔하게 잘 하시네요!
다 아시는 내용이겠지만 달게 없어서 이런거라도 달아봤습니다~ ㅎㅎ

@@ -5,8 +5,9 @@ plugins {
alias(libs.plugins.kotlin.android)
alias(libs.plugins.kotlin.compose)
alias(libs.plugins.kotlin.serialization)
alias(libs.plugins.dagger.hilt)
id("org.jetbrains.kotlin.kapt")

Choose a reason for hiding this comment

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

ksp 써보쉴?
kapt와 ksp의 차이점, ksp의 장점은 아실테니 한번 해보시면 좋을거같아요~

import org.sopt.and.data.dataremote.model.request.RequestSignUpDto
import org.sopt.and.data.dataremote.model.response.ResponseSignUpDto
import org.sopt.and.domain.repository.UserRepository
import retrofit2.Response

Choose a reason for hiding this comment

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

domain에 retrofit 의존성이 들어있네용!
빼주시면 좋을거같아요~

@@ -3,4 +3,5 @@ plugins {
alias(libs.plugins.android.application) apply false
alias(libs.plugins.kotlin.android) apply false
alias(libs.plugins.kotlin.compose) apply false
id("com.google.dagger.hilt.android") version "2.52" apply false

Choose a reason for hiding this comment

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

플러그인 추가하셨는데 그거 활용하셔도 좋을 것 같아요~

@@ -5,8 +5,9 @@ plugins {
alias(libs.plugins.kotlin.android)
alias(libs.plugins.kotlin.compose)
alias(libs.plugins.kotlin.serialization)
alias(libs.plugins.dagger.hilt)
id("org.jetbrains.kotlin.kapt")

Choose a reason for hiding this comment

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

나도 kapt썻다가 ksp썻다가...ksp로 나중에 오류 잡아서 고쳐보자!

userRemoteDataSource.postSignup(requestSignUpDto)

override suspend fun postLogin(requestLoginDto: RequestLoginDto): Response<ResponseLoginDto> =
userRemoteDataSource.postLogin(requestLoginDto)

Choose a reason for hiding this comment

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

Response 타입을 그대로 반환하는데, 도메인 레이어에서 더 사용하기 좋은 형태(Result나 sealed class)로 매핑하는게 더 좋다고 들었는데 어떤가요?

loggingInterceptor: HttpLoggingInterceptor
): OkHttpClient =
OkHttpClient.Builder().apply {
connectTimeout(10, TimeUnit.SECONDS)

Choose a reason for hiding this comment

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

타임아웃 디테일👍 이부분도 나중에는 상수화 하는게 좋겠네요

Copy link

@tunaunnie tunaunnie left a comment

Choose a reason for hiding this comment

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

코드에서 노련함이 느껴져요.. 깔끔한 코드를 이정표 삼아서 열심히 공부해보겠습니다


class UserInfoLocalDataSourceImpl(context: Context) : UserInfoLocalDataSource {
class UserLocalDataSourceImpl @Inject constructor(

Choose a reason for hiding this comment

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

LocalDataSource에 대해서 찾아봤는데 실제로 개발할 때 사용하기 엄청 유용할 것 같더라고요..! 길지 않은 시간 동안 무겁지 않은 정보를 저장하고 싶을 때 유용하게 쓸 것 같아요!!

@@ -1,4 +1,4 @@
package org.sopt.and.domain
package org.sopt.and.domain.model

data class User(

Choose a reason for hiding this comment

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

여기에다가 hobby, token을 한번에 관리하지 않고 다 다른 데이터 클래스에 분리해두는 이유가 뭔가요??

import dagger.hilt.android.HiltAndroidApp

@HiltAndroidApp
class WavveApp : Application()

Choose a reason for hiding this comment

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

Hilt를 쓰니까 @HiltAndroidApp 어노테이션이 없으면 빌드 에러가 나더라구요...? 힐트를 써줄때면 이렇게 어플리케이션을 늘 하나 따로 만들어서 androidmanifest.xml 파일에도 등록해줘야 하는 건가요?! 뭔가 프로젝트 안에 동떨어진 WaveApp.kt 파일이 하나 생기는 게 조금 이질적으로 느껴져서 여쭤봅니다 ㅜㅜ

Copy link

@angryPodo angryPodo 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
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 +2 to +3


Copy link
Contributor

Choose a reason for hiding this comment

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

2줄 ~

Comment on lines +9 to +10


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 +42 to +47
addInterceptor { chain ->
val request = chain.request().newBuilder()
.addHeader("Accept", "*/*")
.build()
chain.proceed(request)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

이 친구 꼭 필요한가요?

package org.sopt.and.domain.model

data class Hobby(
var hobby: String = "",
Copy link
Contributor

Choose a reason for hiding this comment

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

var로 선언하신 이유가 있나요?

@@ -0,0 +1,5 @@
package org.sopt.and.domain.model

data class Token(
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 +3 to +4
import org.sopt.and.data.dataremote.model.request.RequestLoginDto
import org.sopt.and.data.dataremote.model.response.ResponseLoginDto
Copy link
Contributor

Choose a reason for hiding this comment

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

여기도 마찬가지입니다

@@ -0,0 +1,15 @@
package org.sopt.and.domain.usecase

import org.sopt.and.data.dataremote.model.response.ResponseMyHobbyDto
Copy link
Contributor

Choose a reason for hiding this comment

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

여기도

import org.sopt.and.data.datalocal.datasource.UserInfoLocalDataSource
import org.sopt.and.data.service.RetrofitInstance.userService
import org.sopt.and.domain.User
import org.sopt.and.data.datalocal.datasource.UserLocalDataSource
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 +13 to +14
import org.sopt.and.data.dataremote.model.request.RequestLoginDto
import org.sopt.and.data.dataremote.model.response.ResponseFailedDto
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 -13 to +12
import org.sopt.and.domain.User
import org.sopt.and.data.dataremote.model.request.RequestSignUpDto
import org.sopt.and.data.dataremote.model.response.ResponseFailedDto
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
feat Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feat] 6주차 과제
5 participants