Improving code quality - Session 11: Even functions get tired of calls

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.

Even functions get tired of calls

Let's assume we are implementing a feature where users can become "friends" with each other. To check if another user is a friend or to add a new friend, we use the following UseCase class.

class FriendStateUseCase(
    private val currentUserId: UserId,
    ...
) {
    fun isFriend(userId: UserId): Boolean {
        ...
    }

    fun markAsFriend(userId: UserId) {
        ...
    }
}

The calling code looks like this:

if (!friendStateUseCase.isFriend(someUserId)) {
    friendStateUseCase.markAsFriend(someUserId)
}

Is there a problem with this code?

Don't ask again and again

When we extract the structure of the calling code, it becomes if (receiver.a()) { receiver.b() }.

In other words, the code changes the receiver based on its current state.

In such cases, it's often better to let the function that changes the state perform the check itself.

In the case of friendStateUseCase.markAsFriend, it seems to expect that !isFriend is already satisfied. If calling markAsFriend when the user is already a friend throws an exception, a bug will occur if the !isFriend check is missed. To solve this, perform the !isFriend check inside markAsFriend.

fun markAsFriend(userId: UserId) {
    if (isFriend(userId)) {
        return
    }

    // Actual logic to mark `userId` as a "friend".
}

It's often natural for markAsFriend to do nothing if the user is already a friend. However, if it's important to emphasize "doing nothing," you should rename the function to markAsFriendIfNotYet or document the condition.

By letting the receiver check its own state, you can hide unnecessary state and simplify the apparent state transitions.

Ask with return values

Consider refactoring the following code:

if (!friendStateUseCase.isFriend(someUserId)) {
    friendStateUseCase.markAsFriend(someUserId)
    showEventPopup(Event.NEW_FRIEND) // To notify user the operation was successfully finished.
}

Refactoring the above code as follows is inappropriate.

class FriendStateUseCase(...) {

    fun markAsFriend(
        userId: UserId,
        onSucceeded: () -> Unit
    ) {
        if (isFriend(userId)) {
            return
        }

        ... // Actual logic to mark `userId` as a "friend".
        onSucceeded()
    }
}

// Caller 
friendStateUseCase.markAsFriend(someUserId) {
    showEventPopup(Event.NEW_FRIEND)
}

This refactoring of FriendStateUseCase.markAsFriend is inappropriate because the onSucceeded callback creates unnecessary cyclic dependencies. Additionally, it's unclear whether onSucceeded is a synchronous or asynchronous callback.

In such cases, it's better to return the result as a return value rather than making markAsFriend a higher-order function. In this case, a Boolean is sufficient.

/** ... */
fun markAsFriend(userId: UserId): Boolean {
    if (isFriend(userId)) {
        return false
    }

    ... // Actual logic to mark `userId` as a "friend".
    return true
}

// Caller
val isNewlyMarkedAsFriend = friendStateUseCase.markAsFriend(someUserId)
if (isNewlyMarkedAsFriend) {
    showEventPopup(Event.NEW_FRIEND)
}

This way, it's clear when showEventPopup(Event.NEW_FRIEND) is called.

However, if the function name doesn't mention the return value, you need to document the return value (see Writing Readable Code Session 3). Also, enforce return value checks as needed (see Have you double-checked your code? Option 1).

---

Summary

It's sometimes better to perform state checks within the receiver's function.

Keywords: state check logic, responsibility, implicit precondition