I am new to PHP-OOP and am wondering whether there is anything wrong with the way I code. I hope I don't get a decrease in reputation for asking for better coding ways. I have an index.php, product.php, see_category.php.
Below is my product.php:
Next this is my index.php:
Lastly this is my see_category.php
My question is, is this the correct way to do it? Are there better way's to do it?
This is the wrong place for this kind of question, you'll want to be over at codereview.stackexchange.com. However; my opinion is that the see_category.php
file should be something like product_controller.php
and it should serve as a controller for all product related functions. You should be checking for the existence of the post variable and acting accordingly based on what's in the post variable.
require_once('product.php');
$product = new Product();
The server will only be inside of this file if there's a request for a post, no reason not to instantiate the class regardless of the post condition.
if($_POST):
switch($_POST):
case 'category' :
echo $product->set_products_by_category($_POST['category'], TRUE);
break;
endswitch;
endif;
You'll notice in the above I'm not generating a table, I'm just echoing what's returned, but I added a 2nd argument and passed in the TRUE value, let's take a look at our function now.
public function set_products_by_category($category, $echo = FALSE){
//your current code
//after the while statement...
if(TRUE === $echo):
return $this->generate_table();
endif;
}
Because we passed in true as the 2nd argument, it will automatically return the table to us, and we'll echo it out based on our case
condition.
Looks fine, just some points you might wanna investigate: What you are doing is commonly called AHAH, as opposed to AJAX (loading html from the server). Post is usually used to make changes, create products, update products etc. you should stick to GET for the rest. jquery has a very simple way of performing AHAH (this also removes the inline js):
$('#computers').on('click', function() {
$(this).load('see_category.php?cat=computers');
});
You might wanna look up JSON, so that you can move away from rendering on the server side, and serve objects to the client, and render them through javascript.
MySQL Injection
You are doing this, and not escaping mysql characters. So it would be easy to inject code.
"SELECT * FROM product WHERE product_category='" . $category . "'"
The above code I could do a POST, where the $category can contain an injection. Please do a mysql_real_escape_string
Javascript
Provide Javascript in a different file so it is better for your SEO and your website structure. Currently your jquery.js
is in the root directory. If you create a subdirectory for your Javascript files. You got more overview of your project files.
Num rows
You are counting yourself, why not use $sth->num_rows
?
Product class
Seems like your Product
is not actually a product. It is more a group holder. Maybe call it ProductOverview or something more relevant.
Model View Controller
Currently your product class does everything. Maybe look more into a MVC way of building your webpages. Maybe use Smarty Template to keep your HTML seperated. Or a different Template Engine.
OOP
You wan't to be more Object orientated but your using FETCH_ASSOC
which returns an Array not an Object. Why not using FETCH_OBJECT
?
Get products
You are using 2 functions with almost the same result. It might be better to put up an optional parameter where you use 1 function instead of 2.