The original article was published on January 16, 2025.
Hello, I'm Munetoshi Ishikawa, a mobile client developer for the LINE messaging app.
This article is the latest installment of our weekly series "Improving code quality". For more information about the Weekly Report, please see the first article.
Silence may be golden
Suppose there is a data model that represents the result of an API call as follows.
package ...
import ...ApiErrorType
/** ... */
sealed interface ApiResult<T> {
class Success<T>(val value: T) : ApiResult<T>
class Failed(val errorType: ApiErrorType) : ApiResult<Nothing>
}Defining functions like map and flatMap on such a data model that represents a "potentially failing" result can be very convenient, as it allows chaining operations.
In the following example, mapSuccess and flatMap are added to ApiResult. Both are intended to allow writing code that performs the next operation in case of success using method chaining.
@file:Suppress("warnings")
package ...
import ...ApiErrorType
/** ... */
sealed interface ApiResult<T> {
class Success<T>(val value: T) : ApiResult<T>
class Failed(val errorType: ApiErrorType) : ApiResult<Nothing>
/** ... */
fun <S> mapSuccess(transform: (T) -> S): ApiResult<S> = when (this) {
is Success -> Success(transform(value))
is Failed -> this as ApiResult<S>
}
/** ... */
fun <S> flatMap(transform: (T) -> ApiResult<S>): ApiResult<S> = when (this) {
is Success -> transform(value)
is Failed -> this as ApiResult<S>
}
}Is there any problem with this code?
Bitter medicine is hard to swallow
This code suppresses all warnings for the entire file with @file:Suppress("warnings"). However, the only warning we actually want to suppress is the "unchecked cast" that occurs with as ApiResult<S>. Suppressing all warnings for the entire file may unintentionally suppress future warnings, potentially overlooking bugs. For example, you might miss that a function you are using has become deprecated due to a library update.
It is important to not ignore and not excessively suppress warnings. When a warning occurs, you need to do one of the following (though option 1 should be prioritized):
- Resolve the cause of the warning
- Limit the scope of suppression and explain the reason for suppression in a comment
1: Resolve the cause of the warning
If possible, it is desirable to resolve the cause of the warning without suppressing it. In this example, you can resolve the warning by making the type parameter T covariant and removing the downcast. In Kotlin, you can make T covariant by using the out modifier as follows.
/** ... */
sealed interface ApiResult<out T> {
class Success<T>(val value: T) : ApiResult<T>
class Failed(val errorType: ApiErrorType) : ApiResult<Nothing>
/** ... */
fun <S> mapSuccess(transform: (T) -> S): ApiResult<S> = when (this) {
is Success -> Success(transform(value))
is Failed -> this
}
/** ... */
fun <S> flatMap(transform: (T) -> ApiResult<S>): ApiResult<S> = when (this) {
is Success -> transform(value)
is Failed -> this
}
}If it is a situation where T should not be covariant (should remain invariant), you can change the type parameter of Failed from Nothing to T and recreate the Failed instance each time map or flatMap is called.
sealed interface ApiResult<T> {
class Success<T>(val value: T) : ApiResult<T>
class Failed<T>(val errorType: ApiErrorType) : ApiResult<T>
/** ... */
fun <S> mapSuccess(transform: (T) -> S): ApiResult<S> = when (this) {
is Success -> Success(transform(value))
is Failed -> Failed(errorType)
}
/** ... */
fun <S> flatMap(transform: (T) -> ApiResult<S>): ApiResult<S> = when (this) {
is Success -> transform(value)
is Failed -> Failed(errorType)
}
}2: Limit the scope of suppression and explain the reason in a comment
When you cannot resolve the cause of the warning, you may have no choice but to suppress it. Situations where the cause of the warning is due to an external library or framework are typical examples. There may also be situations where you cannot think of a solution. In such cases, please keep the following three points in mind:
- Limit the scope of suppression: Choose suppression at the class, function, or statement level rather than file-level suppression
- Limit the warnings to be suppressed: Instead of suppressing all warnings with "warnings" or "all", specify the type of warning, such as "UNCHECKED_CAST"
- Write a comment explaining the reason: Explain in a comment why suppression is necessary and why the cause cannot be resolved
Even if you cannot resolve the cause of the warning while writing the code, by clearly stating the reason for suppression in a comment, code reviewers or other developers may come up with a solution. In the example of ApiResult, by specifying the reason for "UNCHECKED_CAST" as follows, someone might come up with a solution like using covariance or recreating instances.
@Suppress("UNCHECKED_CAST") // To cast `Failed` from `ApiResult<T>` to `<S>` to reuse the instance.
fun <S> flatMap(transform: (T) -> ApiResult<S>): ApiResult<S> = when(this) {
is Success -> transform(value)
is Failed -> this as ApiResult<S>
}In a nutshell
Attempt to resolve the cause of warnings first, and if suppression is necessary, limit the scope and write a comment.
Keywords: warning, suppression, comment