current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 0 down vote favorite

I am not very experienced in php oop. Moreover, when mysqli comes with php oop, it make me more confused about using it in best efficient way. Anyway, first look at my connection class:

<?php
//connectionclass.php
class connection{
public $conn;
public $warn;
public $err;

function __construct(){
  $this->connect();
}

private function connect(){
  $this->conn = @ new mysqli('localhost', 'sever_user', 'user_password');
    if ($this->conn->connect_error) {
      $this->conn = FALSE;
      $this->warn = '<br />Failed to connect database! Please try again later';
     }
}

public function get_data($qry){
  $result = $this->conn->query($qry);
  if($result->num_rows>=1){
    return $result;
  }else{
    $this->err = $this->conn->error;
    return FALSE;
  }
  $result->free();
}

public function post_data($qry){
  $this->conn->query($qry);
  if($this->conn->affected_rows>=1){
    return TRUE;
  }else{
    $this->err = $this->conn->error;
    return FALSE;
  }
}
}
?>

Now please structure of a php page which uses mysql database to store and get data:

<?php
//login.php
include('/include/connectionclass.php');
$db = new connection();

$query = "SELECT * FROM USERS WHERE user_country='India'";
$data = $db->get_data($query);
  if($data){
    while($row=$data->fetch_assoc()){
      echo 'User Name: ':.$row["user_name"].' Age: '.$row["age"];
    }
  }
?>

So my login.php uses connection class to get data about users. All the things are running well. But one things made me confused. In connectionclass.php $this->conn is itself an object as it calls new mysqli. So this is an object inside another object $db. Moreover, When I am using $data = $db->get_data($query);, a result set is created inside object $db by method get_data, then this result set is copied into a variable $data inside login.php page.

So according to me, actually two instances of same result sets/data sets are creating here, one inside db object and one inside login page. Is it right way to use mysqli and php to get dataset from mysql database? Will it use more memory and server resources when the dataset is larger (when have to get large amount of data for many users)?

If it is not right way, please explain your points and please give me code which can be used efficiently for php oop and mysqli.

share|improve this question
up vote 1 down vote

No, this isn't the right way to use OOP or MySQLi.

First of all, you only provide two methods to access your wrapped mysqli instance: one to get data, one to post data.

This hides a lot of useful and necessary functions. You can't even use prepared statements with your code, making it incredibly insecure.

You also didn't really gain anything with your functions. With your functions, your code is:

$query = "SELECT * FROM USERS WHERE user_country='India'";
$data = $db->get_data($query);
  if($data){
    while($row=$data->fetch_assoc()){
      echo 'User Name: ':.$row["user_name"].' Age: '.$row["age"];
    }
  }

Without your functions it would be:

$query = "SELECT * FROM USERS WHERE user_country='India'";
$data = $db->query($query);
  if($data){
    while($row=$data->fetch_assoc()){
      echo 'User Name: ':.$row["user_name"].' Age: '.$row["age"];
    }
$data->free();

It's essentially the same code, making your functions unnecessary.

If you do want to wrap mysqli - which may not actually be necessary in your case -, make sure that you:

  • provide all the needed functionalities.
  • actually improve mysqli (make it easier to use, add functionalities, ...).
  • keep the interface as close to that of mysqli as possible, to make it easier to use.

Instead of wrapping mysqli, it may make sense to aim for less abstract methods. For example, you may have a UserDAO, which may have methods such as selectByCountry.

share|improve this answer
    
Dear tim, I used class method get_data in order to ensure that if there is atleast one row or more in result set, then the while statement will be executed. But if I use without method (your 2nd chunk of code), then even if there is no matching rows, it will be considered that the query is executed successfully (as there is no error in code) and the while statement will be executed even there is no rows in returned result set. Am I right? – Abdullah Mamun-Ur- Rashid May 23 at 14:36
    
@AbdullahMamun-Ur-Rashid in that case the if would just need to be changed to if ($data->num_rows>=1). Either way, it's essentially the same, your method really only adds the free call. If you consider all the downsides of it (developers are used to the mysqli API, not your methods; essential methods are missing; etc), I don't think that it's worth having this method. – tim May 23 at 17:00
    
Dear Tim, Thanks for your reply. Please keep it mind that I myself is not satisfied with my code, that's why I began this question to this forum. My objective is not to defend you but to understand the background reason from different point of view. Please make me clear for one point. If I use the class and method (get_data), then if the query is not executed, I can get error by that method, I don't have to write code every time to get the error. But If I use direct query, I have to write else statement everytime to collect error. How to overcome this for direct query/mysqli API? – Abdullah Mamun-Ur- Rashid May 23 at 17:19
    
@AbdullahMamun-Ur-Rashid I'm not sure that I see the difference. You either access the error variable of your custom class, or of mysqli directly, but either way, you have to first check if it's a new error message or an old one. I don't see the advantage of your approach. Maybe I'm overlooking something obvious here, but it doesn't really matter either way, as your code is currently too limited to be used in practice (you can't use prepared statements with it, and are thus vulnerable to SQL injection as soon as you need variable input in your queries). – tim May 23 at 17:31
    
Tim, I have omitted escaping methods here for the sake of keeping my sample code simple. Injection can be prevented from the class method too, that doesn't matter. But I have to write if($data){------}else{ $error[]=$db->error;} for every database query. Would you tell me one way so that I don't have to write $error[]=$db->error for every database query for mysqli API? – Abdullah Mamun-Ur- Rashid May 23 at 17:39
up vote 0 down vote 
class db
{
    private $conn;
    public $warn;
    public $err;

    function __construct()
    {
        $this->conn = @ new mysqli('localhost', 'sever_user', 'user_password');
        if ($this->conn->connect_error) {
            $this->conn = FALSE;
            $this->warn = '<br />Failed to connect database! Please try again later';
        }
    }

    public function Fetch($qry)
    {
        $result = $this->conn->query($qry);
        if ($result->num_rows>=1) {
            return $result;
        } else {
            $this->err = $this->conn->error;
            return FALSE;
        }
    }

    public function Insert($qry)
    {
        $this->conn->query($qry);
        if ($this->conn->affected_rows > 0) {
            return TRUE;
        } else {
            $this->err = $this->conn->error;
            return FALSE;
        }
    }
}

So I changed it a bit:

  1. added indenting
  2. removed your private connection function
  3. changed names

What I would Change on your structure. return the rows instead of a mysqli object. You are asking for rows, give them directly not a sublayer

    public function Fetch($qry)
    {
        $result = $this->conn->query($qry);
        if ($result->num_rows>=1) {
            return $result->fetch_assoc();
        } else {
            $this->err = $this->conn->error;
            return FALSE;
        }
    }

Also throw an error when your get a database error or check if $conn is true

share|improve this answer
    
Raoul thanks for your comment. If I fetch rows inside class object, how it could be possible to get if there are more than one rows in the result set? – Abdullah Mamun-Ur- Rashid May 23 at 14:28
    
you do know that the function fetch_assoc() gets all rows right? your while simply looped through that array – Raoul Mensink May 24 at 7:01

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.