3
\$\begingroup\$

I'm writing a simple class to manage a warehouse, so it's supposed that it will use a database connection. All works fine for now, and I'm actually working to finish the UI. Since I think to be a newbie in PHP OOP, I would appreciate much if anyone can review my code to help me improving it.

Here is the class code:

class market{

private $db;
private $stmt;    

private function database(){

if($this->db === null){        
$this->db = new PDO('mysql:host=localhost;dbname=marymarket','root','root');   
    }
return $this->db;    
}    
 /* I need to pass an array with arguments,any help will be appreciated */      
public function insert_article($barcode,$code,$qty,$brand,$article_name,$price,$article_type,$promo_stats,$note){

$stmt = $this->database()->prepare('INSERT INTO products (barcode,code,qty,brand,article_name,price,article_type, promo_stats,note) VALUES (:barcode,:code,:qty,:brand,:article_name,:price,:article_type,:promo_stats,:note)');    
$stmt->bindParam(':barcode',$barcode,PDO::PARAM_STR);
$stmt->bindParam(':code',$code,PDO::PARAM_INT);    
$stmt->bindParam(':qty',$qty,PDO::PARAM_STR);
$stmt->bindParam(':brand',$brand,PDO::PARAM_STR);
$stmt->bindParam(':article_name',$article_name,PDO::PARAM_STR);
$stmt->bindParam(':price',$price,PDO::PARAM_STR);
$stmt->bindParam(':article_type',$article_type,PDO::PARAM_STR);
$stmt->bindParam(':promo_stats',$promo_stats,PDO::PARAM_INT); 
$stmt->bindParam(':note',$note,PDO::PARAM_STR);

if($stmt->execute()){    
echo 'ok';    
} else {
echo 'error';    
}    

}
// this part is commented because i will implement it after the UI will be ready.
//public function insert_supplier(){

//$stmt = $this->database()->prepare('INSERT INTO suppliers (code,supplier_name,iva,tel,fax,email,address,city,cap) VALUES ()');   
//
//$stmt->bindParam(':',PDO::PARAM_STR);
//$stmt->bindParam(':',PDO::PARAM_STR);
//$stmt->bindParam(':',PDO::PARAM_STR);
//$stmt->bindParam(':',PDO::PARAM_STR);
//$stmt->bindParam(':',PDO::PARAM_STR);
//$stmt->bindParam(':',PDO::PARAM_STR);
//$stmt->bindParam(':',PDO::PARAM_STR);
//$stmt->bindParam(':',PDO::PARAM_STR);
//$stmt->bindParam(':',PDO::PARAM_STR);
//$stmt->bindParam(':',PDO::PARAM_STR);
//    
//$stmt->execute();    
//   
//}

//private function insert_brand(){
//$stmt = $this->database()->prepare('INSERT INTO brands () VALUES ()');   
//
//$stmt->bindParam(':',PDO::PARAM_STR);
//$stmt->bindParam(':',PDO::PARAM_STR);
//$stmt->bindParam(':',PDO::PARAM_STR);
//    
//$stmt->execute();    
//   
//}    
// TO DO    
#public function mod_article(){}
// TO DO        
#public function del_article(){}    

public function list_articles(){

$stmt = $this->database()->query('SELECT * FROM products'); 
foreach($stmt as $result):    

echo '<tr><td id="list-tbl-label">'.$result['barcode'].'</td>';
echo '<td id="list-tbl-label">'.$result['article_code'].'</td>';
echo '<td id="list-tbl-label">'.$result['brand'].'</td>';   
echo '<td id="list-tbl-label">'.$result['article_name'].'</td>';
echo '<td id="list-tbl-label">'.$result['price'].'</td>';   
echo '<td id="list-tbl-label">'.$result['article_type'].'</td></tr>';   
#echo    
endforeach;    
}

public function search($q){

$stmt = $this->database()->prepare('SELECT * FROM products WHERE article_name LIKE :q OR brand LIKE :q OR article_code LIKE :q');    
$stmt->bindParam(':q',$q,PDO::PARAM_STR);    
$stmt->execute();    
if($stmt->rowCount() > 0){
$results = $stmt->fetchAll();    
foreach($results as $result):
echo '<table class="table table-bordered" id="">  <thead>
    <tr>
      <th scope="col">INT. CODE</th>
      <th scope="col">BARCODE</th>
      <th scope="col">QTY</th>
      <th scope="col">BRAND</th>
      <th scope="col">PRODUCT NAME</th>
      <th scope="col">PRICE</th>
      <th scope="col">SUPPLIER</th>
    </tr>
  </thead><tbody>';    
echo '<tr><td>'.$result['article_barcode'].'</td>';
echo '<td>'.$result['article_code'].'</td>';
echo '<td>'.$result['brand'].'</td>';   
echo '<td>'.$result['article_name'].'</td>';
echo '<td>'.$result['price'].'</td>';   
echo '<td>'.$result['article_type'].'</td></tr></tbody></table>';    
endforeach;
    }
else {
    return false;  
    }    
} 

}

Since I don't like to write directly PHP code on my html pages, I also wrote a controller that is called by AJAX. It's supposed that will manage all the necessary operations made by the user who post or need to get data. Here is the controller code:

require_once 'core.php';

$core = new market();

if(isset($_POST['code'] )){

$code = filter_var($_POST['code'],FILTER_SANITIZE_STRING); 
# $barcode = filter_var($_POST['barcode'],FILTER_SANITIZE_INT);    
$qty = filter_var($_POST['quantity'],FILTER_SANITIZE_STRING); 

$brand = filter_var($_POST['brand'],FILTER_SANITIZE_STRING);
$article_name = filter_var($_POST['artName'],FILTER_SANITIZE_STRING);  
$price = filter_var($_POST['price'],FILTER_SANITIZE_STRING);
$article_type = filter_var($_POST['artType'],FILTER_SANITIZE_STRING); 
$note = filter_var($_POST['note'],FILTER_SANITIZE_STRING);    

$save = $core->insert_article($code,$qty,$brand,$article_name,$price,$article_type,$note);    
if($save){
echo 'ok';    
} else {
echo 'error';    
}

}

if(isset($_POST['q'])){
$q = filter_var($_POST['q'],FILTER_SANITIZE_STRING);
$results = $core->search($q);    
echo $results;
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. \$\endgroup\$ – Mathias Ettinger May 18 '18 at 7:28
  • 1
    \$\begingroup\$ @MathiasEttinger ok, sorry I will not doing this anymore. \$\endgroup\$ – user9741470 May 18 '18 at 7:47
3
\$\begingroup\$

I'm sorry but I have to start with the obvious: your indentation is barely non-existing, making your code needlessly harder to read.

Indentation is important (but not mandatory)!


Your use of filter_var(..., FILTER_SANITIZE_STRING); seems kinda shady.
Perhaps you're trying to remove tags?

This causes huge usability issues, like, not being able to have a < symbol anywhere, which can be fixed by using the function htmlentities.

If you try running:

$string = 'I <3 PHP';

echo filter_var($string, FILTER_SANITIZE_STRING), PHP_EOL;
echo strip_tags($string), PHP_EOL;
echo htmlentities($string);

You will see that both filter_var and strip_tags remove the heart.


Your insert_article() method seems kinda shady, and your class does way too many things: connect to the database, deal with articles, suppliers and whatnot.

And if I do anything, a new PDO is created?

Instead of:

private function database(){

if($this->db === null){        
$this->db = new PDO('mysql:host=localhost;dbname=marymarket','root','root');   
    }
return $this->db;    
}

Just do:

public function __construct($host, $username, $password, $db){
    $this->db = new PDO('mysql:host=' . $host . ';dbname=' . $db, $username, $password);
}

And pass the values you want like this:

$market = new market('localhost', 'root', 'root', 'marymarket');

Figuring a way to pass the character encoding is left as an exercise for the reader.


Nitpick time!

  • If you're going to have multiple lines commented, use /* ... */ instead of // or #.
  • To make the name stand out of a function, you should Capitalize The First Letter (market -> Market).
  • Instead of a function that takes 300 arguments, make it receive an array.
  • Instead of echoing ok, error or a dump of the HTML.
    Send a json code with the result.
    Better yet: make each method return an array with the answer, and do echo json_encode(...); with the returned array.
  • The alternative syntax is meant to be used for templating (and looks ugly). Use brackets instead, like how you're using on your if()s.
  • private $stmt; is unused. Remove it.
  • If you intend to run the insert_article() more than once, store the prepared statement into a static $stmt, making sure to free the result.
    If you run only once, skip using prepared statements.
  • If you're going to have an else { return false; }, make it the first thing (Good read: https://softwareengineering.stackexchange.com/questions/18454/should-i-return-from-a-function-early-or-use-an-if-statement)
\$\endgroup\$
  • \$\begingroup\$ Thank you for your suggestions. I've read about the __construct() that can be useful to initialize the PDO driver, and I've modified that to reflect this things. I will modify the class as you suggested. I'm now working to give to the controller the response in a json format and then visualize it using Jquery $.each() . About the insert_article() method, I assume that it will run more than one time when the database is compiled the first time, but it will be used only to add new products available in the warehouse, so I will consider to modify it to remove the prepared statements. \$\endgroup\$ – user9741470 May 17 '18 at 18:41
  • 1
    \$\begingroup\$ I was worried about security of the queries <-- NO!!!!! filter_var does NOT do ANYTHING against sql injection attacks. In fact, it is a nice attack vector. Just add a quote that matches the query and everything falls appart. filter_var and htmlentities are used to clean up the string to present later. Prepared statements are what make your code resistent to sql injection attacks. And if you're running it multiple times, use a static variable. This gives the beneficts of a prepared statement without you having to have a variable for the whole class. \$\endgroup\$ – Ismael Miguel May 17 '18 at 21:34
  • 1
    \$\begingroup\$ Please, do not touch the code in the question! Just post a follow-up question whenever you think it is ready, with a link to this question. Also, what I meant with $stmt being static is to simply add the static keyword before the variable. Literally that. Easier than making a pie or scrambled eggs. This is completely outside the scope of this question, but, instead of $.each() and JSON.parse, use for(var i = 0, length = array.length; i < length; i++ ). Depending on the jQuery version and browsers, use $.parseJSON, JSON.parse or a library (IE7 and older don't have JSON.parse). \$\endgroup\$ – Ismael Miguel May 18 '18 at 9:22
  • 1
    \$\begingroup\$ If you want, you can try posting to stackoverflow.com, respecting the rules and guidelines of that website. \$\endgroup\$ – Ismael Miguel May 18 '18 at 9:46
  • 1
    \$\begingroup\$ @user9741470 While I'm happy to help you and am happy that you accepted my answer, I advise you against it for now. Wait a few days (2-3 days) before accepting an answer. This gives a chance for others and, hopefully, you will find a better answer than mine. \$\endgroup\$ – Ismael Miguel May 18 '18 at 13:07
1
\$\begingroup\$

Is it strictly necessary for you to define the data type for all fields? With the lack of indentation I find your code quite hard to follow - perhaps something like this would be more readable:

// Connect to database

$username = ""; 
$password = ""; 
$host = "";
$dbname = ""; 

try {  
    $db = new PDO("mysql:host={$host};dbname={$dbname};charset=utf8", $username, $password, $options);
} 

catch(PDOException $ex) { 
    die("Failed to connect to database: " . $ex->getMessage()); 
} 


// Insert article

$query_insertArticle = "
    INSERT INTO products
    (barcode, code, qty, brand, article_name, price, article_type, promo_stats, note) 
    VALUES 
    (:barcode, :code, :qty, :brand, :article_name, :price, :article_type, :promo_stats, :note)
";

$query_insertArticle_params = array(
    ':providerID' => $providerID,
    ':barcode' => $barcode,
    ':code' => $code,
    ':qty' => $qty,
    ':brand' => $brand,
    ':article_name' => $article_name,
    ':price' => $price,
    ':article_type' => $article_type,
    ':promo_stats' => $promo_stats,
    ':note' => $note
);

try {
    $stmt = $db->prepare($query_insertArticle);
    $stmt->execute($query_insertArticle_params);
}
catch(PDOException $ex) {
    die("Failed to insert article: " . $ex->getMessage());
}

Your list_articles() function is echoing a lot of HTML code unnecessarily. Try to keep all your PHP and HTML seperate. If you are indeed having both PHP and HTML in the same page (not preferable), do something like the following:

<?php

$stmt = $this->database()->query('SELECT * FROM products');

foreach($stmt as $result):

?>

<tr>
    <td id="list-tbl-label"><?=$result['barcode'];?></td>
    <td id="list-tbl-label"><?=$result['article_code'];?></td>
    <td id="list-tbl-label"><?=$result['brand'];?></td>
    <td id="list-tbl-label"><?=$result['article_name'];?></td>
    <td id="list-tbl-label"><?=$result['price'];?></td>
    <td id="list-tbl-label"><?=$result['article_price'];?></td>
</tr>

<?php

endforeach;

?>

You can do the same with your search results table. Build the table with HTML, and pass the data to it, rather than build the whole thing with PHP echoes.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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