[REFACTOR/#297] 에러 핸들링 중앙화 해버리기 #316
Conversation
- 에러 메시지를 제공하는 ErrorMessageProvider 인터페이스와 구현체 ErrorMessageProviderImpl을 추가했습니다. - ErrorMessageModule을 추가하여 ErrorMessageProvider를 주입할 수 있도록 설정했습니다.
- ViewModel에서 NetworkUtil 대신 NetworkConnectivityObserver 사용 - 에러 메시지를 ErrorMessages 객체 대신 ErrorMessageProvider를 통해 제공하도록 수정 - 각 ViewModel의 네트워크 요청 및 에러 처리 로직에 변경된 방식 적용
WalkthroughThis change centralizes error handling and network status logic across the application. It introduces new abstractions for error messages and network connectivity observation, removes hardcoded error strings, and updates ViewModels and repositories to use these new mechanisms. Supporting resources and modules are added, and obsolete utilities and constants are removed. Changes
Sequence Diagram(s)sequenceDiagram
participant VM as ViewModel
participant NCO as NetworkConnectivityObserver
participant EMP as ErrorMessageProvider
participant Repo as Repository
participant DS as DataSource
participant API as Remote API
VM->>NCO: Check networkStatus (Flow)
alt Network Unavailable
VM->>EMP: getNetworkError()
VM-->>VM: Show error message
else Network Available
VM->>Repo: Perform operation
Repo->>DS: safeApiCall { API call }
DS->>API: Make network request
API-->>DS: Response / Error
DS-->>Repo: Result<T> (success/failure)
Repo-->>VM: Result<T>
alt Failure
VM->>EMP: getApiError(error) or getTemporaryError()
VM-->>VM: Show error message
else Success
VM-->>VM: Update UI with data
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Suggested labels
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
app/src/main/java/com/sopt/clody/presentation/ui/login/LoginViewModel.kt (1)
102-107: Complete the error handling centralization.The error handling in
validateKakaoUserandvalidateGoogleUsermethods still uses rawerror.messagedirectly instead of the centralizedErrorMessageProvider. Consider adding appropriate error message methods to the provider for these scenarios.For consistency with the centralized error handling pattern, consider adding methods like:
getSignInFailedError()getSignUpFailedError()getServerError()Then replace the raw error message usage:
-_sideEffects.send(LoginContract.LoginSideEffect.ShowError(msg)) +_sideEffects.send(LoginContract.LoginSideEffect.ShowError(errorMessageProvider.getServerError()))Also applies to: 128-135
app/src/main/java/com/sopt/clody/presentation/ui/diarylist/screen/DiaryListViewModel.kt (1)
89-94: Verify state assignment in error handlingLine 91 assigns a
DiaryDeleteState.Failurevalue but doesn't assign it to_diaryDeleteState.value. This looks like a potential bug where the state isn't properly updated.Apply this fix:
- DiaryDeleteState.Failure(_failureDialogMessage.value) + _diaryDeleteState.value = DiaryDeleteState.Failure(_failureDialogMessage.value)
🧹 Nitpick comments (6)
app/src/main/java/com/sopt/clody/domain/repository/DiaryRepository.kt (1)
3-3: Remove unusedApiResponseimport
ApiResponseis no longer referenced in this interface; keeping the import introduces an unnecessary compiler warning and noise.-import com.sopt.clody.data.remote.dto.base.ApiResponseapp/src/main/java/com/sopt/clody/data/remote/util/SafeApiCall.kt (1)
8-25: Well-implemented error handling utility with proper coroutine support.The function correctly handles cancellation by rethrowing
CancellationExceptionand provides good error mapping. The approach of returningResult<T>enables functional error handling throughout the app.One consideration: Line 15 treats null
response.dataas a temporary error. Verify that this aligns with your API contract, as some endpoints might legitimately return null data for successful operations.app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderViewModel.kt (1)
45-46: Consider centralizing FCM token error messageThe FCM token error message "FCM 못가져옴" is still hardcoded. Consider adding this to the
ErrorMessageProviderfor consistency and localization support.Add a method to
ErrorMessageProvider:interface ErrorMessageProvider { // ... existing methods + fun getFcmTokenRetrievalError(): String }app/src/main/java/com/sopt/clody/presentation/ui/replyloading/screen/ReplyLoadingViewModel.kt (1)
85-89: Improve network status check for clarityThe network connectivity check uses a double negation pattern that could be simplified for better readability.
Consider this clearer approach:
- val isConnected = networkConnectivityObserver.networkStatus.first() == NetworkStatus.Available - if (!isConnected) { + if (networkConnectivityObserver.networkStatus.first() == NetworkStatus.Unavailable) {app/src/main/java/com/sopt/clody/presentation/utils/network/ErrorMessageProviderImpl.kt (1)
49-51: Consider adding fallback for ApiError messages.The current implementation directly returns
apiError.message. Consider adding a fallback in case the message is null or not user-friendly.override fun getApiError(apiError: ApiError): String { - return apiError.message + return apiError.message.takeIf { it.isNotBlank() } ?: getUnknownError() }app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingViewModel.kt (1)
66-66: Network connectivity checks properly migrated to reactive pattern.The migration from synchronous
NetworkUtilchecks to reactiveNetworkConnectivityObserver.networkStatus.first()is architecturally sound and consistent across all methods.Consider caching the network status check result at the beginning of each method to avoid multiple
first()calls:viewModelScope.launch { + val isNetworkUnavailable = networkConnectivityObserver.networkStatus.first() == NetworkStatus.Unavailable - if (networkConnectivityObserver.networkStatus.first() == NetworkStatus.Unavailable) { + if (isNetworkUnavailable) {Also applies to: 99-99, 149-149
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
app/src/main/java/com/sopt/clody/core/network/NetworkConnectivityModule.kt(1 hunks)app/src/main/java/com/sopt/clody/core/network/NetworkConnectivityObserver.kt(1 hunks)app/src/main/java/com/sopt/clody/core/network/NetworkStatus.kt(1 hunks)app/src/main/java/com/sopt/clody/data/remote/datasource/DiaryRemoteDataSource.kt(1 hunks)app/src/main/java/com/sopt/clody/data/remote/datasourceimpl/DiaryRemoteDataSourceImpl.kt(1 hunks)app/src/main/java/com/sopt/clody/data/remote/util/ApiError.kt(1 hunks)app/src/main/java/com/sopt/clody/data/remote/util/NetworkUtil.kt(0 hunks)app/src/main/java/com/sopt/clody/data/remote/util/SafeApiCall.kt(1 hunks)app/src/main/java/com/sopt/clody/data/repositoryimpl/DiaryRepositoryImpl.kt(1 hunks)app/src/main/java/com/sopt/clody/di/NetworkModule.kt(0 hunks)app/src/main/java/com/sopt/clody/domain/repository/DiaryRepository.kt(1 hunks)app/src/main/java/com/sopt/clody/domain/usecase/FetchDraftDiaryUseCase.kt(1 hunks)app/src/main/java/com/sopt/clody/domain/usecase/SaveDraftDiaryUseCase.kt(1 hunks)app/src/main/java/com/sopt/clody/domain/usecase/WriteDiaryUseCase.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpViewModel.kt(6 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderViewModel.kt(3 hunks)app/src/main/java/com/sopt/clody/presentation/ui/diarylist/screen/DiaryListViewModel.kt(6 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt(9 hunks)app/src/main/java/com/sopt/clody/presentation/ui/login/LoginViewModel.kt(3 hunks)app/src/main/java/com/sopt/clody/presentation/ui/replydiary/ReplyDiaryViewModel.kt(5 hunks)app/src/main/java/com/sopt/clody/presentation/ui/replyloading/screen/ReplyLoadingViewModel.kt(8 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingViewModel.kt(7 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/AccountManagementViewModel.kt(7 hunks)app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt(6 hunks)app/src/main/java/com/sopt/clody/presentation/utils/network/ErrorMessageModule.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/utils/network/ErrorMessageProvider.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/utils/network/ErrorMessageProviderImpl.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/utils/network/ErrorMessages.kt(0 hunks)app/src/main/res/values-ko/strings.xml(1 hunks)app/src/main/res/values/strings.xml(1 hunks)app/src/test/java/com/sopt/clody/datasource/FakeDiaryRemoteDataSource.kt(1 hunks)
💤 Files with no reviewable changes (3)
- app/src/main/java/com/sopt/clody/di/NetworkModule.kt
- app/src/main/java/com/sopt/clody/presentation/utils/network/ErrorMessages.kt
- app/src/main/java/com/sopt/clody/data/remote/util/NetworkUtil.kt
🧰 Additional context used
🧬 Code Graph Analysis (2)
app/src/main/java/com/sopt/clody/presentation/ui/replydiary/ReplyDiaryViewModel.kt (1)
app/src/main/java/com/sopt/clody/presentation/ui/replyloading/screen/ReplyLoadingViewModel.kt (1)
setupRetryFlow(67-72)
app/src/main/java/com/sopt/clody/data/remote/datasourceimpl/DiaryRemoteDataSourceImpl.kt (1)
app/src/main/java/com/sopt/clody/data/remote/util/SafeApiCall.kt (1)
safeApiCall(8-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (56)
app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt (1)
198-202: Manual verification required: Double-check all enableDraftAlarm call sitesI ran an automated search for any
enableDraftAlarm(invocations passing arguments (e.g.
rg --type kotlin --line-number 'enableDraftAlarm\('), but found no matches. Absence of hits isn’t proof—please manually review all call sites to ensure none still pass aContextparameter.app/src/main/java/com/sopt/clody/core/network/NetworkStatus.kt (1)
3-5: Kotlin toolchain is set to 2.1.0 (≥1.9)The version catalog confirms you’re on Kotlin 2.1.0, which satisfies the minimum requirement for
data object:• gradle/libs.versions.toml
– [versions] kotlin = "2.1.0"
– alias(libs.plugins.kotlin.android), kotlin-compose, kotlin-serialization all reference version “kotlin”No changes needed—
data objectis fully supported.app/src/main/java/com/sopt/clody/domain/usecase/SaveDraftDiaryUseCase.kt (1)
9-10: Cleaner expression body looks goodThe switch to an expression body is concise and keeps behaviour identical.
app/src/main/java/com/sopt/clody/domain/usecase/WriteDiaryUseCase.kt (1)
6-11: LGTM! Clean use case implementation.The use case follows clean architecture principles with proper dependency injection and direct delegation to the repository. The suspend operator function pattern is consistent with other use cases in the codebase.
app/src/main/java/com/sopt/clody/presentation/ui/login/LoginViewModel.kt (2)
17-17: Good addition of ErrorMessageProvider dependency.The constructor injection of
ErrorMessageProviderfollows the dependency injection pattern and supports the centralized error handling refactoring.Also applies to: 36-36
73-73: Consistent use of centralized error messages.The replacement of hardcoded error messages with
errorMessageProvider.getLoginFailedError()properly implements the centralized error handling pattern.Also applies to: 81-81
app/src/main/res/values-ko/strings.xml (1)
199-208: Well-organized error message strings.The new Korean error message strings are comprehensive and well-organized under a dedicated error section. The translations appear user-friendly and cover the main error scenarios needed for centralized error handling.
app/src/main/java/com/sopt/clody/presentation/utils/network/ErrorMessageModule.kt (1)
11-22: Proper Dagger Hilt module implementation.The module correctly provides a singleton
ErrorMessageProviderusing@ApplicationContextfor string resource access. The setup follows Dagger Hilt best practices and supports the centralized error handling architecture.app/src/main/java/com/sopt/clody/domain/usecase/FetchDraftDiaryUseCase.kt (2)
7-7: Improved parameter naming.The renamed parameter
diaryRepositoryis more descriptive than the previousrepository, improving code readability.
9-10: Clean single-expression function.The simplified single-expression function is more concise and readable while maintaining the same functionality. This aligns well with the direct delegation pattern used in other use cases.
app/src/main/res/values/strings.xml (1)
199-209: LGTM! Well-structured error messages for centralized error handling.The new error strings provide clear, user-friendly messages that align well with the centralized error handling approach. The messages are consistent in tone and provide appropriate guidance to users (e.g., "Please check your network connection").
app/src/main/java/com/sopt/clody/core/network/NetworkConnectivityModule.kt (1)
11-22: LGTM! Clean Dagger Hilt module following best practices.The module properly provides a singleton
NetworkConnectivityObserverwith the correct scope and context injection. The structure follows Dagger Hilt conventions well.app/src/main/java/com/sopt/clody/presentation/ui/writediary/screen/WriteDiaryViewModel.kt (3)
31-34: LGTM! Constructor properly updated for centralized error handling.The constructor correctly replaces
NetworkUtilwithNetworkConnectivityObserverand addsErrorMessageProviderinjection, aligning with the centralized error handling approach.
77-81: Good implementation of reactive network status checking.The use of
networkStatus.first()correctly obtains the current network status synchronously within the coroutine context. The approach maintains the reactive benefits while providing immediate status access when needed.
78-78: Excellent use of centralized error message provider.The consistent usage of
ErrorMessageProvidermethods throughout the ViewModel properly centralizes error message handling. The fallback pattern usinglocalizedMessagewhen available (lines 101, 234) provides good error handling coverage.Also applies to: 99-101, 220-220, 234-234
app/src/main/java/com/sopt/clody/core/network/NetworkConnectivityObserver.kt (3)
34-66: Excellent reactive network monitoring implementation.The
callbackFlowimplementation is well-structured with proper resource management. Key strengths:
- Immediate emission of current status provides responsive UX
- Proper callback registration/unregistration in
awaitClosedistinctUntilChanged()prevents duplicate emissions- Clean conversion from callback-based to reactive API
35-53: Robust network status detection in callbacks.The
NetworkCallbackimplementation correctly checks bothNET_CAPABILITY_INTERNETandNET_CAPABILITY_VALIDATEDcapabilities, ensuring actual internet connectivity rather than just network connection. The use oftrySendis appropriate for non-blocking operations in callbacks.
73-78: Consistent and defensive current status detection.The
isCurrentlyAvailable()method uses the same capability checks as the callback implementation, ensuring consistency. Proper null handling for both network and capabilities provides defensive programming against edge cases.app/src/main/java/com/sopt/clody/presentation/ui/diarylist/screen/DiaryListViewModel.kt (4)
5-8: LGTM: Clean dependency importsThe new imports for
NetworkConnectivityObserver,NetworkStatus, andErrorMessageProviderare correctly added to support the centralized error handling refactor.
20-24: LGTM: Constructor properly updated for dependency injectionThe constructor correctly replaces
NetworkUtilwithNetworkConnectivityObserverand addsErrorMessageProvider, aligning with the centralized error handling approach.
49-49: LGTM: Centralized error message usageError messages are now properly retrieved from the
ErrorMessageProviderinstead of hardcoded strings, successfully centralizing error message management and enabling localization.Also applies to: 61-61, 64-64, 66-66, 90-90, 102-102, 104-104
48-51: Network connectivity check pattern verified across ViewModelsVerification confirms that every ViewModel consistently uses
networkConnectivityObserver.networkStatus.first() == NetworkStatus.{Available,Unavailable}
(e.g. WriteDiaryViewModel, TimeReminderViewModel, SignUpViewModel, HomeViewModel, DiaryListViewModel, AccountManagementViewModel, NotificationSettingViewModel, ReplyLoadingViewModel, ReplyDiaryViewModel).
No inconsistencies found—approving these changes.app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderViewModel.kt (4)
9-11: LGTM: Consistent dependency importsThe imports align with the centralized error handling pattern established across the codebase.
Also applies to: 15-15
24-28: LGTM: Constructor follows the established patternThe constructor properly replaces
NetworkUtilwith the new dependencies, maintaining consistency with other ViewModels in the refactor.
38-41: LGTM: Network connectivity check implementationThe network status checking follows the same pattern as other ViewModels, ensuring consistency across the application.
64-64: LGTM: Centralized error message usageError messages are properly retrieved from the provider, supporting the centralization goal.
Also applies to: 66-66
app/src/main/java/com/sopt/clody/presentation/utils/network/ErrorMessageProvider.kt (1)
1-16: Well-designed interface for centralized error handlingThe
ErrorMessageProviderinterface provides a comprehensive set of methods for various error scenarios. The inclusion of anApiError-specific method shows good integration with the existing error handling infrastructure.The interface successfully abstracts error message retrieval, enabling:
- Centralized error message management
- Localization support
- Consistent error handling across the app
app/src/main/java/com/sopt/clody/presentation/ui/replyloading/screen/ReplyLoadingViewModel.kt (5)
7-8: LGTM: Consistent dependency importsThe imports follow the established pattern for the centralized error handling refactor.
Also applies to: 14-14
28-34: LGTM: Constructor properly updatedThe constructor correctly integrates the new dependencies while maintaining the existing functionality.
99-101: LGTM: Clean destructuring assignmentThe destructuring assignment with
map { it.toInt() }is a clean improvement over the previous implementation.
112-118: LGTM: Enhanced error handling with centralized messagesThe error handling now properly uses the
ErrorMessageProviderfor bothApiErrorinstances and generic errors, with improved logging that includes the throwable.
140-140: LGTM: Consistent error message usageAd-related error messages now use the centralized provider, maintaining consistency across the application.
Also applies to: 154-154
app/src/main/java/com/sopt/clody/presentation/ui/auth/signup/SignUpViewModel.kt (4)
11-12: LGTM: Consistent dependency importsThe imports align with the centralized error handling pattern used across other ViewModels.
Also applies to: 21-21
40-40: LGTM: Proper dependency injection in MvRx ViewModelThe constructor correctly integrates the new dependencies while maintaining compatibility with the MvRx pattern and assisted injection.
Also applies to: 42-42
152-155: LGTM: Network connectivity check implementationThe network status checking follows the established pattern and properly updates the state with centralized error messages.
165-165: LGTM: Centralized error message usageAll error scenarios now use the
ErrorMessageProvider, successfully centralizing error message management across the signup flow.Also applies to: 188-188, 205-205
app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/AccountManagementViewModel.kt (2)
6-30: LGTM! Clean dependency injection setup.The constructor properly injects the new
NetworkConnectivityObserverandErrorMessageProviderdependencies, aligning with the centralized error handling architecture.
65-67: Network check implementation looks good.The asynchronous network status check using
first()correctly retrieves the current network state before proceeding with the API call.app/src/main/java/com/sopt/clody/presentation/ui/replydiary/ReplyDiaryViewModel.kt (4)
37-50: Well-implemented retry mechanism with proper throttling.The retry flow setup correctly prevents rapid successive retries with a 2-second throttle and is properly scoped to the ViewModel lifecycle.
61-65: Clean network connectivity check with proper early return.Good implementation - the network check correctly returns early when unavailable, preventing unnecessary API calls.
79-79: Good null safety handling for content field.Using
orEmpty()ensures the content field is never null, preventing potential NPEs downstream.
87-93: Excellent error handling with proper type discrimination.The error handling correctly distinguishes between
ApiErrorand general exceptions, providing appropriate error messages for each case. The Timber logging aids debugging.app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeViewModel.kt (4)
116-118: Clean error state management with sensible defaults.Good use of default parameters to ensure consistent error messaging while maintaining flexibility.
122-125: Consistent network checking pattern across methods.Good extraction of network checking logic and consistent error handling pattern with early returns.
273-276: Well-implemented network availability helper.The suspend function properly encapsulates the network status check, promoting code reuse across the ViewModel.
259-271: enableDraftAlarm signature update verifiedAll usages of
enableDraftAlarm()have been updated to the new no-argument signature:
- app/src/main/java/com/sopt/clody/presentation/ui/home/screen/HomeScreen.kt:
homeViewModel.enableDraftAlarm()No further changes are required.
app/src/main/java/com/sopt/clody/presentation/utils/network/ErrorMessageProviderImpl.kt (1)
1-52: Clean implementation of centralized error message provider.The implementation properly uses Android string resources for localization and follows a consistent pattern across all methods.
app/src/main/java/com/sopt/clody/data/remote/datasourceimpl/DiaryRemoteDataSourceImpl.kt (1)
11-42: Excellent centralization of API error handling.The consistent use of
safeApiCallacross all methods effectively centralizes error handling and response wrapping. The implementation is clean and follows the single responsibility principle.app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingViewModel.kt (2)
6-7: LGTM! Clean migration to centralized abstractions.The imports correctly introduce the new
NetworkConnectivityObserver,NetworkStatus, andErrorMessageProviderabstractions, along with the requiredfirst()extension for Flow operations. The constructor changes properly replace the deprecatedNetworkUtilwith the new reactive network observer and error message provider.Also applies to: 13-13, 17-17, 24-25
67-67: Excellent centralization of error messages.All hardcoded error messages have been properly replaced with calls to
ErrorMessageProvider. The error categorization (network, temporary, unknown) is consistent and the implementation correctly handles different error scenarios based on HTTP status codes.Also applies to: 83-83, 86-86, 88-88, 100-100, 135-135, 137-137, 150-150, 173-173, 175-175
app/src/test/java/com/sopt/clody/datasource/FakeDiaryRemoteDataSource.kt (2)
20-20: Return types correctly migrated to Result.All method signatures have been properly updated from
ApiResponse<T>toResult<T>, maintaining consistency with the interface changes in the centralized error handling refactor.Also applies to: 24-24, 28-28, 32-32, 36-36, 40-40, 44-44
59-61: LGTM! Safe Result implementation.The
saveDraftDiarymethod correctly returnsResult<Unit>and safely handles the null check without potential null pointer exceptions.app/src/main/java/com/sopt/clody/data/repositoryimpl/DiaryRepositoryImpl.kt (3)
12-13: Excellent architectural simplification.The direct delegation to
diaryRemoteDataSourceeffectively removes boilerplate error handling from the repository layer, aligning with the centralized error handling approach. This moves error management to the more appropriate remote data source layer.Also applies to: 15-16, 18-19, 21-22, 24-25, 27-28, 30-31
33-34: LGTM! Proper Result transformation.The use of
.map { it.toDomain() }correctly transforms the success value while preserving error handling in theResult<T>type. This maintains the repository's domain mapping responsibility.
36-39: Clean separation of concerns.The method correctly handles DTO creation (repository responsibility) before delegating the API call to the remote data source. The direct return of the
Result<Unit>maintains consistency with the new error handling approach.app/src/main/java/com/sopt/clody/data/remote/datasource/DiaryRemoteDataSource.kt (1)
13-22: DiaryRemoteDataSource implementations updated toResult<T>Verified that both the production and test implementations now match the new interface signatures:
• DiaryRemoteDataSourceImpl (app/src/main/java/.../DiaryRemoteDataSourceImpl.kt) uses
safeApiCall(...)to returnResult<T>.
• FakeDiaryRemoteDataSource (app/src/test/java/.../FakeDiaryRemoteDataSource.kt) overrides all methods asResult<T>.No remaining
ApiResponse<T>return types in anyDiaryRemoteDataSourceimplementation.
| data class ApiError( | ||
| override val message: String, | ||
| ) : Exception() |
There was a problem hiding this comment.
ApiError won’t compile – illegal override of nullable message
Exception.message is declared as String?. Overriding it with a non-nullable String violates the Liskov rule and the Kotlin compiler rejects it.
-data class ApiError(
- override val message: String,
-) : Exception()
+data class ApiError(
+ val errorMessage: String,
+) : Exception(errorMessage)This preserves the non-null guarantee for callers via errorMessage while delegating to the base Exception correctly.
🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/data/remote/util/ApiError.kt around lines 3
to 5, the override of the Exception.message property is declared as non-nullable
String, but Exception.message is nullable String?, causing a compilation error.
Change the override to be nullable String? to match the base class signature,
and if a non-nullable message is needed, add a separate property like
errorMessage that guarantees non-nullability while delegating the nullable
message to the base Exception.
| val errorMessage = if (it.message?.contains("200") == false) { | ||
| FAILURE_TEMPORARY_MESSAGE | ||
| errorMessageProvider.getTemporaryError() | ||
| } else { | ||
| UNKNOWN_ERROR | ||
| errorMessageProvider.getUnknownError() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error type detection logic.
The current approach of checking if the error message contains "200" is fragile and could break if error message formats change. Consider using typed errors or error codes instead.
This would be more robust if the repository returned typed errors that can be pattern matched:
onFailure = { throwable ->
retryCount++
if (retryCount >= maxRetryCount) {
UserInfoState.Failure(errorMessageProvider.getTemporaryError())
} else {
val errorMessage = when (throwable) {
is ApiError -> errorMessageProvider.getApiError(throwable)
is NetworkException -> errorMessageProvider.getNetworkError()
else -> errorMessageProvider.getUnknownError()
}
UserInfoState.Failure(errorMessage)
}
}🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/AccountManagementViewModel.kt
around lines 81 to 85, the error handling currently checks if the error message
contains "200", which is fragile. Refactor this to use typed exceptions or error
codes by updating the repository to return specific error types. Then, replace
the string check with a when expression that matches on the throwable type
(e.g., ApiError, NetworkException) and calls the appropriate error message
provider method for each type, falling back to an unknown error for others.
| if (networkConnectivityObserver.networkStatus.first() == NetworkStatus.Unavailable) { | ||
| _failureDialogMessage.value = errorMessageProvider.getNetworkError() | ||
| _showFailureDialog.value = true | ||
| } |
There was a problem hiding this comment.
Add return statement after network unavailability detection.
When network is unavailable, the method should return early to prevent unnecessary state updates and API call attempts.
if (networkConnectivityObserver.networkStatus.first() == NetworkStatus.Unavailable) {
_failureDialogMessage.value = errorMessageProvider.getNetworkError()
_showFailureDialog.value = true
+ return@launch
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (networkConnectivityObserver.networkStatus.first() == NetworkStatus.Unavailable) { | |
| _failureDialogMessage.value = errorMessageProvider.getNetworkError() | |
| _showFailureDialog.value = true | |
| } | |
| if (networkConnectivityObserver.networkStatus.first() == NetworkStatus.Unavailable) { | |
| _failureDialogMessage.value = errorMessageProvider.getNetworkError() | |
| _showFailureDialog.value = true | |
| return@launch | |
| } |
🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/AccountManagementViewModel.kt
around lines 95 to 98, after detecting that the network status is unavailable,
add a return statement immediately to exit the method early. This prevents
further state updates and avoids making unnecessary API calls when there is no
network connectivity.
| if (networkConnectivityObserver.networkStatus.first() == NetworkStatus.Unavailable) { | ||
| _failureDialogMessage.value = errorMessageProvider.getNetworkError() | ||
| _showFailureDialog.value = true | ||
| } |
There was a problem hiding this comment.
Add return statement after network unavailability detection.
Similar to changeNickname, this method should return early when network is unavailable.
if (networkConnectivityObserver.networkStatus.first() == NetworkStatus.Unavailable) {
_failureDialogMessage.value = errorMessageProvider.getNetworkError()
_showFailureDialog.value = true
+ return@launch
}🤖 Prompt for AI Agents
In
app/src/main/java/com/sopt/clody/presentation/ui/setting/screen/AccountManagementViewModel.kt
around lines 146 to 149, after detecting that the network status is unavailable
and setting the failure dialog message and visibility, add a return statement to
exit the method early. This prevents further execution when the network is down,
consistent with the behavior in the changeNickname method.
| ): Result<DraftDiariesResponseDto> { | ||
| return draftDiariesResponse?.let { Result.success(it.data!!) } | ||
| ?: Result.failure(ApiError("draftDiariesResponse not set")) |
There was a problem hiding this comment.
Fix potential null pointer exception.
The non-null assertion it.data!! could cause a crash if the data field is null, even when the ApiResponse object exists.
Apply this diff for safer null handling:
- return draftDiariesResponse?.let { Result.success(it.data!!) }
+ return draftDiariesResponse?.data?.let { Result.success(it) }
?: Result.failure(ApiError("draftDiariesResponse not set"))🤖 Prompt for AI Agents
In app/src/test/java/com/sopt/clody/datasource/FakeDiaryRemoteDataSource.kt
around lines 52 to 54, the code uses a non-null assertion on it.data which can
cause a crash if data is null. To fix this, replace the non-null assertion with
a safe call and handle the null case by returning a failure Result with an
appropriate error message when data is null, ensuring no potential null pointer
exceptions occur.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/main/java/com/sopt/clody/domain/usecase/WriteDiaryUseCase.kt (1)
9-11: Consider adding business logic or removing unnecessary abstraction.This use case currently acts as a thin wrapper around the repository call without adding business value. Consider either:
- Add domain logic: Include validation, business rules, or data transformation
- Remove if unnecessary: If no business logic is needed, consider calling the repository directly from ViewModels
Additionally, consider more domain-specific parameter types or validation:
-suspend operator fun invoke(lang: String, date: String, content: List<String>) = +suspend operator fun invoke(language: String, date: String, content: List<String>): Result<Unit> { + require(content.isNotEmpty()) { "Diary content cannot be empty" } + require(date.isNotBlank()) { "Date cannot be blank" } + return diaryRepository.writeDiary(language, date, content) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/sopt/clody/domain/usecase/WriteDiaryUseCase.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
app/src/main/java/com/sopt/clody/domain/usecase/WriteDiaryUseCase.kt (2)
1-4: LGTM!Package structure and imports are appropriate for a domain layer use case following Clean Architecture patterns.
6-8: LGTM!Constructor injection is properly implemented with appropriate dependency on the repository layer.
📌 ISSUE
closed #297
📄 Work Description
에러 핸들링 중앙화 해버리기
네트워크 감지 추가
✨ PR Point
📸 ScreenShot/Video
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Documentation
Tests