A couple things,
You should be using dependency injection, whenever creating classes. That is just a fancy way of saying pass any data into the class as arguments.
The reason can be illustrated with this one.
public function getID() {
if (isset($_GET['products_id']) && is_numeric($_GET['products_id']) && !empty(HTML::sanitize($_GET['products_id']))) {
$this->Id = HTML::sanitize($_GET['products_id']);
} else {
$this->Id = null;
}
...
}
The problem here is this method can only be used if you have a Query string in the url with products_id in it. Which means you cannot reuse this code unless the "dependency" is met.
Instead of that do this:
public function getID($product_id=null) {
if($product_id){
$this->Id = HTML::sanitize($product_id);
}else{
$this->Id = null;
}
...
}
When you call this method check The get variable
$product_id = empty($_GET['products_id']) ? null : $_GET['products_id'];
$obj->getID($product_id);
Now if you happen to have a product ID you want to use from some where else, maybe it's $_POST you can reuse this code.
$product_id = empty($_POST['products_id']) ? null : $_POST['products_id'];
$obj->getID($product_id);
No issue.
The same holds true for global. Instead of depending on it existing, pass the data in.
class fp_new_products {
public function execute($products_id) {
....
while ($Qproduct->fetch() ) {
$products_id = $Qproduct->valueInt('products_id');
...
}
}
And then when you call
$obj->execute($products_id);
This way your classes don't care where the data comes from, it could be a global, POST, GET or whatever and your code is still usable.
In fact you should not depend on any of those things within a class.
Now if you are changing the value (it looks like you are), you have 2 choices. You can return it and set it:
public function execute($products_id) {
....
while ($Qproduct->fetch() ) {
//this makes no sense by the way...
$products_id = $Qproduct->valueInt('products_id');
...
}
return $products_id; //return the results
}
$products_id = $obj->execute($products_id);
Or you can pass by reference with the &
public function execute(&$products_id) {
....
while ($Qproduct->fetch() ) {
//this makes no sense by the way...
$products_id = $Qproduct->valueInt('products_id');
...
}
}
$obj->execute($products_id);
And it will be "magically" updated.
Lastly as I said this:
//this makes no sense by the way...
$products_id = $Qproduct->valueInt('products_id');
If you look at it, that loop will overwrite this value on each iteration. Which really is probably not what you want.
One thing that may help you a lot is this SOLID principles. Either that or it will just confuse you more ... lol. Basically it has to do with changing how you think of classes, they should have a single responsibility and should be a "Black Box", the outside world shouldn't care what they do, and they shouldn't care about the outside world.
For example (using the first one). The method getID shouldn't care that that data came from $_GET.
Hope that helps.
UPDATE
Just to clarify this:
class Prod {
public function getID() {
if (isset($_GET['products_id']) && is_numeric($_GET['products_id']) && !empty(HTML::sanitize($_GET['products_id']))) {
$this->Id = HTML::sanitize($_GET['products_id']);
} else {
$this->Id = null;
}
-- listing
if (is_null($this->Id) ) {
if (isset($_GET['products_id']) && is_numeric($_POST['products_id']) && !empty(HTML::sanitize($_POST['products_id']))) {
$this->Id = HTML::sanitize($_POST['products_id']);
} else {
global $products_id;
$this->Id = HTML::sanitize($products_id);
}
}
return $this->Id;
}
Can be done this way:
class Prod {
public function getID($products_id=null) {
if ($products_id) {
$this->Id = HTML::sanitize($products_id);
} else {
$this->Id = null;
}
-- listing
return $this->Id;
}
See how much smaller it is. Because the class doesn't need to care about $_GET vs $_POST. But when you call this you will check those.
$obj->getID((empty($_GET['products_id'])?null:$_GET['products_id']));
Which is basically like saying this
if(empty($_GET['products_id']))
$products_id = null;
else
$products_id = $_GET['products_id'];
$obj->getID($products_id);
You should know when you make the call rather you are dealing with $_GET or $_POST.
You can probably simplify this further, if you only pass in Integers or Null
class Prod {
public function getID($products_id=null) {
$this->Id = $products_id ? $products_id : null;
-- listing
return $this->Id;
}
$obj->getID((int)$products_id); //instead of is_numeric
But this is starting to get to the Classroom stage, instead of the simple answer stage.
