Improving code quality - Session 51: Convincing questions

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

Convincing questions

Let's assume a developer has requested a code review for the following function getOnlineUserStates.

fun getOnlineUserStates(userIds: Set<UserId>): Map<UserId, UserState> {
    if (userIds.isEmpty()) {
        return emptyMap()
    }
    
    return userIds.asSequence()
        .map { ... /* Convert to Pair<UserId, UserState> */ }
        .filter { ... /* Filter only online users */ }
        .toMap()
}

This function returns the states of online users as a Map. The initial if statement performs an early return when userIds is empty.

However, the process following asSequence() works correctly even if userIds is empty, and the performance improvement from this early return is quite limited. Therefore, the reviewer thought it would be better to remove this early return to prioritize code simplicity and sent the following review comment.

Reviewer

What is the purpose of this early return?

In response to this comment, the developer who requested the review (the author) replied as follows.

Author

When userIds is empty, the process after asSequence() is unnecessary, so I applied an early return.

The following is the exchange of comments between the reviewer and the author.

Reviewer

However, the possibility of userIds being empty is not very high, and the computation time for asSequence() on empty userIds is sufficiently short. Is this early return still necessary?

Author

Yes, although the effect is small, the performance should definitely improve, so this early return is necessary.

In this exchange, it seems that the reviewer's intention is not being effectively communicated to the author. Where is the problem?

Do you understand what I mean?

One cause of this miscommunication is that the removal of the early return is a "request" from the reviewer, yet it can be interpreted as a mere "question". From the author's perspective, even though they are answering the question, the focus of the discussion may not seem clear. In regular writing or conversation, questions play an important role in making expressions indirect or polite. However, in code reviews, requests and questions should be clearly distinguishable.

Additionally, when making requests or suggestions, it is even better if you can explain their benefits. However, the extent of explanation required depends on the background and context shared with the author.

Reviewer

Since the logic after asSequence() works correctly even when userIds is empty, please remove this early return. (When userIds is empty, having an early return slightly improves performance, but its effect is limited. In such cases, I believe prioritizing code simplicity over performance has greater advantages. Reference: https://speakerdeck.com/munetoshi/code-readability-session-1-ver-2-ja?slide=44 )

Of course, if there are points you are unsure about or do not understand, it is appropriate to ask questions. Leaving unclear points unaddressed can lead to oversights and miscommunication due to incorrect assumptions. Additionally, by asking the author to investigate unclear points, you can balance the workload between the reviewer and the author.

Instead of making every review comment in the form of a question, it is necessary to choose a format that conveys the intention. To do so, when writing review comments, always consider whether the comment can be classified as a request, suggestion, or question.

What is that comment?

To clarify whether a review comment is a request, suggestion, or question, using specific phrases or tags is a good method.

• Request: "Please do...", "We have/need to do...", [Request], [Requirement], [Must]
• Suggestion: "It would be nice to do...", "An option is to do...", [Suggestion], [Optional], [Nice to have], [IMO]
• Question: [Question], [Confirmation], [Just curious]

In a nutshell

When writing review comments, consider whether they correspond to a request, suggestion, or question.

Keywords: code review, review comment, communication

Read other articles on techniques for improving code quality

List of articles on techniques for improving code quality