Skip to content

3주차: API 연동 처리 🚀 - #8

Open
simuelunbo wants to merge 27 commits into
mainfrom
#7
Open

3주차: API 연동 처리 🚀#8
simuelunbo wants to merge 27 commits into
mainfrom
#7

Conversation

@simuelunbo

Copy link
Copy Markdown
Collaborator

#7

지난번 피드백 바탕으로 한번 클래스 및 함수명에 의도와 맞게 신경썼는데 제대로 작성했는지 잘 모르겠네요 😅
한번 확인 해주시고 리뷰 해주시면 감사하겠습니다. 🙇‍♂️

@simuelunbo
simuelunbo requested a review from jerry-f-lab March 25, 2025 11:49
@simuelunbo simuelunbo self-assigned this Mar 25, 2025
Comment thread core/data/src/main/java/com/simuel/sunflower/core/data/di/RepositoryModule.kt Outdated
internal abstract class DataSourceModule {

@Binds
@Singleton

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

왜 싱글톤이어야하죠?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

수정하였습니다.
adb14df

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

단순하게 처음에는 '네트워크나 DB 접근하는 클래스는 다 싱글톤 쓰는게 좋겠다' 라고 생각 했었습니다.
하지만 상태를 유지하거나 무거운 리소스를 들고 있는 것도 아닌데 굳이 Singleton으로 만들 필요가 없는것 같습니다.

private const val BASE_URL = "https://api.unsplash.com/"

@Provides
@Singleton

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

왜 싱글톤이어야하죠?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

네트워크 관련 Retrofit과 OkHttpClient는 전역적으로 한번만 존재해야 하는 자원이기 때문에 매번 생성하는것 보다 한번 생성하여 관리하는 것이 메모리 및 성능 측면에서 유리합니다.
그래서 싱글톤으로 유지하는 것이 좋다고 판단했습니다.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

왜 retrofit과 ok http client는 하나만 존재해야한다고 생각하시죠?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

okhttpclient에 내부적으로 커넥션 풀이 있어서 재사용 가능한 소켓 연결을 유지하는데 인스턴스를 여러개 생성하면 풀도 여러개 생겨서 불필요하게 소켓을 낭비 하여 불필요한 메모리 낭비 하게 됩니다.
뿐만아니라 Retrofit이나 OKhttpClient에 설정값(인터셉터, 공통헤더 등) 한 인스턴스만 쓰면 특정 설정을 한 곳에서만 관리 가능해서 일관성이 유지 되기 때문에 하나만 존재 해야 합니다.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

그런데 이건 retrofit도 ok http도 아닌 logging하는 intercepter일 뿐인데요?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

logging 하는 인터셉터라도 굳이 동일한 목적의 인터셉터를 중복해서 여러 번 생성할 필요가 없고 logging level이나 포맷을 한 군데서만 제어하여 일관성 있게 처리 할수 있기 때문입니다.

}

@Provides
@Singleton

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

왜 싱글톤이어야 하죠?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

하나의 OkHttpClient를 공유하면 커넥션 풀이 효율적으로 재사용할수 있기 때문입니다.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

커넥션 풀을 왜 재사용해야한다는 말씀이실까요? 재사용하지 않으면 무슨 일이 벌어지죠?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

커넥션풀을 재사용 하지 않으면 요청마다 새 클라이언트를 생성하면 유후 풀에 리소스가 낭비됩니다
해당 내용은 공식 문서에서도 확인 할수 있습니다.
https://square.github.io/okhttp/5.x/okhttp/okhttp3/-ok-http-client/

Comment thread core/network/src/main/java/com/simuel/sunflower/core/network/model/PhotoUrls.kt Outdated
@simuelunbo
simuelunbo requested a review from jerry-f-lab March 26, 2025 12:41
@simuelunbo
simuelunbo requested a review from jerry-f-lab March 27, 2025 11:52
@simuelunbo
simuelunbo requested a review from jerry-f-lab March 27, 2025 12:46
return HttpLoggingInterceptor().apply {
level = HttpLoggingInterceptor.Level.BODY
}
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

overkill

val contentType = "application/json".toMediaType()
return Retrofit.Builder().baseUrl(BASE_URL).client(okHttpClient)
.addConverterFactory(json.asConverterFactory(contentType)).build()
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

NG - base url, 인증 service에 의존.

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.

2 participants