Logger class for logging connections

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.

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

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 _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 sh

StackExchange Code Review Q#17842, answer score: 5