Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on improving token handling, updating the base URL for API requests, and integrating a weather API. The changes enhance the app's reliability and introduce new features related to weather data. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이번 PR은 카카오 토큰 관리 방식 변경과 날씨 API 연동 추가에 대한 내용을 담고 있습니다. 전반적으로 Hilt를 적용하여 의존성 주입을 개선하고, 기존 로직을 정리하는 좋은 방향의 수정입니다. 다만, 몇 가지 개선점을 제안합니다. 새로 추가된 날씨 API 관련 코드의 아키텍처 개선, API URL 관리 방식, 그리고 스플래시 화면에서의 토큰 유효성 검사 로직 보강이 필요해 보입니다. 자세한 내용은 각 파일에 남긴 코멘트를 참고해주세요.
| class WeatherRepository @Inject constructor( | ||
| private val weatherService: WeatherService | ||
| ) { | ||
| fun fetchWeather(cityName: String, apiKey: String) { |
There was a problem hiding this comment.
API 키를 매개변수로 전달하는 것은 보안상 위험할 수 있습니다. 키가 ViewModel이나 UI 레이어에 노출될 수 있기 때문입니다. API 키는 BuildConfig나 암호화된 SharedPreferences와 같은 안전한 곳에 저장하고, 데이터 레이어 내에서 직접 참조하는 것이 좋습니다. 함수 시그니처에서 apiKey를 제거하는 것을 고려해 보세요.
| if (accessToken != null) { | ||
| // 1. 토큰이 있으면 로그인된 상태 -> 메인으로 | ||
| navigateToMain() |
There was a problem hiding this comment.
현재 로직은 액세스 토큰의 존재 여부만 확인하고 유효성은 검사하지 않습니다. 만약 토큰이 만료되었다면, 사용자는 메인 화면으로 이동하더라도 API 호출이 계속 실패하여 앱이 비정상적으로 동작하는 것처럼 보일 수 있습니다. 스플래시 화면에서 간단한 API(예: 사용자 정보 가져오기)를 호출하여 토큰의 유효성을 검증하거나, 또는 모든 API 요청에 대해 401 에러를 공통으로 처리하여 토큰 만료 시 자동으로 로그인 화면으로 보내는 로직(예: OkHttp의 Authenticator)을 구현하는 것을 고려해보세요.
| fun fetchWeather(cityName: String, apiKey: String) { | ||
| weatherService.getWeather(cityName, apiKey).enqueue(object : retrofit2.Callback<WeatherResponse> { | ||
| override fun onResponse(call: retrofit2.Call<WeatherResponse>, response: retrofit2.Response<WeatherResponse>) { | ||
|
|
||
| Log.d("PACE_DEBUG", "Status Code: ${response.code()}") | ||
| Log.d("PACE_DEBUG", "Requested URL: ${call.request().url}") | ||
|
|
||
| if (response.isSuccessful) { | ||
| val weatherData = response.body() | ||
| Log.d("PACE_DEBUG", "날씨 데이터 수신 성공: $weatherData") | ||
| } else { | ||
|
|
||
| Log.e("PACE_DEBUG", "에러 발생: ${response.errorBody()?.string()}") | ||
| } | ||
| } | ||
|
|
||
| override fun onFailure(call: retrofit2.Call<WeatherResponse>, t: Throwable) { | ||
|
|
||
| Log.e("PACE_DEBUG", "통신 실패 원인: ${t.message}") | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
현재 fetchWeather 함수는 콜백을 사용하며 결과를 로그로만 남기고 있습니다. 이 방식은 ViewModel 등에서 데이터를 활용하기 어렵게 만들고 테스트도 복잡하게 합니다. 코루틴을 사용하는 현대적인 안드로이드 개발 방식에 맞춰 suspend 함수로 리팩토링하는 것을 강력히 권장합니다. 이렇게 하면 비동기 코드를 더 간결하고 읽기 쉽게 만들 수 있습니다.
suspend fun fetchWeather(cityName: String, apiKey: String): WeatherResponse = suspendCancellableCoroutine { continuation ->
val call = weatherService.getWeather(cityName, apiKey)
call.enqueue(object : retrofit2.Callback<WeatherResponse> {
override fun onResponse(call: retrofit2.Call<WeatherResponse>, response: retrofit2.Response<WeatherResponse>) {
if (response.isSuccessful) {
response.body()?.let { data ->
continuation.resume(data)
} ?: continuation.resumeWithException(IllegalStateException("Weather data is null"))
} else {
continuation.resumeWithException(Exception("API Error: ${response.code()} ${response.message()}"))
}
}
override fun onFailure(call: retrofit2.Call<WeatherResponse>, t: Throwable) {
if (continuation.isCancelled) return
continuation.resumeWithException(t)
}
})
continuation.invokeOnCancellation {
call.cancel()
}
}| fun provideRetrofit(okHttpClient: OkHttpClient): Retrofit { | ||
| return Retrofit.Builder() | ||
| .baseUrl("http://ec2-3-35-233-51.ap-northeast-2.compute.amazonaws.com:8080/") | ||
| .baseUrl("https://pace-server.kro.kr/") |
There was a problem hiding this comment.
이곳과 72라인에 있는 기본 URL들이 하드코딩되어 있으며, app/src/main/java/com/example/pace/data/api/RetrofitClient.kt 파일에도 중복으로 정의되어 있습니다. URL을 여러 곳에 하드코딩하면 향후 유지보수가 어려워질 수 있습니다. 이 URL들을 한 곳(예: 이 파일 상단의 companion object 또는 별도의 상수 파일)에 상수로 정의하고 재사용하는 것을 권장합니다.
| @Inject | ||
| lateinit var authDataStore: AuthDataStore | ||
|
|
||
| //private val authDataStore by lazy { (application as PaceApplication).authDataStore } |
There was a problem hiding this comment.
이 주석 처리된 코드는 더 이상 사용되지 않으므로 삭제하는 것이 좋습니다. 코드를 깨끗하게 유지하는 데 도움이 됩니다.
1 participant
PR템플릿
변경 사항 요약
체크리스트
사진올리는곳