Improving code quality - Session 57: Seeing is believing

The original article was published on February 6, 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.

Seeing is believing

Suppose you are implementing an application that sends and receives text messages. In the following sendMessage function, it converts the received MessageModel argument and calls an API that sends it as a query parameter.

private fun sendMessage(messageModel: MessageModel): SendResult {
    val sendRequestParams = toSendRequestParams(messageModel)

    // ... snip...
    return try {
        val response = apiClient.sendMessage(sendRequestParams)
        SendResult.Success(response)
    } catch (e: ApiException) {
        SendResult.Failure(e.toErrorType())
    }
}

Now, let's say you received a code review request for the diff shown below. This change adds an early return code that returns INVALID_MESSAGE when the message text is an empty string or only whitespace.

    private fun sendMessage(messageModel: MessageModel): SendResult {
+       if (messageModel.text.isBlank()) {
+           return SendResult.Failure(MessageErrorType.INVALID_MESSAGE)
+       }
+
        val sendRequestParams = toSendRequestParams(messageModel)

        // ... snip...
        return try {
            val response = apiClient.sendMessage(sendRequestParams)
            SendResult.Success(response)
        } catch (e: ApiException) {
            SendResult.Failure(e.toErrorType())
        }
    }

Are there any points to be aware of when you receive such a code review request?

Trying to see the invisible

When code that "filters by condition" is added, you should check if there are similar condition filters elsewhere. More generally, when there is a code change, you should check if there is similar code in the same scope, caller, or callee. In this example, it would be good to check the following possibilities:

1. The caller of sendMessage already excludes the isBlank case.
2. toSendRequestParams returns an "invalid" parameter object when isBlank. (Passing that invalid parameter to apiClient.sendMessage returns MessageErrorType.INVALID_MESSAGE.)

Also, note that the filter conditions at the caller/callee may partially overlap. The following toSendRequestParams filters the empty string condition but does not filter the whitespace-only case. In this case, you might overlook the overlap of filter conditions.

fun toSendRequestParams(messageModel: MessageModel): SendRequestParams {
    if (messageModel.text.isEmpty()) {
        return SendRequestParams.INVALID
    }

    ...
    return ...
}

In statically typed languages like Kotlin, to avoid overlooking condition overlaps, it is good to separate filtered types and error types. In this example, instead of creating a null object like SendRequestParams.INVALID to signify an error, using null can more explicitly distinguish errors.

private fun sendMessage(messageModel: MessageModel): SendResult {
    val sendRequestParams = toSendRequestParams(messageModel)
        ?: return SendResult.Failure(MessageErrorType.INVALID_MESSAGE)

    // ... snip...
    return try {
        val response = apiClient.sendMessage(sendRequestParams)
        SendResult.Success(response)
    } catch (e: ApiException) {
        SendResult.Failure(e.toErrorType())
    }
}

/** ... */
fun toSendRequestParams(messageModel: MessageModel): SendRequestParams? {
    if (messageModel.text.isBlank()) {
        return null
    }

    ...
    return ...
}

Telescope for seeing

In code reviews, it is important to compare with existing code to make better suggestions. In particular, it is good to check the following parts:

• The caller and callee of the changed code
• Parent/child classes or sibling classes of the changed class
• Functions or classes with similar names/roles/responsibilities to the changed function or class

To make these reviews easier, it is recommended to check out the code changes and review them in an IDE. If you are using GitHub, you can fetch the pull request with the following git command.

git fetch git@[git domain]:[organization]/[repository name].git refs/pull/[pull request number]/head

If git remote is set correctly, you can simplify it as follows.

git fetch [remote] pull/[pull request number]/head

To make it even easier, you can prepare a script like the following to check out from the pull request number. (Here, it is assumed that the remote name to check out from is origin. Change it as appropriate according to the remote name.)

#!/bin/bash -e
git fetch origin pull/$1/head; git switch --detach FETCH_HEAD

In a nutshell

When doing a code review, check not only the changes themselves but also the surrounding code.

Keywords: code review, code change, diff checkout

Read other articles on techniques for improving code quality

List of articles on techniques for improving code quality