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?

• mseancole: Welcome to CR, we don't have "expert" advice, but I'll see what I can do for you. 32bitfloat did a pretty good job already, I just want to add a few things. First, variable names should be descriptive. $var1, $var2, $var3 doesn't tell us enough. Second, your style is inconsistent. The constructor's opening brace is on a new line while the testMethod()'s is on the same line. Your indentation is also pretty inconsistent. Finally, a single line of code should never be so long as to cause illegibility. You appear to know how to concatenate strings, just concatenate to $query a few times.
• mseancole: Sorry, last comment. This probably shouldn't have been a class. You would be much better off working with procedural code first and working your way up to classes and OOP.

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.

• mseancole: If a variable is called as a function then it is called a variable function. These allow you to set a function's name as a string value in a variable and call the function dynamically, however they are considered big no-no's and should be avoided. In this instance though, I do not think he was trying to use variable functions, however that is what he would have been calling, not a variable :)
• 32bitfloat: You're right, I didn't verbalized that well. Just thought OP has a typo and finish =)

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)?

• mseancole: The more I play around with OOP and the more my knowledge grows, the more I realize that bit about the underscore is true. I would like to point out that is only for the properties. I still find it helpful to distinguish my methods with a preceding underscore.