Really? preg_replace()?

Posted on July 6, 2011 by Jeremy

I found this in a database connection config file. How many problems do you see with this?  I’ll give you a head start — preg_replace() is overkill. And by overkill, I mean “absolutely unnecessary”.

$MM_sql_HOSTNAME = 'hostname';
$MM_sql_DATABASE = 'mssql:dbname';
$MM_sql_DBTYPE = preg_replace('/:.*$/', '', $MM_sql_DATABASE);
$MM_sql_DATABASE = preg_replace('/^[^:]*?:/', '', $MM_sql_DATABASE);
$MM_sql_USERNAME = 'username';
$MM_sql_PASSWORD = 'secret';
This entry was posted in PHP. Bookmark the permalink.
  • http://www.littlehart.net/atthekeyboard Chris Hartjes

    You know, I think what happens in a lot of cases is that people start off with good intentions. I’m sure there was some sort of reason for this decision, a one-off bug they ran into that was solved by using preg_replace to pull some data out. But then it became a hammer to be used all over the damn place. And before you know it, you’re throwing around quotes like David Caruso while putting your sunglasses back on.

    That said, I cannot think of ANY reason to use preg_replace like this for this particular situation. I’m guessing this dev never heard of split() or explode()

  • http://about.me/jeremykendall Jeremy

    Excellent points, Chris. I’ve done things like that more than I’d like to admit.

    My favorite part of this snippet, which I missed at first glance, is that any kind of string manipulation is unnecessary in this case. Trying to figure out why preg_replace() was being used caused me to miss the most obvious error (and the simplest refactoring). Take a careful look at the $MM_sql_DATABASE variable. Both of them ;)

  • http://www.johncongdon.com John Congdon

    They are taking the change only in place thing way to far. I see that they are trying to make it possible to change just the database dsn, and get all the pieces.

    Obviously should have been done the other way, start with individual variables, and concat to make the $MM_sql_DATABASE.

    $MM_sql_HOSTNAME = ‘hostname’;
    $MM_sql_DBTYPE = ‘mssql’;
    $MM_sql_DATABASENAME = ‘dbname’;
    $MM_sql_DATABASE = “$MM_sql_DBTYPE :$MM_sql_DATABASENAME ‘;
    $MM_sql_USERNAME = ‘username’;
    $MM_sql_PASSWORD = ‘secret’;

  • http://about.me/jeremykendall Jeremy

    What you’re saying makes good sense, if the code in the post made any sense at all. Run the snippet and var_dump() $MM_sql_DATABASE and $MM_sql_DBTYPE and you’ll see what I mean.