3
\$\begingroup\$

I have this class whose code I pasted in full. The CredentialsManager class is not shown here as all it does is return the DB connection variables.

Can be improved or is this is OK as I've written it? Namely, I'm interested if it's OK to have a private variable mysqli populate inside the function openConnection() or if I should have done that in the constructor.

I'm pretty confident it would be a bad thing to have the same code for opening a connection in each function of the class, which is why I've put the connection opening code in a function itself.

<?php
class Logger
{
    // private variables
    private $_host;
    private $_database;
    private $_username;
    private $_password;
    private $_mysqli;   

    // public functions
    public function __construct()
    {
        $credientials = CredentialsManager::GetCredentials();
        $this->_host = $credientials['host'];
        $this->_database = $credientials['database'];
        $this->_username = $credientials['user'];
        $this->_password = $credientials['pass'];        
    }

    public function RecordLog($log){
        if ($this->openConnection()){                                   
            if (!($stmt = $this->_mysqli->prepare("INSERT INTO logz (log) VALUES (?)"))) {
                echo "Prepare failed: (" . $this->_mysqli->errno . ") " . $this->_mysqli->error;
            }

            if (! $stmt->bind_param("s", $log)){
                echo "Binding failed: (". $stmt->errno .") " . $stmt->error;
            }

            if (! $stmt->execute() ){
                echo "Execute failed: " . $stmt->error;
            }

            if ($stmt->affected_rows > 0)
                return true;
            else
                return false;
        }
        else
            return false;
    }


    // private functions
    private function openConnection()
    {
        $this->_mysqli = new mysqli($this->_host, $this->_username, $this->_password, $this->_database);
        if ($this->_mysqli->connect_errno) {
            echo "Failed to connect to MySQL: " . $this->_mysqli->connect_error;
            return false;
        }       

        return true;
    }
}
?>
\$\endgroup\$

migrated from stackoverflow.com Oct 23 '12 at 11:02

This question came from our site for professional and enthusiast programmers.

  • \$\begingroup\$ That's not how you spell credentials? Anyways, I'm looking at the code now, will post an answer after I go through it. \$\endgroup\$ – jsanc623 Oct 23 '12 at 16:09
5
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Excellent walk through of the code. I learned something too :) \$\endgroup\$ – Jeff Vanzella Oct 23 '12 at 21:53
  • \$\begingroup\$ You gave me quite a lot to study on, but thank you for that! \$\endgroup\$ – Nikola Oct 24 '12 at 7:22
  • \$\begingroup\$ otherwise PHP will terminate execution - that's not my experience, it will emit a notice though. \$\endgroup\$ – Jack Oct 24 '12 at 13:41
  • \$\begingroup\$ @Jack: Then you probably don't have your error reporting turned on high enough. Though that shouldn't make a difference. Directly from the PHP documentation "Failing to satisfy the type hint results in a catchable fatal error." \$\endgroup\$ – mseancole Oct 24 '12 at 14:07
2
\$\begingroup\$

"I'm pretty confident it would be a bad thing to have the same code for opening a connection in each function of the class, that's why I've put the connection opening code in a function itself."

  • That would be correct when possible, practice DRY: Don't Repeat Yourself

"Namely, I'm interested if it's OK to have a private variable mysqli populate inside the function openConnection() or should I have done that in the constructor?"

  • Nope, its okay the way you did it, but you can call openConnection() in the construct instead of in each function that uses it.

    // private variables
    # You don't need the comment above...it's not a functional comment
    private $_host;
    private $_database;
    private $_username;
    private $_password;
    private $_mysqli;   
    
    // public functions
    # Again, not a good comment. 
    public function __construct(){
        $credentials = CredentialsManager::GetCredentials();
        $this->_host = $credentials['host'];
        $this->_database = $credentials['database'];
        $this->_username = $credentials['user'];
        $this->_password = $credentials['pass']; 
        if (!$this->openConnection())
            echo "Connection failed.";
    }
    
    /**
     * How come there's no comment here? Add a function description here
     * 
     * @return true or false if....
     */
    public function RecordLog($log){
        if (!( $stmt = $this->_mysqli->prepare("INSERT INTO logz (log) VALUES (?)") )) {
            echo "Prepare failed: (" . $this->_mysqli->errno . ") " . $this->_mysqli->error;
        }
    
        if (! $stmt->bind_param("s", $log)){
            echo "Binding failed: (". $stmt->errno .") " . $stmt->error;
        }
    
        if (! $stmt->execute() )
            echo "Execute failed: " . $stmt->error;
        }
    
        if ($stmt->affected_rows > 0)
            return true;
        else
            return false;
    }
    
    
    // private functions
    # Comment above not needed
    /**
     * Let's make a useful descriptive comment here
     * 
     * @return false if mysqli fails, true if connection is successful 
     */ 
    private function openConnection(){
        $this->_mysqli = new mysqli($this->_host, $this->_username, $this->_password, $this->_database);
        if ($this->_mysqli->connect_errno) {
            echo "Failed to connect to MySQL: " . $this->_mysqli->connect_error;
            return false;
        }       
    
        return true;
    }
    

    }

Code looks good to me otherwise. Wait until others weigh in though.

\$\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.