Class that runs a query

Question

I have made this class that runs a query. I want to optimize my code and want to have some expert advice.

Here is the class:

class testClass extends Model
{
    protected $_var1;
    protected $_var2;
    protected $_var3;


    public function __construct($var1,$var2, $var3)
    {
        $this->_var1= $this->$var1;
        $this->_var2= $this->var2;
       $this->_var3= 8;  // i need this 8 if no value is passed

    }

    public function testMethod(){
       $query = "SELECT * from table where id = ".this->_var1." AND name= ".$this->var2." AND x= ".$this->var3; 
            return $runQuery($query);
    // $runquery is another method…not in concern here..it returns the results…just take my words :)
    }
}

Usage of class:

  $r= new testClass($var1, $var2, $var3);
            $result = $r->testMethod();

How can I improve this? Or is this even a good way to do it?

Answer 1

I'm not sure what you want to achieve with the constructor.

public function __construct($var1,$var2, $var3)
    {
        $this->_var1= $this->$var1;
        $this->_var2= $this->var2;
        $this->_var3= 8;  // i need this 8 if no value is passed

    }

If you want to set the properties to the given parameters, you should do

 $this->_var1= $var1;

---

And with

$this->_var3= 8;  // i need this 8 if no value is passed

you always set property $_var3 to 8, nonetheless if $var3 is being passed.

For setting a default value to a paramter you can write it like this

public function __construct($var1, $var2, $var3 = 8)
        {
            $this->_var1 = $var1;
            $this->_var2 = $var2;
            $this->_var3 = $var3;

        }

so if you call

$r = new testClass($var1, $var2, 10);

$this->_var3 would be set to 10, and if you call

$r = new testClass($var1, $var2);

$this->_var3 would be set to the default 8.

---

In your testMethod you're using $this->var2 & $this->var3, without the underline. These properties are not defined.

---

return $runQuery($query);

remove the $ in the function's name, or it would be taken as variable.

---

In your SQL-Query there's a column named "name". In all cases where the column types are strings, you have to write the value in single quotes.

$query = "SELECT * from table where id = ".this->_var1." AND name= '".$this->_var2."' AND x= ".$this->_var3; 

---

For debugging purposes, set

ini_set('display_errors', 1);
error_reporting(E_ALL);

to the starting script.

Answer 2

This class has one serious problem:

SQL Injection

Your method builds a query from variables. If these variables come from user input then you have an SQL Injection vulnerability. This class has no control over what is passed to it, so it is a vulnerable part of your system. You could avoid it by not passing user input to it, but it is much better to never have the vulnerability in the first place.

It looks like you might be using the mysql_* functions. Please, don't use mysql_* functions to write new code. They are no longer maintained and the community has begun deprecation process. Instead you should learn about prepared statements and use either PDO or MySQLi.

Other Comments

It is good that you are passing your dependencies in your constructor.

Underscore in class properties are a waste of time. In OOP you should almost never have public properties. By having only protected and private properties you can rely on the public method interface to interact with the class. This is an important difference because the public methods of a class are testable. Now, without public properties the underscore just gets in the way.

Consider sticking to a single paradigm. runQuery is a procedure that you have defined. Why not encapsulate the query within an object (PDO is a good object for running queries, or perhaps use a DB object of your own)?