At Ghyston, we care deeply about using technology to help businesses succeed in whatever they’re trying to do. For us, a key part of this is ensuring that the code running our client’s businesses is not just doing the right thing but is also maintainable for the long term. Your software isn’t a static thing that is built once and then runs forever, it’s constantly evolving and changing to keep up with the needs of your business. This means that code will be read much more often than it is written, so it’s important that it should be as easy to understand and change as possible.
Developing an understanding of what makes good, maintainable code is one of the first skills we teach new employees at Ghyston, and a key part of that skill is knowing what bad code is. “Code smells” is a term popularised by Kent Beck and Martin Fowler. The idea is that if you notice one of these features, it isn’t necessarily indicative that there’s a bug, but that there’s a likelihood that there may be deeper issues in the code and that some refactoring is needed. This could be in your own code, reviewing someone else’s code, or when taking on maintenance of an existing codebase created by someone else.
Below we’ve listed some of the common code smells we see, how they can come about, and what to do about them:
1. Long methods
If you have a single method or function within your code that does a million and one things, it can be difficult to understand exactly what is going on, as there's so much context. As an example, say you have some Typescript code that processes an order for an e-commerce site. The method might look something like this:
const processOrder = (items: Item[], userId: number, userAddress: Address, userIsLoyaltyMember: boolean, userIsOnCreditHold: boolean): void => {
if (userIsOnCreditHold) {
sendCreditHoldNotification(userId)
throw new Error(`Can't process order for a user that is on credit hold!`)
}
// Calculate total price
let total_price = 0;
for (let item of items) {
total_price += item.price;
}
// Apply discount for loyalty members
if (userIsLoyaltyMember) {
total_price *= (1 - 0.05);
}
// Handle shipping
if (userAddress.isInternational) {
total_price += 15;
} else {
total_price += 5;
}
// Check stock
for (let item of items) {
if (getStock(item) <= 0) {
throw new Error(`Item ${item.name} is out of stock!`);
}
}
// Finalize order
addChargeToUser(userId, totalPrice);
for (let item of items) {
shipToUser(item, userId, userAddress);
}
}
While it's relatively straightforward to see what's going on in this example, you can easily see how this could get more out of hand - calculating shipping costs could be a lot more complicated based on item weights, other types of discounts could be introduced, etc. A more maintainable version might extract out a few named methods which then makes the function easier to read, like this:
const processOrder = (items: Item[], userId: number, userAddress: Address, userIsLoyaltyMember: boolean, userIsOnCreditHold: boolean): void => {
checkUserIsNotOnCreditHold(userId, userIsOnCreditHold)
const totalPrice = calculateTotalPrice(items, userId, userIsLoyaltyMember, userAddress);
checkStock(items);
finalizeOrder(totalPrice, items, userId, userIsLoyaltyMember, userAddress);
}
We've now got some nice, well-encapsulated code, with the shipping and discount logic contained in the calculateTotalPrice function, and it makes it easy for future readers of this code to come along and understand what's going on.
2. Magic strings/numbers
Magic strings and magic numbers refer to codes or strings in the code that are hard-coded in the application and aren't named. In the previous example, we had some code for calculating price which had magic numbers in it:
if (userIsLoyaltyMember) {
total_price *= (1 - 0.05);
}
if (userAddress.isInternational) {
total_price += 15;
} else {
total_price += 5;
}
In the above code, 0.05, 15 and 5 are what we'd call magic numbers. If we wanted to update the loyalty discount or shipping costs in the future, we couldn't be confident that we've caught every place these numbers are used without searching for every instance of these numbers, and checking when updating that the number is actually being used for the same reason.
To improve this code, we can name these constants at the top of the file so that we understand what they're being used for:
const LOYALTY_DISCOUNT = 0.05;
const INTERNATIONAL_SHIPPING_PRICE = 15;
const DOMESTIC_SHIPPING_PRICE = 5;
// ...
if (userIsLoyaltyMember) {
total_price *= (1 - LOYALTY_DISCOUNT);
}
if (userAddress.isInternational) {
total_price += INTERNATIONAL_SHIPPING_PRICE;
} else {
total_price += DOMESTIC_SHIPPING_PRICE;
}
Here, it's nice and obvious what's going on, and allows us to potentially pull the loyalty discount or shipping prices from configuration or the database in the future without having to make so many modifications to the codebase.
3. Long parameter lists
When modifying existing code, it's often the case that the data we need to pass to the function will change slowly over time, with more and more information being added. In these cases, adding an extra parameter is often the easiest fix in the moment, but it can result in code that's difficult to read and use later on. In our example, there's quite a few parameters that are concerned with the user:
const processOrder = (items: Item[], userId: number, userAddress: Address, userIsLoyaltyMember: boolean, userIsOnCreditHold: boolean): void => {
// ...
}
Here we have a userId, a userAddress, as well as booleans for userIsLoyaltyMember and userIsOnCreditHold. At present, it's just 4 parameters, but it's easy to see how without some refactoring, we could easily have several more parameters sneak in, making the code more difficult to read and harder for external callers to use.
In this case, we'd likely want to refactor the code to use a small object, like this:
interface User {
userId: number;
userAddress: Address;
isLoyaltyMember: boolean;
isOnCreditHold: boolean
}
const processOrder = (items: Item[], user: User): void => {
checkUserIsNotOnCreditHold(user);
const totalPrice = calculateTotalPrice(items, user);
checkStock(items);
finalizeOrder(totalPrice, items, user);
}
Now we've got a nice User object that we can pass around in our code, being confident that it has the right level of information, and that there's only one place (the User interface definition) that we need to change if we add a new feature to the user profile in the future.
