The original article was published on September 26, 2024.
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.
Return of values
The following registerNewUser function is designed to register a new user, as its name suggests. This function consists of two steps.
- Create a new user account using
userAttributes - Upload a profile image using
userAttributes.profileImageUri
fun registerNewUser(userAttributes: UserAttributes): RegistrationResult {
val accountCreationRequestParams = AccountCreationRequestParams(
userName = userAttributes.userName,
accountType = userAttributes.accountType,
referrer = userAttributes.referrer,
creationTimestamp = currentTimeProvider()
)
val creationRequestResult = serviceApi.requestCreatingAccount(accountCreationRequestParams)
if (creationRequestResult !is AccountCreationRequestResult.Success) {
return RegistrationResult.ERROR
}
val profileImageBitmap = userAttributes.profileImageUri
?.let(resourceResolver::getSource)
?.let(ImageDecoder::decode)
val isProfileImageUploaded = profileImageBitmap != null &&
serviceApi.uploadProfileImage(
creationRequestResult.userId,
profileImageBitmap
)
return if (isProfileImageUploaded) {
RegistrationResult.REGISTERED_WITH_CUSTOMIZED_IMAGE
} else {
RegistrationResult.REGISTERED_WITH_DEFAULT_IMAGE
}
}This function is somewhat large, making it difficult to immediately see that there are two steps involved. To emphasize the two steps, a developer created the following auxiliary functions. The user account is created with requestCreatingAccount, and the profile image is uploaded with requestUploadingProfileImage.
sealed interface AccountCreationResult {
class Success(val userId: String): AccountCreationResult
class Failure(val error: RegistrationResult): AccountCreationResult
}
fun registerNewUser(userAttributes: UserAttributes): RegistrationResult {
val creationResult = requestCreatingAccount(userAttributes)
if (creationResult is AccountCreationResult.Failure) {
return creationResult.error
}
val userId = (creationResult as? AccountCreationResult.Success)?.userId
return requestUploadingProfileImage(userId, userAttributes)
}
private fun requestCreatingAccount(userAttributes: UserAttributes): AccountCreationResult {
val requestParams = AccountCreationRequestParams(
userName = userAttributes.userName,
accountType = userAttributes.accountType,
referrer = userAttributes.referrer,
creationTimestamp = currentTimeProvider()
)
val creationRequestResult = serviceApi.requestCreatingAccount(requestParams)
return if (creationRequestResult is AccountCreationRequestResult.Success) {
AccountCreationResult.Success(creationRequestResult.userId)
} else {
AccountCreationResult.Failure(RegistrationResult.ERROR)
}
}
private fun requestUploadingProfileImage(
userId: String?,
userAttributes: UserAttributes,
): RegistrationResult {
if (userId == null) {
return RegistrationResult.REGISTERED_WITH_DEFAULT_IMAGE
}
val profileImageBitmap = userAttributes.profileImageUri
?.let(resourceResolver::getSource)
?.let(ImageDecoder::decode)
val isProfileImageUploaded = profileImageBitmap != null &&
serviceApi.uploadProfileImage(userId, profileImageBitmap)
return if (isProfileImageUploaded) {
RegistrationResult.REGISTERED_WITH_CUSTOMIZED_IMAGE
} else {
RegistrationResult.REGISTERED_WITH_DEFAULT_IMAGE
}
}By doing this, it emphasizes that registerNewUser consists of two steps. However, it also introduces a point of confusion. Where is that?
Where is the king's responsibility?
The extracted code has a problem: "The final return value (RegistrationResult) is not clear just by reading registerNewUser." Depending on the conditions, either requestCreatingAccount or requestUploadingProfileImage determines the return value. This means that knowledge about the return value is dispersed across auxiliary functions. If you want to change the return value specification in the future, the scope of changes will be larger, and mistakes are more likely to occur.
In many cases, it is better not to disperse knowledge about the return value and to consolidate it in one place. In this case, only registerNewUser needs to know about RegistrationResult, and it is not a problem if the return types of other functions are different. In the following implementation, requestCreatingAccount and requestUploadingProfileImage use null as the error value, and registerNewUser converts it to RegistrationResult.
fun registerNewUser(userAttributes: UserAttributes): RegistrationResult {
val userId = requestCreatingAccount(userAttributes)
?: return RegistrationResult.ERROR
val hasProfileImage = requestUploadingProfileImage(userId, userAttributes.profileImageUri)
return if (hasProfileImage) {
RegistrationResult.REGISTERED_WITH_CUSTOMIZED_IMAGE
} else {
RegistrationResult.REGISTERED_WITH_DEFAULT_IMAGE
}
}
/**
* Sends a request to create a new account with ...
* This returns the user ID if a new user was successfully created, otherwise returns null.
*/
private fun requestCreatingAccount(userAttributes: UserAttributes): String? {
val requestParams = AccountCreationRequestParams(
userName = userAttributes.userName,
accountType = userAttributes.accountType,
referrer = userAttributes.referrer,
creationTimestamp = currentTimeProvider()
)
val creationRequestResult = serviceApi.requestCreatingAccount(requestParams)
return (creationRequestResult as? AccountCreationRequestResult.Success)?.userId
}
/**
* Uploads the profile image for the new account with ...
* Then, returns true if the upload finishes successfully; false otherwise.
*/
private fun requestUploadingProfileImage(
userId: String,
profileImageUri: Uri?,
): Boolean {
val profileImageBitmap = profileImageUri
?.let(resourceResolver::getSource)
?.let(ImageDecoder::decode)
return profileImageBitmap != null && serviceApi.uploadProfileImage(userId, profileImageBitmap)
}By doing this, you can clarify the flow of registerNewUser and understand the return value without reading other functions. Even if you want to change the specification of RegistrationResult, the direct impact on requestCreatingAccount or requestUploadingProfileImage is likely to be minimal.
In a nutshell
Sometimes it is better to consolidate the code that determines the return value in one place.
Keywords: return value, function responsibility, specification knowledge