Code reviews: How to make it work for you

GoHealth builds its own proprietary technology to support the health insurance enrollment process.

As a team, we operate business-critical software systems. Outages clearly have the potential to adversely impact financial results of any company. Hence, we put the utmost care into the quality, reliability, resilience, and maintainability of our systems. Utmost care is put into quality and maintainability. The same reasons dictate the need to invest significantly in proper architectural design of solutions for new functionalities as well as activities to assure software quality.

Peer reviews

The review process does not differ from the mainstream approach: each change is reviewed by at least 2 colleagues in the team and is merged by a technical lead into the main branch of a given repository.

In general, we do not use feature branches, we converge around just a single main branch. This strategy keeps the artifacts with contributions of different teams tested together and reliably deployed to production. To assure reliable releases of new features, we use feature flags to switch on / off new features, which also facilitates independent testing when such a need arises.

Reviews are done before the contribution is merged into the main branch. Code review focuses on:

• Consistency
• Simplicity
• Usage of established coding style (streams, reactive asynchronous processing, functions)
• Readability: Are the implementation and underlying algorithm understandable without deep study? Explanatory names of variables, methods, classes that “tell the story”. Plain text explanation in code doc as necessary
• Strongly typed value objects

Peer reviews help new joiners to understand established patterns and solutions in code, and thus facilitate faster adaptation. Naturally, review comments may trigger discussion as the code review may reveal additional concerns, scope, or issues with the design. In such cases, we tend to move the discussion to a peer to peer or group conversation — whichever is faster and more productive.

Finally, we use peer reviews (in lack of time without notes) to keep the developers in different team(s) informed about ongoing changes in different subsystems to keep the mental model in their minds updated as soon as such change occurs (while documentation for further adoption follows).

©GoHealth

What is a good code?

Writing software is both an intellectual and creative activity. We would spend significant time thinking about the algorithm, the technology, the way of structuring and writing the code. The wave of agile extreme programming was in many situations misunderstood by replacing the thinking part with just fast writing.

Writing code is like building an airplane. Despite the power of the engine(s) — weight matters. Code is not for free, each word in code, each abstraction has its price. The value of the code must be higher than the price of the code.

Zero value code

With dead, unused code, the strategy is straightforward: Identify and actively remove unused code. Feature flags are a good demarcation of unused old code. After the new code is proven to work in production, the old code may be deleted.

Maybe value in the future

For code that would eventually be used in the future, a similar simple straightforward principle applies: Write code just in time — when required for the requested functionality. New code should be written in minimal necessary size to offer the required functionality. No code should be written for eventual features in the far future. Code is designed with extensibility in mind, so any new requirement is easily implemented.

Of course, we rejuvenate the technology ecosystem together with the implementation of new functionalities and replace deprecated technologies; we still minimize the application domain code to the minimum necessary size.

Small (and simple), is beautiful

One “mechanical” metric, questioned by generations and used as long as programs are written is the number of lines of code. To have a more objective view we can exclude comments (the mandatory copyright on the very beginning of each file) and count words instead of lines. Many times a long line of stream cascade with dot-separated “train” is much simpler to read if it is written into multiple lines, but the number of words stays the same.

Simplification

Complexity is a less obvious theme. Oftentimes, this topic triggers very emotional and opinionated discussions, clash of cultures and experiences.

Method code complexity

Traditional measurement is cyclomatic complexity — the number of different paths through the code in a method or function.

Object-oriented code

Object-oriented abstractions increase mental complexity by a level of magnitude. A simpler solution is preferable: functional constructs and naive value objects.

Interfaces

The complexity that exceeds one file is the implementation of the unnecessary interface(s). Traditional OO design, and also the abstraction paradigm brought by Spring framework suggests to “automatically” create interfaces for each active object/bean (object with functionality). This practice simplifies writing unit tests, but in many cases, the interface — implementation separation is never used — so the interface as an unnecessary abstraction may be deleted. Java has a good object-oriented foundation. So, of course, there are valid use scenarios for interfaces — i.e. interface in separate modules that simplify the construction of the application from multiple modules, and the dependency management between modules. Functional way of writing code — use of functional interfaces defines the service contract. Functional interfaces bring the principle of single responsibility — one reason to change class to extreme and so simplifies the code. The single responsibility of the service interface (“service layer”) is to dispatch to an operation function and to hold the transaction annotation.

Inheritance

Inheritance makes it hard to find the code that is executed — maybe it is in parent hierarchy, maybe overwritten. In Java, it is prohibited to make multi-parent inheritance. Deep inheritance (more than 2 levels) makes the code “magical”, full understanding requires browsing over many files and returning back. By constructing complex classes we prefer composition over inheritance. Simple classes without interfaces and inheritance localize the whole domain logic into one file, and utility and data provider classes hide their complexity as black boxes. Inheritance has its place in a well-designed model, or in JPA though for shared definitions in symmetric/derived models.

Generics

Generics are a powerful construct for expressing universal algorithms. However, if used in application domain logic, its usage can abruptly lead to unnecessary complexity. The combination of generics with inheritance is a very attractive secret sauce for writing very complicated classes, very hard to understand.

Design patterns

It is a fundamental programming skill to know all the basic design patterns. In some cases, usage of design patterns is a symptom of overengineering and may build code that is hard to extend. An example of an unnecessarily increasing complexity is the visitor pattern. In many cases, the functionality may be implemented with a naive direct solution — simple but not simplistic.
Of course, it is not a strict rule, design patterns are a good tool, but have to be used with caution in the right place.

©GoHealth

Code for people

Good code is written for readers, not for compilers. Good code “tells a story”, explains the algorithm, the way of data processing. The readability is achieved by well-chosen descriptive names for classes, methods, method parameters, and variables.

It is helpful to split complex boolean logic and complex calculations into local variables, hence “name” the sub-values, explain the calculations. The names are a good way to document the code and help the flow of reading, that is not distracted by comments.

JavaDoc is the traditional promise to provide code as documentation. If JavaDoc is mandatory, required by build configuration settings, I prefer to generate JavaDoc comments with a sophisticated IDE plugin. The plugin tries to interpret the name of the class or method, the name of the returned class, and the parameters to generate a meaningful description. On one side it is good proof that the names are well-shaped also for human readers, on the other side the comment is good enough for elementary cases and is a good frame for additional description of the algorithm or method functionality.

We prefer long descriptive class (function) names. Long enough to be unambiguous and describe the single responsibility, but short enough that there are redundant or duplicate words. A hard balance, but worth the effort, esp. given people don’t like writing good docs.

How much to invest in a code review?

So the answer is simple and should be driven by the reason we do code reviews: to find bugs as close to their introduction as possible. This does mean that the team needs to understand the impact of a bug in a particular system though, which is often missing context.

Code reviews are not for free. The expectation is not to find everything that could be written better. We involve more developers in each review — more eyes notice more, each programmer has a different experience, a different perspective, and focus. The severity of found problems exponentially shrinks with time. And we have to find the optimal time invested in a review that gives valuable findings.

Written by Tomi Vanek

Follow us on our web and social media!
GoHealth.sk | Facebook | Instagram | LinkedIn | YouTube