As engineers, much of the quality of our days and lives depends upon the quality of the codebases on which we work.
Do you live and work with intent? Do you know what your values are? Code reviews may seem like an odd place to ask these questions, but as engineers, much of the quality of our days and lives depends upon the quality of the codebases on which we work.
Imagine a construction worker building the 40th floor of a building whose 12th floor wasn’t properly designed! This sounds like madness, but is often part of the reality of most software engineers’ lives. We deal with systems that are undocumented, that are unreliable, that are slow, or inflexible, or even incorrect.
At MuleSoft, we’ve been building massive distributed systems for over a decade, and we’ve been so successful that these systems now play a significant part in the global economy. We hold a very high bar for releasing code and have, over the past years, developed playbooks for how to approach coding with a high degree of intent, and with a clear set of values: Concrete, actionable ways to write the cleanest, most maintainable, least brittle code, so that the engineers building the world’s application networks are building them on top of a rock solid foundation.
Code is fundamentally a set of data transformations
Let’s start with how we think about what code is: When you approach code as nothing more than a set of data transformations, you suddenly find a number of things that seem hard become very clear. Let’s explain what we mean by this statement. A web browser transforms HTML markup into a website rendered on-screen. A video game transforms a set of textures and geometries into pixels, and a compiler transforms parsed code into a lower-level representation. Mule itself can transform anything. Why is this a useful way to think about things? Because one of our core principles is single responsibility.
SRP makes it easy to test, document, and evolve code
The single responsibility principle (SRP) in this context states that a code module transforms code from only one domain to only one domain. This is a more useful way to think about it than the classic definition of “functionality” or “reason to change”. A classic example of this in the API world is RESTful API controllers: These modules are the ones that accept an HTTP request and figure out what to do with it. We define them as transforming an HTTP request into a method call. That defines a very clear boundary: Controllers do not make database calls, they do not compute values, they do not make network requests. They simply make a call to another module. This single responsibility makes them easy to test, document, and evolve. Consider the task of documenting a module, something not often done and less often done well. We simply document, for each module, what domain it accepts (HTTP requests) and what domain it targets (method calls to our core business logic.) This creates and requires both human and architectural clarity.
Every module is part of a layer in the system
Moving on, each module is seen as part of a clear layer in our system. The controllers are part of the HTTP server layer. Business logic sits in the business layer, database access is in the data layer, and so on. We build our software in layers and ensure that dependencies only ever point to the next layer down: Any call from a lower layer to a higher immediately sets off PR alerts and triggers a refactoring discussion. Doing all this ensures that we are able to rapidly reuse and add layers. For example, when we need to add a gRPC layer to a system, we are still able to reuse all of the business layers. We simply write new transformations from gRPC invocations to business layer method calls.
Within each layer and modules, we talk often about having a coherent and consistent concept model. This requires empathy and intent: What is it that you are really doing here? What are the components of the system that you are assembling? What are the terms for them?
In many code reviews, we find inconsistencies in how code talks about things: One moment the term “prefix” is used, another, “prepend.” Sometimes an object is called an “object”, sometimes an “entity”, at other times an “item.” When you think about your code as a real product with real users, who will be reading your code, and think about how they will feel, you start being more disciplined. We recognize that humans, even engineers, are not necessarily good at this sort of consistency; we’re fluid thinkers who live in a world that is not black and white, but shades of gray. This is why we strongly recommend the self-PR-review as a best practice: Reviewing your own PR before sending it out will often catch many things that will save reviewers and yourself time.
In the spirit of communication, our favorite rule is so important and useful that we speak of it in uppercase: DO NOT NEST. Consider the following code:
if (user && user.session != null && (match.params.redirectUrl || sessionStorage.get('redirectUrl')) {
window.location.assign(match.params.redirectUrl || sessionStorage.get('redirectUrl'));
}
Not only is this code hard to read, it also contains duplicated logic, and communicates no intent. Consider now what happens when we unnest the code:
const isLoggedIn = user && user.session;
const redirectUrl = match.params.redirectUrl || sessionStorage.get('redirectUrl')
const shouldRedirect = isLoggedIn && !!redirectUrl;
if (shouldRedirect) {
window.location.assign(redirectUrl);
}
By unnesting the code and expressions, we suddenly find that the code becomes much clearer, cleaner, and is now amenable to applying a concept model!
A small but critical point that we should add at this point is the rule to never copy and paste. We have found over time that any copy and paste of code, even a single line, usually either results in a bug, or in some amount of technical debt for which we would be made to pay dearly in the future. And so we keep our eyes out for duplications and treat them as code smells. This rule counts double in test suites, where the opportunities to copy and paste, and the temptations to do so, are very strong.
Denormalization is a last resort
Related to copying code at design time is duplicating state at runtime: Denormalization is a last resort. There should, ideally, be a source of truth for any state in your system, and putting that state in multiple places is also likely to result in errors. Denormalization can be the result of clear intent: Caching is a classic example. But, it can also be the result of a lack of clear intent, leading to architectural smells which are a precursor to often mysterious production bugs.
We have many more guidelines in place for code – and for testing! The same rules above apply to tests. Our golden rule: Hold test code to as high a bar as production code, because test failure can result in blocked development, and lack of test coverage can result in production issues.
We have thought a lot not just about testing, but also documentation. Documentation is incredibly hard: Clear communication and high speed about code that may change, with the purpose of enabling others to understand your intent and modify your work. But, it’s also almost as important as tests, which should also function as high-quality documentation. For documentation, we look for aggressive linking: Documentation should contain links to bug tracking tickets, to relevant Stack Overflow threads, GitHub links, and any other relevant sources of information. We find these links incredibly useful for building context around how a piece of code got into the state in which we find it. We also talk often around documenting policy, not behavior. For example:
// Add 3 to the price
if (product.weight > 100) {
price += 3
}
We’ve all seen examples of this kind of code, in which the comments were done without clear intent, and so do not add any value. Consider this instead:
// For heavy products, we need to increase the price to cover shipping costs
// See the jira ticket for this at https://...
if (product.weight > 100) {
price += 3
}
This documentation provides both a policy explanation and a link to the context for the policy. As an added benefit, we find that such comments rarely become outdated, since policy changes more rarely than code does.
Hopefully, this introduction has provided you with some food for thought. We work continuously to capture our own collected wisdom and expertise in order to raise both the quality of our code and the skill of our engineers and hope you’ve found things here that you can apply to your own projects. Got questions? Thoughts on additional rules and principles? We’d love to hear from you in the comments section below.
Interested in working with us? Check out the open roles on our Careers page.
