DEV Community

Cover image for Unhealthy Code: Primitive Overuse
James Hickey
James Hickey

Posted on • Originally published at blog.jamesmichaelhickey.com

Unhealthy Code: Primitive Overuse

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.


Refactoring TypeScript book


Identification Of Primitive Overuse

Take this code, for example:

const email: string = user.email;

if(email !== null && email !== "") {
    // Do something with the email.
}
Enter fullscreen mode Exit fullscreen mode

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;
Enter fullscreen mode Exit fullscreen mode

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.");
}
Enter fullscreen mode Exit fullscreen mode

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.");
}
Enter fullscreen mode Exit fullscreen mode

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();
Enter fullscreen mode Exit fullscreen mode

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.

  1. If the user's session already exists, then send the user to a special home page.

  2. If the user has locked their account due to too many login attempts, send them to a special page.

  3. 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();
Enter fullscreen mode Exit fullscreen mode

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
}
Enter fullscreen mode Exit fullscreen mode

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();
}
Enter fullscreen mode Exit fullscreen mode

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]();
Enter fullscreen mode Exit fullscreen mode

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.

Refactoring TypeScript book

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)

Collapse
 
stealthmusic profile image
Jan Wedel • Edited

There are a couple of good examples here! One thing that I don’t like about the enum is that it mixes two concerns:

  1. Authentication Result (Success, Failure), there are really only those two
  2. User State, might even allow more than one (FirstLogin, LockedOut) etc

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.

Collapse
 
sleeplessbyte profile image
Derk-Jan Karrenbeld

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

Collapse
 
jamesmh profile image
James Hickey

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!

Collapse
 
sleeplessbyte profile image
Derk-Jan Karrenbeld

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 🍻

Collapse
 
antontsvil profile image
Anton T

Any reason we wouldn't use switch/case with the strategy pattern?

Collapse
 
jamesmh profile image
James Hickey

You can. If there many different cases then I just find this technique cleaner.

Collapse
 
ben profile image
Ben Halpern

Great post

Collapse
 
jamesmh profile image
James Hickey

Thanks Ben.

Collapse
 
selbekk profile image
selbekk

Hm... cool patterns! And thanks for sharing parts of your book for free. Definitely buying it 😘

Collapse
 
jamesmh profile image
James Hickey

Awesome!

Collapse
 
jenbutondevto profile image
Jen

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.

Collapse
 
techwekk profile image
Tijana Wekker

As a student this is helpful because we don‘t practice enough hands-on to develop smarter codings. Thanks for sharing!