How do I know when to refactor global database query calls, and when to leave them alone?

Question

I have the following global statement calls sprinkled throughout my PHP legacy codebase:

$jobnumber = db_quick($sql);
db_query($sql);

That is, those function calls have been initialized in the global scope and they themselves call upon a previously initialized MySQL global object, i.e:

//initializes MySQL connection object in the global scope
$dblink = MySqlConnector::getMySql();

function db_query($sql)
{
    //previously initialized MySQL connection object
    global $dblink;  

    //native MySQL API call:
    return mysqli_query($dblink, $sql);
}

Question

Are there any benefits to refactoring these global statements, other than just to say "I have now removed them from the global scope"?

The spirit of my question goes ... those are already in the codebase, and they work. Must I mess with them? So far [3 years working with the code] there has been no apparent harm. Even with transition from deprecated mysql to mysqli, it was a painless transition. Will keeping these as global cause any measurable harm later? Are these good enough to be left as is?

What if I refactor?

By the way if I refactor I am thinking of using a Singleton pattern for database link instantiation and then calling it via an instantiated class, that calls the Singleton. Like so:

//in a calling class:
$repository = new GenericRepository();
$repository->getMySql()->persist($sql);
$value = $repository->getMySql()->paramQuery($sql, $param);

//GenericRepository makes a static call to the actual connector
class GenericRepository 
{
    final public function getMySql()
    {
        if ($this->link === null)
            $this->link = MySqlConnector::getMySql();

            return $this->link;
    }
}

//Connector calls MySQL API
class MySqlConnector
{

    public static function getMySql()
    {
        if (null === static::$instance)
        {
            include 'include/config.php';

            //class MySql extends mysqli library
            static::$instance = new MySql(DBHOST, DBUSER, DBPASS);
        }

        return static::$instance;
    }
}

So in essence, by refactoring, I will be removing the globals database methods and replacing them with instantiated calls to the Singleton pattern. The code change will be fairly extensive, as there are a couple of thousand of global database calls.

Effects of the above Refactoring

Currently I can call the methods "from anywhere" - they are global.

db_query($sql);    

After refactoring I will still be able to call them "from anywhere", but using a longer connection string.

(new \Namespace\GenericRepository())->getMySql()->persist($sql);

That sounds a bit like a detriment - longer string, more to type, more to copy & paste. Are there really some benefits to this?

Answer 1

There's a reason why db_query is in the global scope, and that is because you need it to be visible everywhere so that you can call it from anywhere. When you use jQuery,$ is in the global scope, and that's why.

Before you go through a complex refactoring just to remove a function from the global scope, consider the reasons why global scope is problematic. I think you'll find that it's not global scope that is bad per se, but rather global variables.

Also, note that global variables aren't always bad. If you're using one so that you don't have to repeat a connection string endlessly throughout your program, that's a good thing.

Answer 2

Refactoring is not an end in itself, it is a means to an end. And the goal is to make code more and testable, maintainable, and evolvable.

In your case, using the same global function $db_querywhich accesses the same global $dblink object during the whole codebase prevents you from easily using this function with two different database connections during one execution run. The restriction "one and only one db connection per application instance" is manifested into the architecture. Any code which is build upon your $dbquery will have to live with this assumption, and it cannot be easily reused in a different context where the db environment will look differently, or put into a testing context with no database at all.

This can be perfectly ok. For many applications, the "one db connection at a time" architecture is all they need during their whole life span. If your application is of this type, and you do not notice any real drawbacks from this restriction, not even for testing, then leave it as it is.

However, if this architecture starts getting in your way for implementing new features or tests, then a kind of refactoring you need is a refactoring where the $dblink will be provided in a context-dependend manner, or the database behind $dbquery can be easily mocked out. That shows that why your Singleton refactoring is probably the wrong approach, because it does not solve the core restrictions of your architecture.