OO and non-OO usage of Mysqli











up vote
1
down vote

favorite












I'm starting to learn the object oriented aspect of PHP. In this regard I made a small "exercise" of sorts "translating" one of my PHP functions into OO mode of PHP. I'm asking if there are there any recommendations you can give me concerning my predicament.



Example of non-OO PHP:



<?php
function pristup($servername, $username, $password, $dbname, $sql){

$conn=new mysqli($servername, $username, $password, $dbname);

if($conn->connect_error){

die("Neuspela konekcija: ".$conn->connect_error);

}

$result = $conn->query($sql);
if ($result == TRUE) {
//echo "Uspela konekcija";
} else {
echo "Neuspešno izvršavanje upita: " . $conn->error;
}
return $result;

$conn->close();

}

?>


Example of this done in an object oriented manner:



<?php

class konekcija{

private $servername;
private $username;
private $password;
private $dbname;
private $sql;

//Setter functions
public function setVal($par1, $par2, $par3, $par4, $par5){ //Setovanje vrednosti za upit.
$this->servername = $par1;
$this->username = $par2;
$this->password = $par3;
$this->dbname = $par4;
$this->sql = $par5;

}

//Getter functions
public function getServername() {
return $this->servername;
}

public function getUsername() {
return $this->username;
}

public function getPassword() {
return $this->password;
}

public function getDBname() {
return $this->dbname;
}

public function getSQL() {
return $this->sql;
}

//Function that executes query.
public function pristup($server_name, $user_name, $pass_word, $db_name, $sql_query){

$conn=new mysqli($server_name, $user_name, $pass_word, $db_name);

if($conn->connect_error){

die("Neuspela konekcija: ".$conn->connect_error);

}

$result = $conn->query($sql_query);

if ($result == TRUE){

echo "Uspela konekcija";

}
else{

echo "Neuspešno izvršavanje upita: " . $conn->error;

}

return $this->$result;

$conn->close();

}

}


$kon = new konekcija(); //Creation of an object.

//Setting values.
$kon -> setVal("localhost", "root", "", "test", "CREATE TABLE example(
id INT(6) UNSIGNED AUTO_INCREMENT PRIMARY KEY,
firstname VARCHAR(30) NOT NULL,
lastname VARCHAR(30) NOT NULL,
email VARCHAR(50),
reg_date TIMESTAMP
)");

//Getting values and inserting them into class method.
$kon -> pristup($kon->getServername(), $kon->getUsername(), $kon->getPassword(), $kon->getDBname(), $kon->getSQL());

?>


This works, and I'm asking if this code is any good. Is there a better/more established way of doing this?










share|improve this question
















bumped to the homepage by Community 2 days ago


This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.















  • You don't really need to assign so many methods to assigning things. You won't really ever retrieve those again, so dedicating methods to assigning them is not necessary, I don't think.
    – Rasclatt
    Dec 30 '17 at 6:01










  • @Rasclatt So one method to rule'em all?
    – Veljko Stefanovic
    Dec 30 '17 at 6:16










  • Post edited according to advice given by @Rasclatt .
    – Veljko Stefanovic
    Dec 30 '17 at 6:22












  • I am not sure if I am going to write a distinct answer, so just a few pointers. First of all, you shouldn't write some code, or use some paradigm only for sake of it, just because everyone else is doing it. Your new code should be better than old one. Do you see any benefit in your class? If so, what is it? Regarding the class itself, I've got an article, Your first database wrapper's childhood diseases, it is definitely worth to read
    – Your Common Sense
    Dec 30 '17 at 10:00










  • Also I wrote an answer to the similar question recently, you may find it useful: codereview.stackexchange.com/questions/183801/…
    – Your Common Sense
    Dec 30 '17 at 10:07















up vote
1
down vote

favorite












I'm starting to learn the object oriented aspect of PHP. In this regard I made a small "exercise" of sorts "translating" one of my PHP functions into OO mode of PHP. I'm asking if there are there any recommendations you can give me concerning my predicament.



Example of non-OO PHP:



<?php
function pristup($servername, $username, $password, $dbname, $sql){

$conn=new mysqli($servername, $username, $password, $dbname);

if($conn->connect_error){

die("Neuspela konekcija: ".$conn->connect_error);

}

$result = $conn->query($sql);
if ($result == TRUE) {
//echo "Uspela konekcija";
} else {
echo "Neuspešno izvršavanje upita: " . $conn->error;
}
return $result;

$conn->close();

}

?>


Example of this done in an object oriented manner:



<?php

class konekcija{

private $servername;
private $username;
private $password;
private $dbname;
private $sql;

//Setter functions
public function setVal($par1, $par2, $par3, $par4, $par5){ //Setovanje vrednosti za upit.
$this->servername = $par1;
$this->username = $par2;
$this->password = $par3;
$this->dbname = $par4;
$this->sql = $par5;

}

//Getter functions
public function getServername() {
return $this->servername;
}

public function getUsername() {
return $this->username;
}

public function getPassword() {
return $this->password;
}

public function getDBname() {
return $this->dbname;
}

public function getSQL() {
return $this->sql;
}

//Function that executes query.
public function pristup($server_name, $user_name, $pass_word, $db_name, $sql_query){

$conn=new mysqli($server_name, $user_name, $pass_word, $db_name);

if($conn->connect_error){

die("Neuspela konekcija: ".$conn->connect_error);

}

$result = $conn->query($sql_query);

if ($result == TRUE){

echo "Uspela konekcija";

}
else{

echo "Neuspešno izvršavanje upita: " . $conn->error;

}

return $this->$result;

$conn->close();

}

}


$kon = new konekcija(); //Creation of an object.

//Setting values.
$kon -> setVal("localhost", "root", "", "test", "CREATE TABLE example(
id INT(6) UNSIGNED AUTO_INCREMENT PRIMARY KEY,
firstname VARCHAR(30) NOT NULL,
lastname VARCHAR(30) NOT NULL,
email VARCHAR(50),
reg_date TIMESTAMP
)");

//Getting values and inserting them into class method.
$kon -> pristup($kon->getServername(), $kon->getUsername(), $kon->getPassword(), $kon->getDBname(), $kon->getSQL());

?>


This works, and I'm asking if this code is any good. Is there a better/more established way of doing this?










share|improve this question
















bumped to the homepage by Community 2 days ago


This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.















  • You don't really need to assign so many methods to assigning things. You won't really ever retrieve those again, so dedicating methods to assigning them is not necessary, I don't think.
    – Rasclatt
    Dec 30 '17 at 6:01










  • @Rasclatt So one method to rule'em all?
    – Veljko Stefanovic
    Dec 30 '17 at 6:16










  • Post edited according to advice given by @Rasclatt .
    – Veljko Stefanovic
    Dec 30 '17 at 6:22












  • I am not sure if I am going to write a distinct answer, so just a few pointers. First of all, you shouldn't write some code, or use some paradigm only for sake of it, just because everyone else is doing it. Your new code should be better than old one. Do you see any benefit in your class? If so, what is it? Regarding the class itself, I've got an article, Your first database wrapper's childhood diseases, it is definitely worth to read
    – Your Common Sense
    Dec 30 '17 at 10:00










  • Also I wrote an answer to the similar question recently, you may find it useful: codereview.stackexchange.com/questions/183801/…
    – Your Common Sense
    Dec 30 '17 at 10:07













up vote
1
down vote

favorite









up vote
1
down vote

favorite











I'm starting to learn the object oriented aspect of PHP. In this regard I made a small "exercise" of sorts "translating" one of my PHP functions into OO mode of PHP. I'm asking if there are there any recommendations you can give me concerning my predicament.



Example of non-OO PHP:



<?php
function pristup($servername, $username, $password, $dbname, $sql){

$conn=new mysqli($servername, $username, $password, $dbname);

if($conn->connect_error){

die("Neuspela konekcija: ".$conn->connect_error);

}

$result = $conn->query($sql);
if ($result == TRUE) {
//echo "Uspela konekcija";
} else {
echo "Neuspešno izvršavanje upita: " . $conn->error;
}
return $result;

$conn->close();

}

?>


Example of this done in an object oriented manner:



<?php

class konekcija{

private $servername;
private $username;
private $password;
private $dbname;
private $sql;

//Setter functions
public function setVal($par1, $par2, $par3, $par4, $par5){ //Setovanje vrednosti za upit.
$this->servername = $par1;
$this->username = $par2;
$this->password = $par3;
$this->dbname = $par4;
$this->sql = $par5;

}

//Getter functions
public function getServername() {
return $this->servername;
}

public function getUsername() {
return $this->username;
}

public function getPassword() {
return $this->password;
}

public function getDBname() {
return $this->dbname;
}

public function getSQL() {
return $this->sql;
}

//Function that executes query.
public function pristup($server_name, $user_name, $pass_word, $db_name, $sql_query){

$conn=new mysqli($server_name, $user_name, $pass_word, $db_name);

if($conn->connect_error){

die("Neuspela konekcija: ".$conn->connect_error);

}

$result = $conn->query($sql_query);

if ($result == TRUE){

echo "Uspela konekcija";

}
else{

echo "Neuspešno izvršavanje upita: " . $conn->error;

}

return $this->$result;

$conn->close();

}

}


$kon = new konekcija(); //Creation of an object.

//Setting values.
$kon -> setVal("localhost", "root", "", "test", "CREATE TABLE example(
id INT(6) UNSIGNED AUTO_INCREMENT PRIMARY KEY,
firstname VARCHAR(30) NOT NULL,
lastname VARCHAR(30) NOT NULL,
email VARCHAR(50),
reg_date TIMESTAMP
)");

//Getting values and inserting them into class method.
$kon -> pristup($kon->getServername(), $kon->getUsername(), $kon->getPassword(), $kon->getDBname(), $kon->getSQL());

?>


This works, and I'm asking if this code is any good. Is there a better/more established way of doing this?










share|improve this question















I'm starting to learn the object oriented aspect of PHP. In this regard I made a small "exercise" of sorts "translating" one of my PHP functions into OO mode of PHP. I'm asking if there are there any recommendations you can give me concerning my predicament.



Example of non-OO PHP:



<?php
function pristup($servername, $username, $password, $dbname, $sql){

$conn=new mysqli($servername, $username, $password, $dbname);

if($conn->connect_error){

die("Neuspela konekcija: ".$conn->connect_error);

}

$result = $conn->query($sql);
if ($result == TRUE) {
//echo "Uspela konekcija";
} else {
echo "Neuspešno izvršavanje upita: " . $conn->error;
}
return $result;

$conn->close();

}

?>


Example of this done in an object oriented manner:



<?php

class konekcija{

private $servername;
private $username;
private $password;
private $dbname;
private $sql;

//Setter functions
public function setVal($par1, $par2, $par3, $par4, $par5){ //Setovanje vrednosti za upit.
$this->servername = $par1;
$this->username = $par2;
$this->password = $par3;
$this->dbname = $par4;
$this->sql = $par5;

}

//Getter functions
public function getServername() {
return $this->servername;
}

public function getUsername() {
return $this->username;
}

public function getPassword() {
return $this->password;
}

public function getDBname() {
return $this->dbname;
}

public function getSQL() {
return $this->sql;
}

//Function that executes query.
public function pristup($server_name, $user_name, $pass_word, $db_name, $sql_query){

$conn=new mysqli($server_name, $user_name, $pass_word, $db_name);

if($conn->connect_error){

die("Neuspela konekcija: ".$conn->connect_error);

}

$result = $conn->query($sql_query);

if ($result == TRUE){

echo "Uspela konekcija";

}
else{

echo "Neuspešno izvršavanje upita: " . $conn->error;

}

return $this->$result;

$conn->close();

}

}


$kon = new konekcija(); //Creation of an object.

//Setting values.
$kon -> setVal("localhost", "root", "", "test", "CREATE TABLE example(
id INT(6) UNSIGNED AUTO_INCREMENT PRIMARY KEY,
firstname VARCHAR(30) NOT NULL,
lastname VARCHAR(30) NOT NULL,
email VARCHAR(50),
reg_date TIMESTAMP
)");

//Getting values and inserting them into class method.
$kon -> pristup($kon->getServername(), $kon->getUsername(), $kon->getPassword(), $kon->getDBname(), $kon->getSQL());

?>


This works, and I'm asking if this code is any good. Is there a better/more established way of doing this?







php object-oriented comparative-review mysqli






share|improve this question















share|improve this question













share|improve this question




share|improve this question








edited Feb 28 at 21:52









200_success

127k15148411




127k15148411










asked Dec 30 '17 at 4:06









Veljko Stefanovic

265




265





bumped to the homepage by Community 2 days ago


This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.







bumped to the homepage by Community 2 days ago


This question has answers that may be good or bad; the system has marked it active so that they can be reviewed.














  • You don't really need to assign so many methods to assigning things. You won't really ever retrieve those again, so dedicating methods to assigning them is not necessary, I don't think.
    – Rasclatt
    Dec 30 '17 at 6:01










  • @Rasclatt So one method to rule'em all?
    – Veljko Stefanovic
    Dec 30 '17 at 6:16










  • Post edited according to advice given by @Rasclatt .
    – Veljko Stefanovic
    Dec 30 '17 at 6:22












  • I am not sure if I am going to write a distinct answer, so just a few pointers. First of all, you shouldn't write some code, or use some paradigm only for sake of it, just because everyone else is doing it. Your new code should be better than old one. Do you see any benefit in your class? If so, what is it? Regarding the class itself, I've got an article, Your first database wrapper's childhood diseases, it is definitely worth to read
    – Your Common Sense
    Dec 30 '17 at 10:00










  • Also I wrote an answer to the similar question recently, you may find it useful: codereview.stackexchange.com/questions/183801/…
    – Your Common Sense
    Dec 30 '17 at 10:07


















  • You don't really need to assign so many methods to assigning things. You won't really ever retrieve those again, so dedicating methods to assigning them is not necessary, I don't think.
    – Rasclatt
    Dec 30 '17 at 6:01










  • @Rasclatt So one method to rule'em all?
    – Veljko Stefanovic
    Dec 30 '17 at 6:16










  • Post edited according to advice given by @Rasclatt .
    – Veljko Stefanovic
    Dec 30 '17 at 6:22












  • I am not sure if I am going to write a distinct answer, so just a few pointers. First of all, you shouldn't write some code, or use some paradigm only for sake of it, just because everyone else is doing it. Your new code should be better than old one. Do you see any benefit in your class? If so, what is it? Regarding the class itself, I've got an article, Your first database wrapper's childhood diseases, it is definitely worth to read
    – Your Common Sense
    Dec 30 '17 at 10:00










  • Also I wrote an answer to the similar question recently, you may find it useful: codereview.stackexchange.com/questions/183801/…
    – Your Common Sense
    Dec 30 '17 at 10:07
















You don't really need to assign so many methods to assigning things. You won't really ever retrieve those again, so dedicating methods to assigning them is not necessary, I don't think.
– Rasclatt
Dec 30 '17 at 6:01




You don't really need to assign so many methods to assigning things. You won't really ever retrieve those again, so dedicating methods to assigning them is not necessary, I don't think.
– Rasclatt
Dec 30 '17 at 6:01












@Rasclatt So one method to rule'em all?
– Veljko Stefanovic
Dec 30 '17 at 6:16




@Rasclatt So one method to rule'em all?
– Veljko Stefanovic
Dec 30 '17 at 6:16












Post edited according to advice given by @Rasclatt .
– Veljko Stefanovic
Dec 30 '17 at 6:22






Post edited according to advice given by @Rasclatt .
– Veljko Stefanovic
Dec 30 '17 at 6:22














I am not sure if I am going to write a distinct answer, so just a few pointers. First of all, you shouldn't write some code, or use some paradigm only for sake of it, just because everyone else is doing it. Your new code should be better than old one. Do you see any benefit in your class? If so, what is it? Regarding the class itself, I've got an article, Your first database wrapper's childhood diseases, it is definitely worth to read
– Your Common Sense
Dec 30 '17 at 10:00




I am not sure if I am going to write a distinct answer, so just a few pointers. First of all, you shouldn't write some code, or use some paradigm only for sake of it, just because everyone else is doing it. Your new code should be better than old one. Do you see any benefit in your class? If so, what is it? Regarding the class itself, I've got an article, Your first database wrapper's childhood diseases, it is definitely worth to read
– Your Common Sense
Dec 30 '17 at 10:00












Also I wrote an answer to the similar question recently, you may find it useful: codereview.stackexchange.com/questions/183801/…
– Your Common Sense
Dec 30 '17 at 10:07




Also I wrote an answer to the similar question recently, you may find it useful: codereview.stackexchange.com/questions/183801/…
– Your Common Sense
Dec 30 '17 at 10:07










1 Answer
1






active

oldest

votes

















up vote
0
down vote













I think you might be better served breaking what you have into a database class and a query-type class that will use the database class to accomplish queries and return rows, maybe something like:



/vendor/MyApp/Database/Model.php



<?php
namespace MyAppDatabase;
# You may want to implement an interface here or abstract class to allow for
# different connection types (looking to the future)
class Model
{
protected $conn,
$Log = false;
# I think you might want to pass a logging class to save server/connection errors
public function __construct(MyAppLoggingModel $Log)
{
$this->Log = $Log;
}

public function createConnection($host,$username,$password,$dbname)
{
$this->conn = new mysqli($host, $username, $password, $dbname);
if(!empty($this->conn->connect_error)) {
# Here is where you want to log potentially sensitive errors.
$this->Log->saveError($this->conn->connect_error);
# Just let the user know something has gone wrong, they don't need
# to know the error code and cryptic messages doled out by MySQL
throw new Exception('Database can not connect.');
}

return $this;
}

public function getConnection()
{
if($this->conn instanceof MySQLi)
return $this->conn;

throw new Exception('Database connection not yet set.');
}

public function closeConnection()
{
if($this->conn instanceof MySQLi)
$this->conn->close();
}
}


/vendor/MyApp/Query/Controller.php



<?php
namespace MyAppQuery;
# You can use the database model to create a contained connection
# You could extend this DB class to make this Query class, I am just going
# to use it instead though
use MyAppDatabaseModel as Connection;

class Controller
{
private $con,
$query,
$Log,
$Db;

public function __construct(MyAppLoggingModel $Log)
{
# You probably want to log failed queries, so use the same logging class
$this->Log = $Log;
# Pass that class to the connection
$this->Db = new Connection($this->Log);
# Create the connection here using the CONSTANTS in your config file
# Assign the connection internally to this class
$this->con = $this->Db->createConnection(DB_HOST,DB_USERNAME,DB_PASSWORD,DB_NAME)
->getConnection();
}
# You probably want to make a way to bind parameters
# Also note, I use PDO, so is copied from yours...
public function query($sql)
{
$this->query = $this->con->query($sql);
if($this->query != TRUE) {
# Same as connection, you need to know what really happened,
# but your user doesn't
$this->Log->saveError($conn->error);
throw new Exception('An error occurred.');
}

return $this;
}
# Here is where you would send back the rows (probably requires a while() here)
public function getResults()
{
return $this->query;
}
# You may want to be able to get the raw connection, who knows...
public function getConnection()
{
return $this->con;
}
}


You won't ever (or very rarely) have to retrieve the database credentials so you don't really need to dedicate a bunch of methods to assign those.



Depending on your app, you can do a contained engine without having to always add your db credentials to the main database class:



<?php
# I might do a config file that has this kind of info or an editable php
# array with the credentials in it. Something that can not be directly
# accessed by a user
define('DB_HOST','localhost');
define('DB_USERNAME','root');
define('DB_PASSWORD','');
define('DB_NAME','dbname');
# Create the logging class
$Logger = new MyAppLoggingModel();
# Create the query engine, add in a logging class to keep track of errors
# You don't want to show the user the actual errors, those are best kept
# for the administrator's eyes. The user just needs to know something is not
# working
# One note here, you would want to pass $qEngine to all classes that use
# the query engine / database connection, don't make more instances of this
$qEngine = new MyAppQueryController($Logger);
# Create a query and get the results
# This is to allow for writing queries and fetch rows (the getResults() method)
$results = $qEngine->query("SELECT * FROM `users`")->getResults();


Finally, you don't really want to echo anything except in your view so you can use a try to catch errors and print them in the view, not inside the data fetching methods. Depending on the error, die() might be a bit much. If you throw an Exception, depending on the severity of it, you can print a designed page that has your error printed to the page instead of the rather bleak look of a die('String of text here.').



Anyway, I am no hardcore expert, there are lots of books and articles regarding patterns and such that would be helpful to you. These are some things I have done in the past (and still do for the most part). Also note, I haven't tested this specifically, it's more to just give you some ideas.






share|improve this answer























  • A little review, if you let me. You are making a very common mistake, for some reason trying to handle a few random errors and leaving all other errors unhandled. That makes your code inconsistent and WET. What if, for example, a call to $this->con->query($sql); will result in the error? There is no code to handle it. And it shouldn't be. As well as there shouldn't be a code to handle an erroneous query. You should throw an useful exception instead, whereas logging and whatever 'An error occurred.' message processing should occur elsewhere, in the centralized manner, making your code shorter
    – Your Common Sense
    Dec 30 '17 at 10:06










  • Besides, your code creates a new database connection for the every object, which is a BIG no-no, it will result in a database server error "Too many connections". You are sending a Logger instance in the constructor but do not follow this pattern for the database class. is there any reason for that? Also, given the usage pattern of this class, I don't see why there is a getConnection method. Why cannot createConnection return it already?
    – Your Common Sense
    Dec 30 '17 at 10:10












  • @YourCommonSense Thanks for the review, if you allow me the opportunity for rebuttal. In the example I have stated to pass the query engine which initiates the connection inside it's constructor into proceeding classes and not to create it again so multiple connections should not be an issue unless that is not adhered to. Since the user is just exploring OOP, my example is really only demonstrating a one layer script to give the OP the idea of separating DB create from a query engine, I believe they are two different things. Extend would be a better plan to remove redundancy, I agree there.
    – Rasclatt
    Dec 30 '17 at 17:06










  • @YourCommonSense Also, I agree with you logging would be better served centralized to capture and separate logging from visual errors, however I am not demonstrating how to create a framework and am not trying to create a class matrix to cover every scenario, rather some concepts on dependency injection and throwing errors/logging rather than dying/echoing, etc. inside a Model. I also believe useful exceptions should not include MySQL warnings when a query failed straight from the DB. I believe the average user just needs to know something is wrong with the site, not a mysql error code/string.
    – Rasclatt
    Dec 30 '17 at 17:11










  • I short-read it about MVC paradigm and sort of understand it's benefits(code clarity and maintenance). But for my mind clarity sake i'll keep with solution posted in my original post on SO. At least till i have better grasp at MVC.
    – Veljko Stefanovic
    Dec 30 '17 at 19:35











Your Answer





StackExchange.ifUsing("editor", function () {
return StackExchange.using("mathjaxEditing", function () {
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
});
});
}, "mathjax-editing");

StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");

StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});

function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});


}
});














 

draft saved


draft discarded


















StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f183916%2foo-and-non-oo-usage-of-mysqli%23new-answer', 'question_page');
}
);

Post as a guest















Required, but never shown

























1 Answer
1






active

oldest

votes








1 Answer
1






active

oldest

votes









active

oldest

votes






active

oldest

votes








up vote
0
down vote













I think you might be better served breaking what you have into a database class and a query-type class that will use the database class to accomplish queries and return rows, maybe something like:



/vendor/MyApp/Database/Model.php



<?php
namespace MyAppDatabase;
# You may want to implement an interface here or abstract class to allow for
# different connection types (looking to the future)
class Model
{
protected $conn,
$Log = false;
# I think you might want to pass a logging class to save server/connection errors
public function __construct(MyAppLoggingModel $Log)
{
$this->Log = $Log;
}

public function createConnection($host,$username,$password,$dbname)
{
$this->conn = new mysqli($host, $username, $password, $dbname);
if(!empty($this->conn->connect_error)) {
# Here is where you want to log potentially sensitive errors.
$this->Log->saveError($this->conn->connect_error);
# Just let the user know something has gone wrong, they don't need
# to know the error code and cryptic messages doled out by MySQL
throw new Exception('Database can not connect.');
}

return $this;
}

public function getConnection()
{
if($this->conn instanceof MySQLi)
return $this->conn;

throw new Exception('Database connection not yet set.');
}

public function closeConnection()
{
if($this->conn instanceof MySQLi)
$this->conn->close();
}
}


/vendor/MyApp/Query/Controller.php



<?php
namespace MyAppQuery;
# You can use the database model to create a contained connection
# You could extend this DB class to make this Query class, I am just going
# to use it instead though
use MyAppDatabaseModel as Connection;

class Controller
{
private $con,
$query,
$Log,
$Db;

public function __construct(MyAppLoggingModel $Log)
{
# You probably want to log failed queries, so use the same logging class
$this->Log = $Log;
# Pass that class to the connection
$this->Db = new Connection($this->Log);
# Create the connection here using the CONSTANTS in your config file
# Assign the connection internally to this class
$this->con = $this->Db->createConnection(DB_HOST,DB_USERNAME,DB_PASSWORD,DB_NAME)
->getConnection();
}
# You probably want to make a way to bind parameters
# Also note, I use PDO, so is copied from yours...
public function query($sql)
{
$this->query = $this->con->query($sql);
if($this->query != TRUE) {
# Same as connection, you need to know what really happened,
# but your user doesn't
$this->Log->saveError($conn->error);
throw new Exception('An error occurred.');
}

return $this;
}
# Here is where you would send back the rows (probably requires a while() here)
public function getResults()
{
return $this->query;
}
# You may want to be able to get the raw connection, who knows...
public function getConnection()
{
return $this->con;
}
}


You won't ever (or very rarely) have to retrieve the database credentials so you don't really need to dedicate a bunch of methods to assign those.



Depending on your app, you can do a contained engine without having to always add your db credentials to the main database class:



<?php
# I might do a config file that has this kind of info or an editable php
# array with the credentials in it. Something that can not be directly
# accessed by a user
define('DB_HOST','localhost');
define('DB_USERNAME','root');
define('DB_PASSWORD','');
define('DB_NAME','dbname');
# Create the logging class
$Logger = new MyAppLoggingModel();
# Create the query engine, add in a logging class to keep track of errors
# You don't want to show the user the actual errors, those are best kept
# for the administrator's eyes. The user just needs to know something is not
# working
# One note here, you would want to pass $qEngine to all classes that use
# the query engine / database connection, don't make more instances of this
$qEngine = new MyAppQueryController($Logger);
# Create a query and get the results
# This is to allow for writing queries and fetch rows (the getResults() method)
$results = $qEngine->query("SELECT * FROM `users`")->getResults();


Finally, you don't really want to echo anything except in your view so you can use a try to catch errors and print them in the view, not inside the data fetching methods. Depending on the error, die() might be a bit much. If you throw an Exception, depending on the severity of it, you can print a designed page that has your error printed to the page instead of the rather bleak look of a die('String of text here.').



Anyway, I am no hardcore expert, there are lots of books and articles regarding patterns and such that would be helpful to you. These are some things I have done in the past (and still do for the most part). Also note, I haven't tested this specifically, it's more to just give you some ideas.






share|improve this answer























  • A little review, if you let me. You are making a very common mistake, for some reason trying to handle a few random errors and leaving all other errors unhandled. That makes your code inconsistent and WET. What if, for example, a call to $this->con->query($sql); will result in the error? There is no code to handle it. And it shouldn't be. As well as there shouldn't be a code to handle an erroneous query. You should throw an useful exception instead, whereas logging and whatever 'An error occurred.' message processing should occur elsewhere, in the centralized manner, making your code shorter
    – Your Common Sense
    Dec 30 '17 at 10:06










  • Besides, your code creates a new database connection for the every object, which is a BIG no-no, it will result in a database server error "Too many connections". You are sending a Logger instance in the constructor but do not follow this pattern for the database class. is there any reason for that? Also, given the usage pattern of this class, I don't see why there is a getConnection method. Why cannot createConnection return it already?
    – Your Common Sense
    Dec 30 '17 at 10:10












  • @YourCommonSense Thanks for the review, if you allow me the opportunity for rebuttal. In the example I have stated to pass the query engine which initiates the connection inside it's constructor into proceeding classes and not to create it again so multiple connections should not be an issue unless that is not adhered to. Since the user is just exploring OOP, my example is really only demonstrating a one layer script to give the OP the idea of separating DB create from a query engine, I believe they are two different things. Extend would be a better plan to remove redundancy, I agree there.
    – Rasclatt
    Dec 30 '17 at 17:06










  • @YourCommonSense Also, I agree with you logging would be better served centralized to capture and separate logging from visual errors, however I am not demonstrating how to create a framework and am not trying to create a class matrix to cover every scenario, rather some concepts on dependency injection and throwing errors/logging rather than dying/echoing, etc. inside a Model. I also believe useful exceptions should not include MySQL warnings when a query failed straight from the DB. I believe the average user just needs to know something is wrong with the site, not a mysql error code/string.
    – Rasclatt
    Dec 30 '17 at 17:11










  • I short-read it about MVC paradigm and sort of understand it's benefits(code clarity and maintenance). But for my mind clarity sake i'll keep with solution posted in my original post on SO. At least till i have better grasp at MVC.
    – Veljko Stefanovic
    Dec 30 '17 at 19:35















up vote
0
down vote













I think you might be better served breaking what you have into a database class and a query-type class that will use the database class to accomplish queries and return rows, maybe something like:



/vendor/MyApp/Database/Model.php



<?php
namespace MyAppDatabase;
# You may want to implement an interface here or abstract class to allow for
# different connection types (looking to the future)
class Model
{
protected $conn,
$Log = false;
# I think you might want to pass a logging class to save server/connection errors
public function __construct(MyAppLoggingModel $Log)
{
$this->Log = $Log;
}

public function createConnection($host,$username,$password,$dbname)
{
$this->conn = new mysqli($host, $username, $password, $dbname);
if(!empty($this->conn->connect_error)) {
# Here is where you want to log potentially sensitive errors.
$this->Log->saveError($this->conn->connect_error);
# Just let the user know something has gone wrong, they don't need
# to know the error code and cryptic messages doled out by MySQL
throw new Exception('Database can not connect.');
}

return $this;
}

public function getConnection()
{
if($this->conn instanceof MySQLi)
return $this->conn;

throw new Exception('Database connection not yet set.');
}

public function closeConnection()
{
if($this->conn instanceof MySQLi)
$this->conn->close();
}
}


/vendor/MyApp/Query/Controller.php



<?php
namespace MyAppQuery;
# You can use the database model to create a contained connection
# You could extend this DB class to make this Query class, I am just going
# to use it instead though
use MyAppDatabaseModel as Connection;

class Controller
{
private $con,
$query,
$Log,
$Db;

public function __construct(MyAppLoggingModel $Log)
{
# You probably want to log failed queries, so use the same logging class
$this->Log = $Log;
# Pass that class to the connection
$this->Db = new Connection($this->Log);
# Create the connection here using the CONSTANTS in your config file
# Assign the connection internally to this class
$this->con = $this->Db->createConnection(DB_HOST,DB_USERNAME,DB_PASSWORD,DB_NAME)
->getConnection();
}
# You probably want to make a way to bind parameters
# Also note, I use PDO, so is copied from yours...
public function query($sql)
{
$this->query = $this->con->query($sql);
if($this->query != TRUE) {
# Same as connection, you need to know what really happened,
# but your user doesn't
$this->Log->saveError($conn->error);
throw new Exception('An error occurred.');
}

return $this;
}
# Here is where you would send back the rows (probably requires a while() here)
public function getResults()
{
return $this->query;
}
# You may want to be able to get the raw connection, who knows...
public function getConnection()
{
return $this->con;
}
}


You won't ever (or very rarely) have to retrieve the database credentials so you don't really need to dedicate a bunch of methods to assign those.



Depending on your app, you can do a contained engine without having to always add your db credentials to the main database class:



<?php
# I might do a config file that has this kind of info or an editable php
# array with the credentials in it. Something that can not be directly
# accessed by a user
define('DB_HOST','localhost');
define('DB_USERNAME','root');
define('DB_PASSWORD','');
define('DB_NAME','dbname');
# Create the logging class
$Logger = new MyAppLoggingModel();
# Create the query engine, add in a logging class to keep track of errors
# You don't want to show the user the actual errors, those are best kept
# for the administrator's eyes. The user just needs to know something is not
# working
# One note here, you would want to pass $qEngine to all classes that use
# the query engine / database connection, don't make more instances of this
$qEngine = new MyAppQueryController($Logger);
# Create a query and get the results
# This is to allow for writing queries and fetch rows (the getResults() method)
$results = $qEngine->query("SELECT * FROM `users`")->getResults();


Finally, you don't really want to echo anything except in your view so you can use a try to catch errors and print them in the view, not inside the data fetching methods. Depending on the error, die() might be a bit much. If you throw an Exception, depending on the severity of it, you can print a designed page that has your error printed to the page instead of the rather bleak look of a die('String of text here.').



Anyway, I am no hardcore expert, there are lots of books and articles regarding patterns and such that would be helpful to you. These are some things I have done in the past (and still do for the most part). Also note, I haven't tested this specifically, it's more to just give you some ideas.






share|improve this answer























  • A little review, if you let me. You are making a very common mistake, for some reason trying to handle a few random errors and leaving all other errors unhandled. That makes your code inconsistent and WET. What if, for example, a call to $this->con->query($sql); will result in the error? There is no code to handle it. And it shouldn't be. As well as there shouldn't be a code to handle an erroneous query. You should throw an useful exception instead, whereas logging and whatever 'An error occurred.' message processing should occur elsewhere, in the centralized manner, making your code shorter
    – Your Common Sense
    Dec 30 '17 at 10:06










  • Besides, your code creates a new database connection for the every object, which is a BIG no-no, it will result in a database server error "Too many connections". You are sending a Logger instance in the constructor but do not follow this pattern for the database class. is there any reason for that? Also, given the usage pattern of this class, I don't see why there is a getConnection method. Why cannot createConnection return it already?
    – Your Common Sense
    Dec 30 '17 at 10:10












  • @YourCommonSense Thanks for the review, if you allow me the opportunity for rebuttal. In the example I have stated to pass the query engine which initiates the connection inside it's constructor into proceeding classes and not to create it again so multiple connections should not be an issue unless that is not adhered to. Since the user is just exploring OOP, my example is really only demonstrating a one layer script to give the OP the idea of separating DB create from a query engine, I believe they are two different things. Extend would be a better plan to remove redundancy, I agree there.
    – Rasclatt
    Dec 30 '17 at 17:06










  • @YourCommonSense Also, I agree with you logging would be better served centralized to capture and separate logging from visual errors, however I am not demonstrating how to create a framework and am not trying to create a class matrix to cover every scenario, rather some concepts on dependency injection and throwing errors/logging rather than dying/echoing, etc. inside a Model. I also believe useful exceptions should not include MySQL warnings when a query failed straight from the DB. I believe the average user just needs to know something is wrong with the site, not a mysql error code/string.
    – Rasclatt
    Dec 30 '17 at 17:11










  • I short-read it about MVC paradigm and sort of understand it's benefits(code clarity and maintenance). But for my mind clarity sake i'll keep with solution posted in my original post on SO. At least till i have better grasp at MVC.
    – Veljko Stefanovic
    Dec 30 '17 at 19:35













up vote
0
down vote










up vote
0
down vote









I think you might be better served breaking what you have into a database class and a query-type class that will use the database class to accomplish queries and return rows, maybe something like:



/vendor/MyApp/Database/Model.php



<?php
namespace MyAppDatabase;
# You may want to implement an interface here or abstract class to allow for
# different connection types (looking to the future)
class Model
{
protected $conn,
$Log = false;
# I think you might want to pass a logging class to save server/connection errors
public function __construct(MyAppLoggingModel $Log)
{
$this->Log = $Log;
}

public function createConnection($host,$username,$password,$dbname)
{
$this->conn = new mysqli($host, $username, $password, $dbname);
if(!empty($this->conn->connect_error)) {
# Here is where you want to log potentially sensitive errors.
$this->Log->saveError($this->conn->connect_error);
# Just let the user know something has gone wrong, they don't need
# to know the error code and cryptic messages doled out by MySQL
throw new Exception('Database can not connect.');
}

return $this;
}

public function getConnection()
{
if($this->conn instanceof MySQLi)
return $this->conn;

throw new Exception('Database connection not yet set.');
}

public function closeConnection()
{
if($this->conn instanceof MySQLi)
$this->conn->close();
}
}


/vendor/MyApp/Query/Controller.php



<?php
namespace MyAppQuery;
# You can use the database model to create a contained connection
# You could extend this DB class to make this Query class, I am just going
# to use it instead though
use MyAppDatabaseModel as Connection;

class Controller
{
private $con,
$query,
$Log,
$Db;

public function __construct(MyAppLoggingModel $Log)
{
# You probably want to log failed queries, so use the same logging class
$this->Log = $Log;
# Pass that class to the connection
$this->Db = new Connection($this->Log);
# Create the connection here using the CONSTANTS in your config file
# Assign the connection internally to this class
$this->con = $this->Db->createConnection(DB_HOST,DB_USERNAME,DB_PASSWORD,DB_NAME)
->getConnection();
}
# You probably want to make a way to bind parameters
# Also note, I use PDO, so is copied from yours...
public function query($sql)
{
$this->query = $this->con->query($sql);
if($this->query != TRUE) {
# Same as connection, you need to know what really happened,
# but your user doesn't
$this->Log->saveError($conn->error);
throw new Exception('An error occurred.');
}

return $this;
}
# Here is where you would send back the rows (probably requires a while() here)
public function getResults()
{
return $this->query;
}
# You may want to be able to get the raw connection, who knows...
public function getConnection()
{
return $this->con;
}
}


You won't ever (or very rarely) have to retrieve the database credentials so you don't really need to dedicate a bunch of methods to assign those.



Depending on your app, you can do a contained engine without having to always add your db credentials to the main database class:



<?php
# I might do a config file that has this kind of info or an editable php
# array with the credentials in it. Something that can not be directly
# accessed by a user
define('DB_HOST','localhost');
define('DB_USERNAME','root');
define('DB_PASSWORD','');
define('DB_NAME','dbname');
# Create the logging class
$Logger = new MyAppLoggingModel();
# Create the query engine, add in a logging class to keep track of errors
# You don't want to show the user the actual errors, those are best kept
# for the administrator's eyes. The user just needs to know something is not
# working
# One note here, you would want to pass $qEngine to all classes that use
# the query engine / database connection, don't make more instances of this
$qEngine = new MyAppQueryController($Logger);
# Create a query and get the results
# This is to allow for writing queries and fetch rows (the getResults() method)
$results = $qEngine->query("SELECT * FROM `users`")->getResults();


Finally, you don't really want to echo anything except in your view so you can use a try to catch errors and print them in the view, not inside the data fetching methods. Depending on the error, die() might be a bit much. If you throw an Exception, depending on the severity of it, you can print a designed page that has your error printed to the page instead of the rather bleak look of a die('String of text here.').



Anyway, I am no hardcore expert, there are lots of books and articles regarding patterns and such that would be helpful to you. These are some things I have done in the past (and still do for the most part). Also note, I haven't tested this specifically, it's more to just give you some ideas.






share|improve this answer














I think you might be better served breaking what you have into a database class and a query-type class that will use the database class to accomplish queries and return rows, maybe something like:



/vendor/MyApp/Database/Model.php



<?php
namespace MyAppDatabase;
# You may want to implement an interface here or abstract class to allow for
# different connection types (looking to the future)
class Model
{
protected $conn,
$Log = false;
# I think you might want to pass a logging class to save server/connection errors
public function __construct(MyAppLoggingModel $Log)
{
$this->Log = $Log;
}

public function createConnection($host,$username,$password,$dbname)
{
$this->conn = new mysqli($host, $username, $password, $dbname);
if(!empty($this->conn->connect_error)) {
# Here is where you want to log potentially sensitive errors.
$this->Log->saveError($this->conn->connect_error);
# Just let the user know something has gone wrong, they don't need
# to know the error code and cryptic messages doled out by MySQL
throw new Exception('Database can not connect.');
}

return $this;
}

public function getConnection()
{
if($this->conn instanceof MySQLi)
return $this->conn;

throw new Exception('Database connection not yet set.');
}

public function closeConnection()
{
if($this->conn instanceof MySQLi)
$this->conn->close();
}
}


/vendor/MyApp/Query/Controller.php



<?php
namespace MyAppQuery;
# You can use the database model to create a contained connection
# You could extend this DB class to make this Query class, I am just going
# to use it instead though
use MyAppDatabaseModel as Connection;

class Controller
{
private $con,
$query,
$Log,
$Db;

public function __construct(MyAppLoggingModel $Log)
{
# You probably want to log failed queries, so use the same logging class
$this->Log = $Log;
# Pass that class to the connection
$this->Db = new Connection($this->Log);
# Create the connection here using the CONSTANTS in your config file
# Assign the connection internally to this class
$this->con = $this->Db->createConnection(DB_HOST,DB_USERNAME,DB_PASSWORD,DB_NAME)
->getConnection();
}
# You probably want to make a way to bind parameters
# Also note, I use PDO, so is copied from yours...
public function query($sql)
{
$this->query = $this->con->query($sql);
if($this->query != TRUE) {
# Same as connection, you need to know what really happened,
# but your user doesn't
$this->Log->saveError($conn->error);
throw new Exception('An error occurred.');
}

return $this;
}
# Here is where you would send back the rows (probably requires a while() here)
public function getResults()
{
return $this->query;
}
# You may want to be able to get the raw connection, who knows...
public function getConnection()
{
return $this->con;
}
}


You won't ever (or very rarely) have to retrieve the database credentials so you don't really need to dedicate a bunch of methods to assign those.



Depending on your app, you can do a contained engine without having to always add your db credentials to the main database class:



<?php
# I might do a config file that has this kind of info or an editable php
# array with the credentials in it. Something that can not be directly
# accessed by a user
define('DB_HOST','localhost');
define('DB_USERNAME','root');
define('DB_PASSWORD','');
define('DB_NAME','dbname');
# Create the logging class
$Logger = new MyAppLoggingModel();
# Create the query engine, add in a logging class to keep track of errors
# You don't want to show the user the actual errors, those are best kept
# for the administrator's eyes. The user just needs to know something is not
# working
# One note here, you would want to pass $qEngine to all classes that use
# the query engine / database connection, don't make more instances of this
$qEngine = new MyAppQueryController($Logger);
# Create a query and get the results
# This is to allow for writing queries and fetch rows (the getResults() method)
$results = $qEngine->query("SELECT * FROM `users`")->getResults();


Finally, you don't really want to echo anything except in your view so you can use a try to catch errors and print them in the view, not inside the data fetching methods. Depending on the error, die() might be a bit much. If you throw an Exception, depending on the severity of it, you can print a designed page that has your error printed to the page instead of the rather bleak look of a die('String of text here.').



Anyway, I am no hardcore expert, there are lots of books and articles regarding patterns and such that would be helpful to you. These are some things I have done in the past (and still do for the most part). Also note, I haven't tested this specifically, it's more to just give you some ideas.







share|improve this answer














share|improve this answer



share|improve this answer








edited Dec 30 '17 at 7:23

























answered Dec 30 '17 at 7:16









Rasclatt

1093




1093












  • A little review, if you let me. You are making a very common mistake, for some reason trying to handle a few random errors and leaving all other errors unhandled. That makes your code inconsistent and WET. What if, for example, a call to $this->con->query($sql); will result in the error? There is no code to handle it. And it shouldn't be. As well as there shouldn't be a code to handle an erroneous query. You should throw an useful exception instead, whereas logging and whatever 'An error occurred.' message processing should occur elsewhere, in the centralized manner, making your code shorter
    – Your Common Sense
    Dec 30 '17 at 10:06










  • Besides, your code creates a new database connection for the every object, which is a BIG no-no, it will result in a database server error "Too many connections". You are sending a Logger instance in the constructor but do not follow this pattern for the database class. is there any reason for that? Also, given the usage pattern of this class, I don't see why there is a getConnection method. Why cannot createConnection return it already?
    – Your Common Sense
    Dec 30 '17 at 10:10












  • @YourCommonSense Thanks for the review, if you allow me the opportunity for rebuttal. In the example I have stated to pass the query engine which initiates the connection inside it's constructor into proceeding classes and not to create it again so multiple connections should not be an issue unless that is not adhered to. Since the user is just exploring OOP, my example is really only demonstrating a one layer script to give the OP the idea of separating DB create from a query engine, I believe they are two different things. Extend would be a better plan to remove redundancy, I agree there.
    – Rasclatt
    Dec 30 '17 at 17:06










  • @YourCommonSense Also, I agree with you logging would be better served centralized to capture and separate logging from visual errors, however I am not demonstrating how to create a framework and am not trying to create a class matrix to cover every scenario, rather some concepts on dependency injection and throwing errors/logging rather than dying/echoing, etc. inside a Model. I also believe useful exceptions should not include MySQL warnings when a query failed straight from the DB. I believe the average user just needs to know something is wrong with the site, not a mysql error code/string.
    – Rasclatt
    Dec 30 '17 at 17:11










  • I short-read it about MVC paradigm and sort of understand it's benefits(code clarity and maintenance). But for my mind clarity sake i'll keep with solution posted in my original post on SO. At least till i have better grasp at MVC.
    – Veljko Stefanovic
    Dec 30 '17 at 19:35


















  • A little review, if you let me. You are making a very common mistake, for some reason trying to handle a few random errors and leaving all other errors unhandled. That makes your code inconsistent and WET. What if, for example, a call to $this->con->query($sql); will result in the error? There is no code to handle it. And it shouldn't be. As well as there shouldn't be a code to handle an erroneous query. You should throw an useful exception instead, whereas logging and whatever 'An error occurred.' message processing should occur elsewhere, in the centralized manner, making your code shorter
    – Your Common Sense
    Dec 30 '17 at 10:06










  • Besides, your code creates a new database connection for the every object, which is a BIG no-no, it will result in a database server error "Too many connections". You are sending a Logger instance in the constructor but do not follow this pattern for the database class. is there any reason for that? Also, given the usage pattern of this class, I don't see why there is a getConnection method. Why cannot createConnection return it already?
    – Your Common Sense
    Dec 30 '17 at 10:10












  • @YourCommonSense Thanks for the review, if you allow me the opportunity for rebuttal. In the example I have stated to pass the query engine which initiates the connection inside it's constructor into proceeding classes and not to create it again so multiple connections should not be an issue unless that is not adhered to. Since the user is just exploring OOP, my example is really only demonstrating a one layer script to give the OP the idea of separating DB create from a query engine, I believe they are two different things. Extend would be a better plan to remove redundancy, I agree there.
    – Rasclatt
    Dec 30 '17 at 17:06










  • @YourCommonSense Also, I agree with you logging would be better served centralized to capture and separate logging from visual errors, however I am not demonstrating how to create a framework and am not trying to create a class matrix to cover every scenario, rather some concepts on dependency injection and throwing errors/logging rather than dying/echoing, etc. inside a Model. I also believe useful exceptions should not include MySQL warnings when a query failed straight from the DB. I believe the average user just needs to know something is wrong with the site, not a mysql error code/string.
    – Rasclatt
    Dec 30 '17 at 17:11










  • I short-read it about MVC paradigm and sort of understand it's benefits(code clarity and maintenance). But for my mind clarity sake i'll keep with solution posted in my original post on SO. At least till i have better grasp at MVC.
    – Veljko Stefanovic
    Dec 30 '17 at 19:35
















A little review, if you let me. You are making a very common mistake, for some reason trying to handle a few random errors and leaving all other errors unhandled. That makes your code inconsistent and WET. What if, for example, a call to $this->con->query($sql); will result in the error? There is no code to handle it. And it shouldn't be. As well as there shouldn't be a code to handle an erroneous query. You should throw an useful exception instead, whereas logging and whatever 'An error occurred.' message processing should occur elsewhere, in the centralized manner, making your code shorter
– Your Common Sense
Dec 30 '17 at 10:06




A little review, if you let me. You are making a very common mistake, for some reason trying to handle a few random errors and leaving all other errors unhandled. That makes your code inconsistent and WET. What if, for example, a call to $this->con->query($sql); will result in the error? There is no code to handle it. And it shouldn't be. As well as there shouldn't be a code to handle an erroneous query. You should throw an useful exception instead, whereas logging and whatever 'An error occurred.' message processing should occur elsewhere, in the centralized manner, making your code shorter
– Your Common Sense
Dec 30 '17 at 10:06












Besides, your code creates a new database connection for the every object, which is a BIG no-no, it will result in a database server error "Too many connections". You are sending a Logger instance in the constructor but do not follow this pattern for the database class. is there any reason for that? Also, given the usage pattern of this class, I don't see why there is a getConnection method. Why cannot createConnection return it already?
– Your Common Sense
Dec 30 '17 at 10:10






Besides, your code creates a new database connection for the every object, which is a BIG no-no, it will result in a database server error "Too many connections". You are sending a Logger instance in the constructor but do not follow this pattern for the database class. is there any reason for that? Also, given the usage pattern of this class, I don't see why there is a getConnection method. Why cannot createConnection return it already?
– Your Common Sense
Dec 30 '17 at 10:10














@YourCommonSense Thanks for the review, if you allow me the opportunity for rebuttal. In the example I have stated to pass the query engine which initiates the connection inside it's constructor into proceeding classes and not to create it again so multiple connections should not be an issue unless that is not adhered to. Since the user is just exploring OOP, my example is really only demonstrating a one layer script to give the OP the idea of separating DB create from a query engine, I believe they are two different things. Extend would be a better plan to remove redundancy, I agree there.
– Rasclatt
Dec 30 '17 at 17:06




@YourCommonSense Thanks for the review, if you allow me the opportunity for rebuttal. In the example I have stated to pass the query engine which initiates the connection inside it's constructor into proceeding classes and not to create it again so multiple connections should not be an issue unless that is not adhered to. Since the user is just exploring OOP, my example is really only demonstrating a one layer script to give the OP the idea of separating DB create from a query engine, I believe they are two different things. Extend would be a better plan to remove redundancy, I agree there.
– Rasclatt
Dec 30 '17 at 17:06












@YourCommonSense Also, I agree with you logging would be better served centralized to capture and separate logging from visual errors, however I am not demonstrating how to create a framework and am not trying to create a class matrix to cover every scenario, rather some concepts on dependency injection and throwing errors/logging rather than dying/echoing, etc. inside a Model. I also believe useful exceptions should not include MySQL warnings when a query failed straight from the DB. I believe the average user just needs to know something is wrong with the site, not a mysql error code/string.
– Rasclatt
Dec 30 '17 at 17:11




@YourCommonSense Also, I agree with you logging would be better served centralized to capture and separate logging from visual errors, however I am not demonstrating how to create a framework and am not trying to create a class matrix to cover every scenario, rather some concepts on dependency injection and throwing errors/logging rather than dying/echoing, etc. inside a Model. I also believe useful exceptions should not include MySQL warnings when a query failed straight from the DB. I believe the average user just needs to know something is wrong with the site, not a mysql error code/string.
– Rasclatt
Dec 30 '17 at 17:11












I short-read it about MVC paradigm and sort of understand it's benefits(code clarity and maintenance). But for my mind clarity sake i'll keep with solution posted in my original post on SO. At least till i have better grasp at MVC.
– Veljko Stefanovic
Dec 30 '17 at 19:35




I short-read it about MVC paradigm and sort of understand it's benefits(code clarity and maintenance). But for my mind clarity sake i'll keep with solution posted in my original post on SO. At least till i have better grasp at MVC.
– Veljko Stefanovic
Dec 30 '17 at 19:35


















 

draft saved


draft discarded



















































 


draft saved


draft discarded














StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f183916%2foo-and-non-oo-usage-of-mysqli%23new-answer', 'question_page');
}
);

Post as a guest















Required, but never shown





















































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown

































Required, but never shown














Required, but never shown












Required, but never shown







Required, but never shown







Popular posts from this blog

Ellipse (mathématiques)

Quarter-circle Tiles

Mont Emei