Hardcoded constraints and how to avoid them

Introduction

I review code of other developers quite often. I’ve finished projects initially started by other developers many times. What I often see in the code is using some hardcoded constraints and it very often cause serious problems, especially when project is growing and is developed by multiple developers.

Using strings / numbers

The first thing is really common. Often you can find code similar this:

$admins = User::where('role', 'admin')->get();

1	$admins = User::where('role', 'admin')->get();

At first glance there is no problem with this code but when you think a bit more – what is admin here? It’s a simple string and are you sure this role is admin and not for example administrator? What in case today it is admin but later someone will change this in database to administrator just to display longer version of the same in Html select?

Also assuming you use similar code in many places of your application it’s not very hard to imagine, that someone finally made a typo for example like this:

$admins = User::where('role', 'adnin')->get();

1	$admins = User::where('role', 'adnin')->get();

Have you noticed that someone used n instead of m now? In case this is not a crucial part of system such bug could cause serious problems in application business logic and it might be found after a few days, weeks or months.

Solution is quite simple – instead of using hardcoded strings, use always constants. You can put such constant for example in User model or if you have more roles you can create dedicated RoleType class like this:

class RoleType
{
    const ADMIN = 'admin';
    const USER = 'user';
}

$admins = User::where('role', RoleType::ADMIN)->get();

class RoleType

{

const ADMIN = 'admin';

const USER = 'user';

}

$admins = User::where('role', RoleType::ADMIN)->get();

so instead of using hardcoded string, you are now using constant. There are 2 main benefits of such attitude.

First of all, in case someone decides to update admin into administrator in database for example, it should be enough to update only this constant value like so:

const ADMIN = 'administrator';

1	const ADMIN = 'administrator';

and nothing more should be changed.

The second thing it’s much harder to make typo when using constants. Your IDE will help you to use constant name so you shouldn’t be able to any more make typo like this ‚adnin’ when using constants.

Using hardcoded arrays

Similar situation can happen when you use hardcoded arrays. Let’s assume in your code you want to get users with all roles and you wrote code like this:

$users = User::where('role', [RoleType::ADMIN, RoleType::USER])->get();

1	$users = User::where('role', [RoleType::ADMIN, RoleType::USER])->get();

Of course this is quite naive example but shows the problem. You can validate role also when creating user. And use roles array there. As you see constants were already used here so what’s the problem in this code?

The main problem is that you used hardcoded array [RoleType::ADMIN, RoleType::USER] and in fact any new developer doesn’t know what this array really contain. Well, it contains 2 roles – admin and user but should it really contain those?

Let’s assume you wrote code like this and the application you wrote is quite complicated. You don’t work any more on this code and there is new developer who is working with this code. His duty is to add new role, so he added new const to RoleType class:

class RoleType
{
    const ADMIN = 'administrator';
    const USER = 'user';
    const MANAGER = 'manager';
}

class RoleType

{

const ADMIN = 'administrator';

const USER = 'user';

const MANAGER = 'manager';

}

and he implemented handling this new role in multiple places but he finally found this line:

$users = User::where('role', [RoleType::ADMIN, RoleType::USER])->get();

1	$users = User::where('role', [RoleType::ADMIN, RoleType::USER])->get();

and you know what – he has a serious problem. $users variable is later used by multiple other classes and they don’t know – should manager be added into this array or maybe not? Does this array represents all the users or maybe only users with some selected roles? To be honest I wouldn’t know what I should reply to this developer because I would have to analyze a lot of other code to find out what is happening in there.

That’s why similar to using constants, it’s also very useful to have methods that will help you avoid using hardcoded arrays.

For example in our case we could use:

class RoleType
{
    const ADMIN = 'administrator';
    const USER = 'user';
    const MANAGER = 'manager';

    public static function all()
    {
        return [
            static::ADMIN,
            static::USER,
            static::MANAGER,
        ];
    }

    public function userOrAdmin()
    {
        return [
            static::ADMIN,
            static::USER,
        ];
    }

}

class RoleType

{

const ADMIN = 'administrator';

const USER = 'user';

const MANAGER = 'manager';

public static function all()

{

return [

static::ADMIN,

static::USER,

static::MANAGER,

];

}

public function userOrAdmin()

{

return [

static::ADMIN,

static::USER,

];

}

and now, depending on code logic we could use:

$users = User::where('role', RoleType::all())->get();

1	$users = User::where('role', RoleType::all())->get();

$users = User::where('role', RoleType::userOrAdmin())->get();

1	$users = User::where('role', RoleType::userOrAdmin())->get();

If the code looked like this at the beginning after adding manager role, the only thing that new developer should do is adding manager role to array returned by all method as showed above.

Using hardcoded identifiers

Probably this is the worst thing but it also happens in some applications. Let’s assume you have moved roles into separate table and you have such data:

# Id # Description #

==============================

# 1 # System administrator #

# 2 # User #

# 3 # Manager #

Let’s assume we need to find system administrators so we wrote code like this:

$users = User::where('role_id', 1)->get();

1	$users = User::where('role_id', 1)->get();

because in users table we have now role_id column instead of role column. What is the problem here?

Well, in fact there are 2 problems here. First of all we again used hardcoded value – well, it’s not a string any more but it’s hardcoded number – are you sure that anyone would guess what users are chosen here without looking at database? Does 1 means system administrators or maybe regular users? It’s hard to say, because 1 tells us nothing. It’s again also really easy to make a typo and instead of 1 use for example 12.

But the other, even more serious thing is – are you sure that administrator role has really id 1? Let’s imagine in other system it doesn’t. Could it cause serious problems for your application? Probably it could and it’s not so hard to imagine when in other database roles have different ids. Let’s assume for example when deploying application on production some exception was thrown and during adding roles to database rollback was done. After solving the problem and adding roles again to database it might happen that admin role will have id 4, user role will have id 5 and manager role will have id 6.

I hope you imagine how problematic would it be and how big impact would it have to your application.

The solution? Never rely on ids of objects for such cases. What you should do is using slugs for this. So after modification your roles table could look like this:

# Id # Slug # Description #

========================================

# 1 # admin # System administrator #

# 2 # user # User #

# 3 # manager # Manager #

so as you see additional slug column was added into database.

Your role class could look like this:

class Role extends Model
{
    const ADMIN = 'administrator';

    public static function findBySlug($slug)
    {
        return static::where('slug', $slug)->firstOfFail();
    }

}

class Role extends Model

{

const ADMIN = 'administrator';

public static function findBySlug($slug)

{

return static::where('slug', $slug)->firstOfFail();

}

and now instead of

$users = User::where('role_id', 1)->get();

1	$users = User::where('role_id', 1)->get();

you would use:

$users = User::where('role_id', Role::findBySlug(Role::ADMIN)->id)->get();

1	$users = User::where('role_id', Role::findBySlug(Role::ADMIN)->id)->get();

Although it’s longer it’s much better. Now it’s really obvious that user taken from database should be admin. Of course in such case you could create some helper method to use a bit shorter syntax for example:

$users = User::byRole(Role::ADMIN)->get();

1	$users = User::byRole(Role::ADMIN)->get();

and you could also consider caching because now additional query will be run that will find id of admin role in database and it’s very likely this won’t be ever or almost ever changed.

author: Marcin Nabiałek, leading programmer at Devpark Laravel team