Improving code quality - Session 25: TL;DR: So... what's the point?

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.

TL;DR: So... what's the point?

Let's say a developer created the following class and requested a code review.

data class UserModel(
    val userId: Int,
    val accountName: String,
    val ...
    var onlineStatus: OnlineStatus,
    var statusMessage: String
)

The reviewer commented as follows:

The data class has a copy function. If you want to change the value of a property, you can call that function to create a new instance instead of modifying the property directly.

For UserModel.onlineStatus and statusMessage, it seems you're making them reassignable with var without using copy because these values are frequently updated, and you want to share the updated state by having the same object.

However, generally speaking, sharing mutable objects can lead to bugs due to properties changing at unintended times. It's usually better to create a new instance each time the state is updated to maintain robustness.

In conclusion, I think all properties of this class should be val. You might worry about the cost of copying instances, but I don't think the state of UserModel is updated that often. Moreover, since it seems that only onlineStatus and statusMessage are frequently updated, it might be better to separate them from other stable properties. For example, how about doing it like this:

data class UserModel(
   val userId: Int,
   val accountName: String,
   val ...
)

class UserStatus(
   val userModel: UserModel,
   val onlineStatus: OnlineStatus,
   val statusMessage: String
)

How can this review comment be improved?

In other words... basically...

In review comments, it's better to first write suggestions or requests, and then supplement with reasons.

In this case, the improved comment would be as follows:

For UserModel, separate the class based on the frequency of value updates, and make all properties val.

data class UserModel(
   val userId: Int,
   val accountName: String,
   val ...
)

class UserStatus(
   val userModel: UserModel,
   val onlineStatus: OnlineStatus,
   val statusMessage: String
)

This change is based on two ideas.

1. Immutability of objects: It's easier to prevent bugs from properties changing at unintended times by using immutable objects for each state update rather than sharing mutable objects. Also, defining var properties in a data class can cause confusion with the use of copy, so it's safer to avoid it. (For more details, refer to https://...)

2. Lifecycle of values: onlineStatus and statusMessage seem to be updated more frequently than other properties. By separating frequently updated properties from less frequently updated ones, it's easier to prevent incorrect updates.

By presenting suggestions or requests first, the review requester can understand the main points of the comment initially and then verify the validity of the reasons. This approach reduces the need for rereading.

In the improved comment, there's also an effort to explain the "reasons". It starts by indicating how many points there are and adds headings to each point to make the structure easier to understand.

As a side note, in this weekly report, we sometimes intentionally write important parts at the end. This is because we value "thinking about it" more than "quick understanding" ;) 

---

In a nutshell

In review comments, write suggestions or requests first, and then supplement with reasons.

Keywords: code review, review comment, explanation order

Read other articles on techniques for improving code quality

List of articles on techniques for improving code quality