One of the classic "code smells" is called Primitive Overuse.
It's deceptively simple.
Note: This is an excerpt from my book Refactoring TypeScript: Keeping Your Code Healthy.
Identification Of Primitive Overuse
Take this code, for example:
const email: string = user.email;
if(email !== null && email !== "") {
// Do something with the email.
}
Notice that we are handling the email's raw data?
Or, consider this:
const firstname = user.firstname || "";
const lastname = user.lastname || "";
const fullName: string = firstname + " " + lastname;
Notice all that extra checking around making sure the user's names are not null? You've seen code like this, no doubt.
What's Wrong Here?
What's wrong with this code? There are a few things to think about:
That logic is not sharable and therefore will be duplicated all over the place
In more complex scenarios, it's hard to see what the underlying business concept represents (which leads to code that's hard to understand)
If there is an underlying business concept, it's implicit, not explicit
Business Concepts By Chance
The business concept in the code example above is something like a user's display name or full name.
However, that concept only exists temporarily in a variable that just happened to be named correctly. Will it be named the same thing in other places? If you have other developers on your team - probably not.
We have code that's potentially hard to grasp from a business perspective, hard to understand in complex scenarios and is not sharable to other places in your application.
How can we deal with this?
Deceptive Booleans
Primitive types should be the building blocks out of which we create more useful business-oriented concepts/abstractions in our code.
This helps each specific business concept to have all of its logic in one place (which means we can share it and reason about it much easier), implement more robust error handling, reduce bugs, etc.
I want to look at the most common cause of primitive overuse that I've experienced. I see it all the time.
Scenario
Imagine we're working on a web application that helps clients to sell their used items online.
We've been asked to add some extra rules around the part of our system that authenticates users.
Right now, the system only checks if a user was successfully authenticated.
const isAuthenticated: boolean = await userIsAuthenticated(username, password);
if(isAuthenticated) {
redirectToUserDashboard();
} else {
returnErrorOnLoginPage("Credentials are not valid.");
}
New Business Rules
Our company now wants us to check if users are active. Inactive users will not be able to log in.
Many developers will do something like this:
const user: User = await userIsAuthenticated(username, password);
const isAuthenticated: boolean = user !== null;
if(isAuthenticated) {
if(user.isActive) {
redirectToUserDashboard();
} else {
returnErrorOnLoginPage("User is not active.");
}
} else {
returnErrorOnLoginPage("Credentials are not valid.");
}
Oh no. We've introduced code smells that we know are going to cause maintainability issues!
We've got some null checks and nested conditions in there now (which are both signs of unhealthy code that are addressed in the Refactoring TypeScript book.)
So, let's refactor that first by applying (a) the special case pattern and (b) guard clauses (both of these techniques are explained at length in the book too.)
// This will now always return a User, but it may be a special case type
// of User that will return false for "user.isAuthenticated()", etc.
const user: User = await userIsAuthenticated(username, password);
// We've created guard clauses here.
if(!user.isAuthenticated()) {
returnErrorOnLoginPage("Credentials are not valid.");
}
if(!user.isActive()) {
returnErrorOnLoginPage("User is not active.");
}
redirectToUserDashboard();
Much better.
More Rules...
Now that your managers have seen how fast you were able to add that new business rule, they have a few more they need.
If the user's session already exists, then send the user to a special home page.
If the user has locked their account due to too many login attempts, send them to a special page.
If this is a user's first login, then send them to a special welcome page.
Yikes!
If you've been in the industry for a few years, then you know how common this is!
At first glance, we might do something naïve:
// This will now always return a User, but it may be a special case type
// of User that will return false for "user.isAuthenticated()", etc.
const user: User = await userIsAuthenticated(username, password);
// We've created guard clauses here.
if(!user.isAuthenticated()) {
returnErrorOnLoginPage("Credentials are not valid.");
}
if(!user.isActive()) {
returnErrorOnLoginPage("User is not active.");
}
if(user.alreadyHadSession()) {
redirectToHomePage();
}
if(user.isLockedOut()) {
redirectToUserLockedOutPage();
}
if(user.isFirstLogin()) {
redirectToWelcomePage();
}
redirectToUserDashboard();
Notice that because we introduced guard clauses, it's much easier to add new logic here? That's one of the awesome benefits of making your code high-quality - it leads to future changes being much easier to change and add new logic to.
But, in this case, there's an issue. Can you spot it?
Our User class is becoming a dumping ground for all our authentication logic.
Is It Really That Bad?
Is it that bad? Yep.
Think about it: what other places in your app will need this data? Nowhere - it's all authentication logic.
One refactoring would be to create a new class called AuthenticatedUser and put only authentication-related logic in that class.
This would follow the Single Responsibility Principle.
But, there's a much simpler fix we could make for this specific scenario.
Just Use Enums
Any time I see this pattern (the result of a method is a boolean or is an object that has booleans which are checked/tested immediately), it's a much better practice to replace the booleans with an enum.
From our last code snippet above, let's change the method userIsAuthenticated to something that more accurately describes what we are trying to do: tryAuthenticateUser.
And, instead of returning either a boolean or a User - we'll send back an enum that tells us exactly what the results were (since that's all we are interested in knowing).
enum AuthenticationResult {
InvalidCredentials,
UserIsNotActive,
HasExistingSession,
IsLockedOut,
IsFirstLogin,
Successful
}
There's our new enum that will specify all the possible results from attempting to authenticate a user.
Next, we'll use that enum:
const result: AuthenticationResult = await tryAuthenticateUser(username, password);
if(result === AuthenticationResult.InvalidCredentials) {
returnErrorOnLoginPage("Credentials are not valid.");
}
if(result === AuthenticationResult.UserIsNotActive) {
returnErrorOnLoginPage("User is not active.");
}
if(result === AuthenticationResult.HasExistingSession) {
redirectToHomePage();
}
if(result === AuthenticationResult.IsLockedOut) {
redirectToUserLockedOutPage();
}
if(result === AuthenticationResult.IsFirstLogin) {
redirectToWelcomePage();
}
if(result === AuthenticationResult.Successful) {
redirectToUserDashboard();
}
Notice how much more readable that is? And, we aren't polluting our User class anymore with a bunch of extra data that is unnecessary!
We are returning one value. This is a great way to simplify your code.
This is one of my favorite refactorings! I hope you will find it useful too.
Bonus: Strategy Pattern
Whenever I use this refactoring, I know automatically that the strategy pattern may help us some more.
Imagine the code above had lots more business rules and paths.
We can further simplify it by using a form of the strategy pattern:
const strategies: any = [];
strategies[AuthenticationResult.InvalidCredentials] =
() => returnErrorOnLoginPage("Credentials are not valid.");
strategies[AuthenticationResult.UserIsNotActive] =
() => returnErrorOnLoginPage("User is not active.");
strategies[AuthenticationResult.HasExistingSession] =
() => redirectToHomePage();
strategies[AuthenticationResult.IsLockedOut] =
() => redirectToUserLockedOutPage();
strategies[AuthenticationResult.IsFirstLogin] =
() => redirectToWelcomePage();
strategies[AuthenticationResult.Successful] =
() => redirectToUserDashboard();
strategies[result]();
How To Keep Your Code Healthy
This post was an excerpt from Refactoring TypeScript which is designed as an approachable and practical tool to help developers get better at building quality software.
Keep In Touch
Don't forget to connect with me on:
Navigating Your Software Development Career Newsletter
An e-mail newsletter that will help you level-up in your career as a software developer! Ever wonder:
✔ What are the general stages of a software developer?
✔ How do I know which stage I'm at? How do I get to the next stage?
✔ What is a tech leader and how do I become one?
✔ Is there someone willing to walk with me and answer my questions?
Sound interesting? Join the community!
Top comments (12)
There are a couple of good examples here! One thing that I don’t like about the enum is that it mixes two concerns:
I would probably use a type like a tuple that returns something like
{Success, [FirstTime]}
{Failure, [LockedOut]}
Just a quick scratch, but you might the idea.
The canonical term you'll find in other documents including research papers is Primitive Obsession.
Smells usually don't tell us that something is wrong, but only indicate their might be something stinky. Overuse sounds very emotive to me.
Great article idea
Yes, I wanted the book to be a bit more approachable and less academic-like, so I took some liberties with the official terms for most code smells.
Thanks!
Ah. That makes sense to me!
I'm personally not a fan of this specific term (whereas I like the other conventions you've forgone 🔥💝) because the term is judgy and opinionated.
Keep writing 🍻
Any reason we wouldn't use switch/case with the strategy pattern?
You can. If there many different cases then I just find this technique cleaner.
Great post
Thanks Ben.
Hm... cool patterns! And thanks for sharing parts of your book for free. Definitely buying it 😘
Awesome!
As a student this is helpful because we don‘t practice enough hands-on to develop smarter codings. Thanks for sharing!
hehe, I am a swift/iOS developer by preference, (web) fullstack developer by trade. I’ve spent a long time wishing j/typescript had a pretty way of doing guard statements, and widely adopted.