Avoid hard coding in mysql query

Question

I have a table of ban reasons:

id | short_name | description
 1      virus     Virus detected in file
 2      spam      Spammy file
 3      illegal   Illegal content

When I ban a file for being a virus, in my code I do this:

$file -> banVirus();

Which inserts the file id and ban reason into a table:

"INSERT INTO 'banned_files' VALUES (61234, 1)"

My question is; is it a problem that I have hard-coded the value 1?, to indicate a spam file.

Should I use defines in my config like define ('SPAM', 1), so i can replace 1 with a define? Or does it not matter at all?

If you maintained the code, what would you like to see in the source, SPAM or 1? — Joachim Isaksson
– Joachim Isaksson, Commented Oct 11, 2013 at 15:05
Looks fine to me as you've only got 3 ban types, they're very easy to remember. If you had 50+ of them I might think differently. — H2ONOCK
– H2ONOCK, Commented Oct 11, 2013 at 15:07
not really a problem, but you might want to put in a comment saying that this 1 corresponds to an entry in your "ban reasons" table, so later maintainers know where to look... and since you're wrapping the insert in a function with a pretty descriptive name, that helps a lot too — Marc B
– Marc B, Commented Oct 11, 2013 at 15:08
Instead of using an auto-incremented primary key for the ban reason id ... you could just use the short_name that keeps your database relational and normalised and gives you a descriptive ban reason when used elsewhere (in the application for example). — CD001
– CD001, Commented Oct 11, 2013 at 15:08

deceze · Accepted Answer · 2013-10-11 15:08:10Z

2

If the id is an auto incrementing field, then it is a very big problem! Since the ids are automatically generated, it's hard to guarantee their stability; i.e. they may change.

If the id is something you manually assigned, it's not such a big problem, but it's bad practice. Because magic numbers easily lead to confusion and mistakes. Who knows what "1" means when reading your code?

So either way, you'd be better off to assign a stable, readable id to each case.

I agree with @Tenner that it also hardly makes sense to have a table for this static, unchanging data to begin with. Your banned_files table should have a column like this:

reason ENUM('virus', 'spam', 'illegal') NOT NULL

You need nothing more in your database. When outputting this for the user, you can add a readable reason with a simple array through your PHP code.

answered Oct 11, 2013 at 15:08

deceze♦

525k89 gold badges806 silver badges954 bronze badges

Sign up to request clarification or add additional context in comments.

9 Comments

AgRizzo Over a year ago

I disagree with your first statement. While it could be an autoincrementing field, it is in a "lookup" table. There is another table, banned_files that has a foreign key to the banned reason files.

deceze Over a year ago

So? But the 1 is hardcoded in PHP while the id may change in the database. That's an unstable relationship, hence a problem.

paul Over a year ago

That seems a better solution for inserting. If possible, could you provide a simple example of how to add a readable reason using an array?

deceze Over a year ago

@AgRizzo For whatever reason you setup the database tables while the autoincrement value is not set to 1. The records will receive some random id. Just the pure fact that you're relying on auto-generated ids where there's a possibility that they may not be generated exactly the same way every time is bad.

deceze Over a year ago

@paul You decide those values, you have full control over them. You do not have perfect control over auto-incremented values. You'll also insert those values into the database from PHP, e.g.: INSERT INTO banned_files (..., reason) VALUES (..., 'virus'), so they're now hardcoded only in PHP. The column definition in the database is only a restriction which values the column accepts; yes, you'll need to keep that in sync with your code.

|

CD001 · Accepted Answer · 2013-10-11 15:24:30Z

1

Since you have a fixed (and small) number of parameters, I'd be tempted to make the IDs an enum in your code and not even include them as a separate database table at all.

Think about something like gender -- which has two (or more) options, both fixed. (We won't be adding multiple new genders anytime soon.) I guarantee most registration systems' don't have a GENDER table with two entries in it.

So, table banned_files would be something like this:

id      | reason
--------+------------
12345   | 1
67890   | 2

and your code would contain enums as necessary:

enum BanReason {
    Virus = 1,
    Spam = 2,
    Illegal = 3
}

(please convert to PHP; I'm a C# developer!)

In PHP:

$aBanReason = array(
    'Virus' => 1,
    'Spam' => 2,
    'Illegal' => 3
);

edited Oct 11, 2013 at 15:24

CD001

8,5123 gold badges26 silver badges29 bronze badges

answered Oct 11, 2013 at 15:06

James Cronen

5,7732 gold badges36 silver badges53 bronze badges

1 Comment

melc Over a year ago

Since the id is used at the database level as a value of a column it is not good practice to have it described in some random code with enumerations. It couples the two, it is certainly not clear if required to look it up in the future. Having a dictionary table is a common good practice, unless the value is binary like GENDER as you mentioned where a TINYINT in MySQL will suffice and the column name describes itself.

Collectives™ on Stack Overflow

Avoid hard coding in mysql query

2 Answers 2

9 Comments

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

9 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related