The first suggestion I would like to make is for you to become familiar with Inversion of Control (IoC) and Dependency Injection (DI). These two principles state that dependencies should not be hardcoded into your classes, but should be "injected" into them. This ensures that your class does not become dependent upon a single entity and can thus be extended. This is most commonly combined with type hinting.
Type hinting ensures that a parameter is of the proper type, otherwise PHP will terminate execution of the script with an error. You can use the class you are expecting, or you can instead use a parent class or interface so that any similar class can be used, thus making it more extensible. I will demonstrate using the former as I don't know if there is a parent class or interface to use.
public function __construct( CredentialsManager $credentials ) {
//etc...
}
//usage
$log = new Logger( CredentialsManager::GetCredentials() );
You should also become aware of the Arrow Anti-Pattern. Through it you learn that unnecessarily indented code is bad. To illustrate this they usually show code that comes to peaks to form an arrow. The easiest way to refactor your code to avoid this anti-pattern is to return early. For example, if we reverse your first if statement in your RecordLog() method we can return early thus making the else statement unnecessary and removing an entire level of indentation from that method.
if( ! $this->openConnection() ) {
return FALSE;
}
//etc...
Speaking of that else statement. PHP inherently requires braces, otherwise you would not be forced to add them after your statements reached more than one line. Adding them helps ensure no mistakes are made and keeps your code consistent. I would strongly recommend adding braces, even if you think they are unnecessary.
else {
return FALSE;
}
Always avoid assigning variables in statements. There are quite a few reasons for this:
- They are extremely difficult to debug. I actually missed this the first time through and thought you were using globals.
- They decrease legibility. Usually they increase line length and make statements more complex.
- There is usually no way for the reader, or your IDE, to determine if this was done accidentally. Therefore it is fairly easy to accidentally assign a value to a variable when you actually meant to compare it.
There are exceptions to this rule, such as when setting up an iterator, but typically this should be avoided.
//common
for( $i = 0; $i < $count; $i++ ) {
//not as common, but fine
while( ( $buffer = fgets( $handle, 4096 ) ) !== false ) {
//as it should be
$stmt = $this->_mysqli->prepare( 'INSERT INTO logz (log) VALUES (?)' );
if( ! $stmt ) {
When you want a boolean return value that reflects a boolean state of what you are comparing, you can simply return the comparison and skip the unnecessary if/else structure.
return $stmt->affected_rows > 0;
Your RecordLog() method violates the Single Responsibility Principle. This principle states that your methods should do one thing and delegate everything else. Instead you have this method doing everything. A better way to rewrite this is to call the logger whenever an error is encountered and pass that error to it. Instead you are calling the logger to perform the functionality that might produce errors in order to catch them. This is backwards.
if( ! $this->performAction() ) {
$this->RecordLog( $error );
}
Expanding upon this concept, and those mentioned previously, our classes should be equally singular. It may not be as obvious, but your logger should really only be concerned with logging the errors, not with the how of it. If you instead inject the database into this class, then that database can change without needing to modify the logger. So you should be able to go from using a MySQLi database to PDO or any other database without needing to modify this class. Though this is more advanced and I wouldn't suggest trying to tackle this until you are more comfortable with the rest.
$log = new Logger( $mysqli );
$log = new Logger( $pdo );
The way you are currently connecting to your database restarts the connection every time RecordLog() is called. This is bad. You only need one connection per instance of the class. Creating it in the constructor ensures this. In fact, since you are not reusing any of those connection properties, you could probably get away with openConnection() BEING the constructor. If you just directly apply the credentials then declaring them as properties will become unnecesary.
$this->_mysqli = new mysqli(
$credientials[ 'host' ],
$credientials[ 'user' ],
$credientials[ 'pass' ],
$credientials[ 'database' ]
);
If you take my advice about that RecordLog() method, then you can "log" the connection error from the constructor as well, thus making your code more uniform.
if( $this->_mysqli->connect_errno ) {
$this->RecordLog(
"Failed to connect to MySQL: "
. $this->_mysqli->connect_error
);
}
So, something you should begin to notice from all of these suggestions is that your class now doesn't do much. You have a constructor and a logger. The constructor is a wrapper for the MySQLi class' constructor and as such can be abstracted out. This leaves just the logger method, and if we look at what this is doing, then we can also determine that this is unnecessary. Assuming you are planning on modifying this functionality to do more than just echo the error, then that leaves just the one method. This is not enough to justify a class. A function yes, but not a class. I would say forgo the class and just use PHP's MySQLi class directly. Extending, or in this case, pseudo extending, a class is unnecessary unless you are planning on doing something DRASTIC with that extension. Just creating a connection is not enough. However, it is a good start to trying to understand OOP concepts.
