Server Side JS Patterns to Avoid
Every organization has legacy code that was assembled a certain way for a certain reason but is now hard to read.
From time to time I look at some piece of code in our codebase and think “how could this have been done differently?”. At Hippo, we’ve since made the decision to optimize our code for readability vs. write-ability. We want those reading the code to be able to follow what’s happening, click to references easily in their IDE, and see the entire stack of function calls in a stack trace. In some parts of our legacy code, we attempted to optimize by re-using code and writing common functions. This optimization has often made the code harder to read.
This post highlights a few patterns to avoid, lists some reasons why to avoid them, and provides alternatives to the current code structure.
Renaming Constants and Losing Context
This is the simplest item in this post but the small amount of time it saves over hundreds or thousands of views adds up. Some of our code contains lines that will import or rename a constant to a regular variable.
Renaming the constant to a variable name requires the reader to jump to one more reference to figure out where the value came from:
const val = constModule.MY_CONST
or
const cookieNameFlags = require('../../constants').COOKIE_NAME_FLAGS;
The constant is more readable in its original ALL_CAPS format. If we want to use a constant with its original ALL_CAPS format, we do:
const { MY_CONST } = require('./const')
or:
const constModule = require('...');
if (myVal === constModule.MY_CONST) {
...
};Overusing the Plugin Pattern with Anonymous Functions
The “plugin pattern” is a pattern when you want some common code to execute around a unique function. The Java sort comparator is probably the simplest example of a plugin pattern implementation that I’ve found. A comparator is defined:
class AgeComparator implements Comparator<Person> {
@Override
public int compare(Person a, Person b) {
return a.age < b.age ? -1 : a.age == b.age ? 0 : 1;
}
}Then the sort is executed by plugging that comparator into the sort function:
Collections.sort(people, new AgeComparator());
In a simple case like a sort comparator, the plugin pattern can be obvious and useful. The following example of using the plugin pattern in Javascript attempts to be elegant by wrapping a logic function inside another function that’s meant to retrieve a lead. Instead of being elegant, it’s difficult to follow, read, and debug:
app.get('/lead/:leadId',
getProducerAndOrganization(),
getCookieMetadata,
getLead(leadRoutes.getLead));app.get('/lead/:leadId/history', getLead(leadRoutes.lossHistory));
The code attempts to wrap a controller-level business logic function in a getLead function. In the first case, it also specifies two functions to execute (Express executes middleware functions in order) before the business logic - one generates a function and one is a function. With so many patterns mixed together and the plugin of drastically different business logic functions, this becomes a nightmare to read. This is especially true when we’re already using Express middleware functions, which are essentially a plugin.
The contained function becomes anonymous and the stack trace is broken at runtime. This makes it hard to trace a problem to the lossHistory function, instead ending the stack trace at getLead
An easy-to-read alternative would be:
const lossHistory = async function(req, res, next) {
const lead = getLead(req, res);
// Do some logic.
return history;
}Here the code can be traced and the stack trace will show both the lossHistory and getLead functions if execution is stopped in either.
Unnecessary Promise Chains
When Javascript introduced Promises it was a good way to manage async sequences. That has been replaced mostly with async/await which is easier to read. Instead of chaining promises together, it’s better to use async/await to wait for a response.
Instead of:
.then(() => {
return doSomethingAsync();
}).then(() => {
return doSomethingElseAsync();
})Do this:
await doSomethingAsync();
await doSomethingElseAsync();Which is easier to follow.
If there is a long block of code in one of the then blocks, sometimes it’s hard to pick out which function is the async function returning a promise. This is especially true if there are multiple functions called or there are multiple return statements due to conditionals.
Mixing Express Routes and Business Logic
Express is a popular NodeJS MVC framework. Applications built with express generally have 3 layers: routes, controllers, and models. Routes handle all of the marshaling HTTP requests and responses from the business logic embedded in the controller layer. When route and controller logic are mixed, it becomes hard to unit test business logic and hard to trace side effects when the reader of the code is looking at two layers instead of one.
Take this block of code:
if (request.producer) {
await verifyProducerAndOrgChainLicensesForState(
request.producer, request.organization, lead.state);
}
if (lead.isBound()) {
const leadUxData = await getBoundLeadData(lead);
return response.status(responseCodes.SUCCESS).send(leadUxData);
}
const tpi =
await lead.getTransactionData().enrichPolicyDataForLead();
const result = getUnboundLeadData(
{ tpi, lead, fullQuote: false, includeWorkflow: true }
);updateLeadFlags(lead, request);
updateLeadCookies(lead, request);
result.leadMetadataFlags = _.get(
lead, 'data.metadata.tracking.flags');response.status(responseCodes.SUCCESS).send(result);
return lead.saveAll();
It contains the following places where one could argue that “business logic” is being executed:
- Verifying the insurance agent license
- Checking if the lead is bound (policy is sold) and sending a response if it is
- Getting additional transaction data on the lead to send back to the client
- Updating flags and cookies related to the client on the lead object
The first item is business logic that should be handled in a controller layer. The second item seems like logic that belongs in the route, except for the fact that getBoundLeadData is implemented at the route layer and not the controller layer. The third item executes a database read on the model itself, skipping the controller layer. The fourth item is mutating the lead on an HTTP GET operation, which could be an unexpected side effect AND cause a race condition (see Race Conditions in a Distributed System ).
Here’s some pseudocode that would refactor this to a more clean and testable set of interfaces:
if (req.producer) {
await controller.verifyProducerAndOrgChainLicensesForState(
req.producer, req.organization, lead.state);
}
const leadUxData = controller.getBoundLead(lead);
if (leadUxData !== null) {
return res.status(responseCodes.SUCCESS).send(leadUxData);
}const unboundLeadData = controller.getUnboundLeadData(
{lead: lead, fullQuote: false, includeWorkflow: true});
controller.updateLeadFlagsAndCookies(lead, req);
unboundLeadData.leadMetadataFlags = _.get(
lead, 'data.metadata.tracking.flags');
res.status(responseCodes.SUCCESS).send(unboundLeadData);
This update results in an easier-to-read route and all of the business logic pushed down to the controller. There’s possible further improvement in not sending the request to the controller in updateLeadFlagsAndCookies and possibly not relying on leadUxData to be null if not found. The separation of concerns between layers also makes it easier to write unit tests to cover most of the code in this route.
Code Consistency: Factoring Middleware Out
Keeping code organized and consistent for readability can be just as important as writing it well. Take this sequence of code:
const leadRoutes = {
successHandler: function (req, res, next) {
res.render = function (data) {
res.status(200).json({success: 1, data: data});
};
next();
},
}
cloneextend.extend(leadRoutes, require('./lead'));
app.use(leadRoutes.successHandler);A new object is defined with a function that has an Express middleware signature. This object is extended with an import from a file in the same directory and the middleware is then used in the app (also an Express concept).
Our code has a middleware directory. This directory contains other middleware implementations. Some of those are imported into the file in the require statement. Other functions extended onto leadRoutes are not middleware.
The lack of consistency, import in the middle of the file (which could have easily been imported at the beginning of the file), and the combinations of functions and middleware functions in the same import make this code hard to read, hard to follow, and hard to understand.
Instead, this would be simpler, keep directory structure consistency, and more explicitly self-documents what functions are being imported and executed:
{ leadSuccessHander } = require('../middleware/lead');
{ leadHelper } = require('./lead');
if (condition === true) {
leadHelper(leadData);
}app.use(leadSuccessHandler);
Moving Forward
As we move our monolith to smaller services, we still spot places in our legacy code base that could be improved. This article attempts to point a few of those out.
If you’d like to learn more about working in engineering at Hippo, go to https://www.hippo.com/careers
Engineering Blog, Episode 5
