Raising code quality for Faire’s Kotlin codebase

At Faire, we strive to keep our code simple and readable and have developed an extensive style guide with rules designed to prevent bugs as well as maintain a readable codebase. We’re regularly launching new features to serve our customer community, as well as making improvements to existing ones, so having maintainable code enables us to iterate and deliver value to customers faster. By actively seeking to maintain a codebase with a degree of consistency and maintainability, we operate with a level of agility that enables us to work more efficiently to better serve the brands and retailers on Faire’s platform.

With that in mind, imagine your style guide includes the following rule: in a test, if you’re asserting on an empty list, you should call the function isEmpty rather than directly asserting that the length of the list is zero. It’s late in the day, and you’ve been tasked with reviewing a large pull request. You want to be as thorough as you can in upholding the style guide, but when faced with the following code, your eyes glaze over the violation.

Missing something like this can be understandable, as reviewers should be focused on architectural concerns not syntactic rules. Instead of relying on human reviewers, syntax issues should be regulated by static code analysis tools like Detekt that enforce coding standards in Kotlin. With tools like this, remembering the nuances of the style guide becomes a task for machines rather than an additional cognitive load on human reviewers.

Using Detekt, we automatically catch this violation when attempting to push changes and instruct the developer on what needs to be changed before the submission can succeed. The developer then proceeds to refactor the code to the following:

This is how we leverage Detekt at Faire to enforce rules like the “Use isEmpty instead of hasSize to assert empty lists” rule described earlier. Specifically, we use Detekt to look at dot qualified expressions and determine whether those dot qualified expressions have a receiver which is an “assertThat” expression, and then check if the callee is an “hasSize(0)” call. This looks like the following:

Beyond style issues, Detekt can also help us prevent bugs. For example, at Faire, we use a framework written in Java. However, since we use Kotlin, our API endpoints are written in Kotlin, which means we have to be mindful of issues regarding interoperability between Kotlin and Java. As an example, let’s look at the following code:

From reading this code, one might assume that if a POST request is made to this endpoint without the countryCode parameter, it will default to USD; however, this is not the case as we use Jersey for our endpoints and since Jersey is a Java library, it will not use default arguments supplied by Kotlin code. To avoid this bug, we have a Detekt rule that ensures that default arguments are not present in API endpoint definitions. With this Detekt rule, the above code snippet would change to:

We also use Detekt to enable observability. Imagine the following: you’re running an engineering team at Faire dedicated to enabling retailers (buyers on our marketplace) to search our vast collection of products. As part of your mandate, you’re required to ensure that the various search endpoints on Faire’s backend monolith are healthy. Ideally, you would prefer to do this via a Datadog dashboard that you could leverage to easily view the relevant data. You could try to do this manually, but then you would have to add new endpoints to the dashboard as your engineers create them, which is a process vulnerable to human error.

We solved this problem by requiring all entry points into our system to be annotated within our codebase with the product area that it belongs to, as follows:

In the case where someone doesn’t annotate an entry point with a product area, Detekt will record a violation on that PR which will prevent the PR from being merged into our main branch.

Each product area is associated with a team which enables us to direct issues with these API endpoints to the appropriate team via our Datadog and Pagerduty integrations thereby, ensuring that as issues occur, we are able to determine who should take ownership.

There are other complex issues to catch as well. For example, we have a rule mandating that all code be reachable. To illustrate what that entails, let’s consider the following example:

Luckily, Detekt can handle scenarios like this by integrating with the Kotlin compiler through Detekt’s type resolution feature. We could run this on commit but using this feature makes validating code with Detekt take an extra 2 minutes because Detekt’s type resolution involves compiling code beforehand to obtain type information and then run the Detekt rules with that type information. Instead, we chose the alternative of running this on CI. This option includes the disadvantage of late feedback, which we’ve tolerated as an acceptable trade-off since we favor fast-running rules that don’t use the full benefits of the Kotlin compiler on commit.

That said, we built out custom rules that employ type resolution such that we have heuristics that do not require type resolution so that we can run these heuristics within our git post-commit hook. For example, one of our custom rules mandates explicit return types on expression body functions, which generally requires type resolution to infer the type employed. Still, many functions employ literals, and we can use the presence of these literals as a heuristic to tell engineers adding new expression body functions to specify the return type. As an example:

Since the expression body of isEnabled evaluates to a boolean value, it would be caught by the heuristic requiring an explicit return type. This same dynamic holds for String literals, numeric literals, and basic arithmetic or logical expressions.

Another tool we employ to optimize our usage of Detekt is auto-correction. We use this tool to reduce churn for our developers by ensuring that when our Detekt rules discover violations, that same rule automatically fixes the violation. For example, we have a Detekt rule which mandates that when writing tests, our engineers must write “assertThat(someValue).isTrue()” instead of “assertThat(someValue).isEqualTo(true)” when asserting whether or not a boolean evaluates to true. When an engineer writes one of these invalid assertions, instead of having to fix it themselves, our Detekt rule fixes it for them by modifying the AST:

I’ve outlined the many advantages of embedding Detekt into your organization’s development workflow. Once you’ve taken steps to develop a set of custom rules to automate elements of your organization’s style guide, what’s next? How do you continue to accommodate changes to your organization’s style guide as you scale? At Faire, we have a dedicated internal group in our #detektors Slack channel committed to completing 1 rule every month. This group meets monthly to discuss the progress of the automation of our style guide and to improve our handling of automating code standards in general. This group is also committed to ensuring that our style guide is enforced through a fully automated system, making our development process easier and more consistent. Furthermore, all engineers at Faire are empowered to submit new ideas to the group and are encouraged to actively participate as membership in #detektors does not require traversing byzantine barriers.

At Faire, there are 3 million lines of code in our Kotlin codebase, and we seek to enforce consistent rules across that codebase, a feat that would not be achievable without static analysis tools like Detekt. Additionally, we employ Detekt to encourage code patterns that are relevant to company objectives like observability and reliability. To continue improving both the efficiency and scalability of the program, we maintain our commitment to Detekt through a decentralized group within the company that builds out additional rules as necessary.

Faire’s Engineering team is growing–join us! www.faire.com/careers