5 replies to this topic

[Mko]

Hey all,
I've written some relatively straightforward functions designed to simply query a database. However, some of my friends have told me that what I currently have written is 'inefficient'. I'm a bit confused as to what they truly meant and what aspects of my code are inefficient

My code:

function udb_sqli_query($pquery) {
$mysqli = new mysqli(HOST, USERNAME, PASSWORD, USER_DATABASE);
if (!@$mysqli) {
die("Could not connect to MySQLi Database: " . mysqli_error($mysqli));
}
$query = @mysqli_query($mysqli, $pquery);
if (!@$query) {
die("Error running Query ('" . $pquery . "'): " . mysqli_error($mysqli));
}
$mysqli->close();
return $query;
}
function promo_sqli_query($pquery) {
$mysqli = new mysqli(HOST, USERNAME, PASSWORD, PROMO_DATABASE);
if (!@$mysqli) {
die("Could not connect to MySQLi Database: " . mysqli_error($mysqli));
}
$query = @mysqli_query($mysqli, $pquery);
if (!@$query) {
die("Error running Query ('" . $pquery . "'): " . mysqli_error($mysqli));
}
$mysqli->close();
return $query;
}

Essentially, I'm wondering about:
a. What parts in my code could be made more efficient?
b. If connecting to the database every time a query is executed is the wrong way to go about doing this?
c. If I shouldn't be connecting to the database every time a query is executed, what would be a better way so my code is more efficient while yielding the same results?

Thanks for any and all help,
Mark

Edited by Mko, 20 January 2013 - 01:09 PM.

[kicken]

Essentially, I'm wondering about:
a. What parts in my code could be made more efficient?
b. If connecting to the database every time a query is executed is the wrong way to go about doing this?

You should be connecting only once.  Creating the database connection involves a lot of overhead in setting up the TCP/IP Connection, initializing settings, etc.  This is something you want to do only once then run all your queries using the same connection.

c. If I shouldn't be connecting to the database every time a query is executed, what would be a better way so my code is more efficient while yielding the same results?

You want to only create your connection once and just reference it each time you need to run a query.  Have a function to do the connection for you and save the connection to a variable.  Have your query functions accept a parameter which is the connection to use for running the query.  A nice way to do this is to wrap it all up in a class so you can store the connection variable as a class level variable.  MySQLI already supports an OOP style setup which you should be able to just extend to add whatever functionality you may want.

Eg:

class DB extends MySQLi { 
    public function udb_query($query){
        if (!$this->select_db(USER_DATABASE)){
            die('Could not change database');
        }
        
        $res = $this->query($query);
        if (!$res){ 
            die('Error running query');
        }
        return $res;
    }

    public function promo_query($query){
        if (!$this->select_db(PROMO_DATABASE)){
            die('Could not change database');
        }
        
        $res = $this->query($query);
        if (!$res){ 
            die('Error running query');
        }
        return $res;
    }
}

[Mko]

You should be connecting only once. Creating the database connection involves a lot of overhead in setting up the TCP/IP Connection, initializing settings, etc. This is something you want to do only once then run all your queries using the same connection.



You want to only create your connection once and just reference it each time you need to run a query. Have a function to do the connection for you and save the connection to a variable. Have your query functions accept a parameter which is the connection to use for running the query. A nice way to do this is to wrap it all up in a class so you can store the connection variable as a class level variable. MySQLI already supports an OOP style setup which you should be able to just extend to add whatever functionality you may want.

Eg:

class DB extends MySQLi { 
    public function udb_query($query){
        if (!$this->select_db(USER_DATABASE)){
            die('Could not change database');
        }
        
        $res = $this->query($query);
        if (!$res){ 
            die('Error running query');
        }
        return $res;
    }

    public function promo_query($query){
        if (!$this->select_db(PROMO_DATABASE)){
            die('Could not change database');
        }
        
        $res = $this->query($query);
        if (!$res){ 
            die('Error running query');
        }
        return $res;
    }
}

Thanks for the reply!
One final question: When you say "store the connection variable as a class level variable", I'm a bit confused as to what you mean by that. Could you provide an example of declaring a class level variable, please?

Thanks

[kicken]

class DB {
  private $mConnection;  //class level variable
 
  public function __construct($host, $user, $pass){
     $this->mConnection = mysqli_connect($host, $user, $pass);
  }

  public function query($query){
     return mysqli_query($this->mConnection, $query);
  }
}

The $mConnection variable is a class-level variable.  It is available to any function defined within that class, all you have to do is use it as $this->mConnection.  That class would connect when you first create it, then on each query use the connection.

In my previous post the class extends the already existing mysqli class which already manages the connection for you, so there is no need for such a connection variable inside the class.

[Mko]

Alright. I've used what you've said and implemented it successfully (I think).
How does this look?

My file (called test.php, this is NOT the class):

include_once('functions.php');
define("HOST", "localhost");
define("USERNAME", "a");
define("PASSWORD", "b");
$conn_o = new DB(HOST, USERNAME, PASSWORD);
$query = $conn_o->udb_query($conn_o, "SELECT * FROM `user` WHERE `userid`='" . (int) $vbulletin->userinfo['userid'] . "';");
$conn_o->close();
$row = mysqli_fetch_array($query);

The class (called functions.php):

class DB extends MySQLi {
    public function udb_query($mysqli, $query){
        if (!$this->select_db(USER_DATABASE)){
            die('Could not change database!');
        }

        $res = $this->query($query);
        if (!$res){
            die("Error running Query ('" . $pquery . "'): " . mysqli_error($mysqli));
        }
        return $res;
    }

    public function promo_query($mysqli, $query){
        if (!$this->select_db(PROMO_DATABASE)){
            die('Could not change database!');
        }

        $res = $this->query($query);
        if (!$res){
            die("Error running Query ('" . $pquery . "'): " . mysqli_error($mysqli));
        }
        return $res;
    }
}

How does it look? Also, would it be better if I made a function inside the class to connect so I'd remove the need to have to define the DB values in the file?

Thanks for your continued help

[kicken]

You don't need to be passing your $conn_o variable into the functions.  It is implicitly available as $this.

class DB extends MySQLi {
        public function udb_query($query){
                if (!$this->select_db(USER_DATABASE)){
                        die('Could not change database!');
                }

                $res = $this->query($query);
                if (!$res){
                        die("Error running Query ('" . $query . "'): " . $this->error);
                }
                return $res;
        }

        public function promo_query($query){
                if (!$this->select_db(PROMO_DATABASE)){
                        die('Could not change database!');
                }

                $res = $this->query($query);
                if (!$res){
                        die("Error running Query ('" . $query . "'): " . $this->error);
                }
                return $res;
        }
}

I would also suggest you re-design how you handle your errors and either return false or throw an exception rather than call die().  Then in the code which is issuing the query you check for the failure and handle it gracefully, such as by showing an error page of some sort.

Edited by kicken, 20 January 2013 - 05:51 PM.