warehouse management class code

Question

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;
}

Answer 1

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)

Answer 2

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.