Detekt is a static code analyzer for Kotlin. This post talks about how I used Detekt for non-trivial, systematic improvements to code quality, using custom Detekt rules.
Source files can be found here: https://github.com/ygaller/detekt-antipatterns
Antipatterns, antipatterns everywhere
I review a lot of pull requests on a regular basis. During reviews, antipatterns show up all the time. Many of them are repetitive and addressing them is important, but does not involve deep insight into the code. In fact, repetitive nitpicking in code reviews is an antipattern in itself. Repeating the same points over and over is exasperating, and they keep popping up in PRs coming from different developers.
Sometimes you see an antipattern that is so bad that it sends you on a gardening task to weed out all instances of it in the codebase. One such example is the instantiation of Jackson’s ObjectMapper.
Antipattern example – misusing ObjectMapper
Instantiating an ObjectMapper is intensive as far as object allocation and memory use. If you’re not careful and causally use it in code that is called frequently, it will cause memory churn – the intensive allocation and release of objects, leading to frequent garbage collection. This can directly contribute to the instability of a JVM process.
Let’s have a look at an example of bad usage of the ObjectMapper: We have a CustomerRecord class which represents a DB record. This record has a json string field. When we fetch the record we want to convert the record into a DTO. This requires us to deserialize the string so that we can access the serialized object.
data class CustomerRecord(
val firstName: String,
val lastName: String,
val emailAddress: String,
val preferences: String // json
) {
fun toDto(): CustomerDto {
val preferences = ObjectMapper().registerModule(KotlinModule()).readValue<CustomerPreferences>(preferences)
return CustomerDto(
firstName,
lastName,
emailAddress,
preferences.language,
preferences.currency
)
}
}
The problem here is that with each invocation of toDto(), we are creating a new instance of ObjectMapper.
How bad is it? Let’s create a test and compare performance. For comparison, version #2 of this code has a statically instantiated ObjectMapper:
data class CustomerRecord(
val firstName: String,
val lastName: String,
val emailAddress: String,
val preferences: String // json
) {
companion object {
private val objectMapper = ObjectMapper()
.registerModule(KotlinModule()) // To enable no default constructor
}
fun toDtoStatic(): CustomerDto {
val preferences = objectMapper.readValue<CustomerPreferences>(preferences)
return CustomerDto(
firstName,
lastName,
emailAddress,
preferences.language,
preferences.currency
)
}
}
Let’s compare performance:
| Iterations | New Duration (Deserializations / s) | Static Duration (Deserializations / s) | Ratio |
| 10K | 767ms (13,037) | 44ms (227,272) | x17 |
| 100K | 4675ms (21,390) | 310ms (322,580) | x15 |
| 1M | 37739ms (26,504) | 1556ms (642,673) | x24 |
Static – creating ObjectMapper() once
This is an amazing result! Extracting the ObjectMapper to a static field and preventing an instance from being created each time has improved performance by a factor of over 20 in some cases. This is the ratio of time that initializing an ObjectMapper takes vs. what the actual derserialization takes.
The problem with the one-time gardener
A large codebase could have hundreds of cases of bad ObjectMapper usages. Even worse, a month after cleaning up the codebase and getting rid of all these cases, some new engineers will join the group. They are not familiar with the best practice of static ObjectMappers and inadvertently reintroduce this antipattern into the code.
The solution here is to proactively prevent engineers from committing such code. It should not be up to me, the reviewer to catch such errors. After all, I should be focusing on deeper insights into the code, not on being a human linter.
Detekt to our aid
To catch these antipatterns in Kotlin, I used a framework called Detekt. Detekt is a static code analyzer for Kotlin. It provides many code smell rules out of the box. A lesser known feature is that it can be extended with custom rules.
Let’s first try and figure out what would valid uses of ObjectMapper look like. An ObjectMapper would be valid if instantiated:
- Statically, in a companion class
- In a top level object which is a singleton itself
- In a private variable in the top-level of a file, outside of a class but not accessible from outside the file
What would an ObjectMapper code smell look like? When it’s instantiated:
- As a member of a class that may be instantiated many times
- As a member of an object that is not a companion object within a class (highly unlikely but still…)
I created a rule to detect the bad usages and permit the good ones. In order to do this, I extended Detekt’s Rule abstract class.
Detekt uses the visitor pattern, exposing a semantic tree of the code via dozens of functions that can be overridden in Rule. Each function is invoked for a different component in the code. At first glance, it’s hard to understand which is the correct method to override. It took quite a bit of trial and error to figure out that the correct function to override is visitCallExpression. This function is invoked for all expressions in the code, including the instantiation of ObjectMapper.
An example of how we examine the expression can be found in class JsonMapperAntipatternRule. We search the expression’s parents for the first class & object instances. Depending on what we find, we examine the expression in the context of a class or of a file.
val objectIndex = expression.parents.indexOfFirst { it is KtObjectDeclaration }
val classIndex = expression.parents.indexOfFirst { it is KtClass }
val inFileScope = objectIndex == -1 && classIndex == -1
if (inFileScope) {
validateFileScope(expression)
} else {
validateObjectScope(objectIndex, classIndex, expression)
}
Test coverage can be found in JsonMapperAntipatternRuleTest. Tests include positive and negative examples and can be added as we encounter more edge cases that we may not have thought of.
The positive test cases we want to allow:
// Private ObjectMapper in class' companion object
class TestedUnit {
companion object {
private val objectMapper = ObjectMapper()
}
}
// ObjectMapper in object
object TestedObject {
private val objectMapper = ObjectMapper()
}
// Private ObjectMapper in file (top level)
private val objectMapper = ObjectMapper()
class TestedObject {
}
// Part of a more complex expression
private val someObject = ObjectMapper().readValue("{}")
class TestedObject {
}
On the other hand, cases which we want to prevent:
// ObjectMapper not in companion class
class TestedUnit {
private val objectMapper = ObjectMapper()
}
// Within a non-companion object
class External {
object Internal {
val notGood = ObjectMapper()
}
}
// Not private in top level of file
val notGood = ObjectMapper()
class External {
}
The detekt-custom-rules module is a minimal Detekt plugin module. Once built, the result is a jar that can be used interactively within IntelliJ, or as a plugin when building the rest of the project.
Using the plugin with IntelliJ
After installing the Detekt plugin in IntelliJ, we enable it and point it to the plugin jar like so:
The result is that we now have syntax highlighting for this issue:
Using the plugin with a Maven build
To use is as part of the build, we need to point the maven plugin to the jar:
<plugin>
<groupId>com.github.ozsie</groupId>
<artifactId>detekt-maven-plugin</artifactId>
<version>${detekt.version}</version>
<executions>
<execution>
<id>detekt-check</id>
<phase>verify</phase>
<goals>
<goal>check</goal>
</goals>
</execution>
</executions>
<configuration>
<config>config/detekt.yml</config>
<plugins>
<plugin>detekt-custom-rules/target/detekt-custom-rules-1.0-SNAPSHOT.jar</plugin>
</plugins>
</configuration>
</plugin>
With the plugin in place we receive a warning during the build:
custom-rules - 5min debt
JsonMapperAntipatternRule - [preferences] at detekt-antipatterns/customers/src/main/java/com/ygaller/detekt/CustomerRecord.kt:18:23
In detekt.yml it’s possible to configure the build to either fail or just warn on the custom rule.
Conclusion
Using Detekt and the custom rule, I managed to screen for a non-trivial antipattern. Adding it to IntelliJ and to the build means that it is much less likely to pop up in future code reviews.
Moreover, it decenteralizes the knowledge of this issue and enables any engineer working on our code in the future to be alerted to the fact that he is introducing an inefficiency into the system. One that can easily be avoided.