Storing, fetching, and deleting battle simulation results

Question

my connection code:

    <?php
try {
    $pdo = new PDO("mysql:host=localhost;dbname=game;
    charset=utf8", "root");
}
catch(PDOException $e){
    echo $e->getMessage();
}
?>

my class code:

    <?php
include './database/config.php';

class DB {
    public function dataWarrior($battle,$name,$type){
        global $pdo;
        try {
            $stmt = $pdo->prepare("SELECT battle_id,warrior_name,warrior_type,health,attack,defense,speed,evade
            FROM battle WHERE battle_id = :battle AND warrior_name = :name AND warrior_type = :type LIMIT 1");

            $stmt->execute(array(":battle"=>$battle, ":name"=>$name, ":type"=>$type));

            return $stmt;
        }
        catch(PDOException $e){
            echo $e->getMessage();
        }
    }
    public function checkBattleID($battle){
        global $pdo;
        try {
            $stmt = $pdo->prepare("SELECT battle_id FROM battle WHERE battle_id=:battle LIMIT 1");

            $stmt->execute(array(":battle"=>$battle));

            if($stmt->rowCount()>0){
                return false;
            }
            else {
                return true;
            }
        }
        catch(PDOException $e){
            echo $e->getMessage();
        }
    }
    public function createBattle($warrior){
        global $pdo;
        try {
            $stmt = $pdo->prepare('INSERT INTO battle(battle_id,warrior_name,warrior_type,
            health,attack,defense,speed,evade) VALUES(:battle,:name,:type,:health,:attack,:defense,:speed,:evade)');

            $stmt->execute(array(':battle'=>$warrior['battle'], ':name'=>$warrior['name'], 
            ':type'=>$warrior['type'], ':health'=>$warrior['health'], ':attack'=>$warrior['attack'],
            ':defense'=>$warrior['defense'], ':speed'=>$warrior['speed'], ':evade'=>$warrior['evade']));            
        }
        catch(PDOException $e){
            echo $e->getMessage();  
        }   
    }
    public function deleteBattle($battle){
        global $pdo;
        try {
            $stmt = $pdo->prepare('DELETE FROM battle WHERE battle_id = :battle');
            $stmt->execute(array(':battle'=>$battle));
        }
        catch(PDOException $e){
            echo $e->getMessage();  
        }
    }
}
?>

Answer 1

Stay away from globals. You will never need them. Never. Especially don't use globals in a class where they are completely unnecessary. They are an older technology that is full of security holes. Because $pdo is in the global scope, if I were of malicious intent I could hack into your database simply by using the already authenticated variable you gave me. This also makes it difficult to figure out where your variables are coming from. Without any kind of context it could become very easy to lose track of it. Instead of separating your connection logic from your class, just put it in the class constructor and use class properties to simulate a "global" state. Unlike Peter's example, this keeps the connection logic associated with the class instead of injecting it. The problem with dependency injection, in this case, is that it makes no sense unless you are reusing that same connection in another class at the same time. With the limited knowledge of this system I cannot know one way or the other and so DI seems a bit abstract.

private $pdo;//declare class property first

public function __construct() {
    try {
        $this->pdo = new PDO(//define class property
            'mysql:host=localhost;
            dbname=game;
            charset=utf8',

            'root'
        );
    } catch( PDOException $e ) {
        echo $e->getMessage();
    }
}

//to use class property
$this->pdo->prepare(/*etc...*/);

The reason we are using private in the above scenario is the same reason we aren't using globals. If the property were public it could be accessed outside of the class, which is not something we want to provide access to.

You have three more variables that could be made into properties. Properties are an essential part of OOP, they encapsulate the data needed to make the class work. They do this by allowing different methods to communicate with each other by storing and accessing properties within their class instead of returning and parsing parameters. Not to say methods can't have parameters, only that most become unnecessary. Without properties, you might as well be using regular functions. So, how do we apply this? The parameters you pass to your dataWarrior() method seem to be reused at different points in the class. The $battle parameter is used to determine the battle_id in every method, the $name parameter is used to get the warrior_name in two of the methods, and the $type parameter is used to get the type in two methods. So, instead of passing these values again and again, just assign them to a property and use that property whenever you would have used the passed parameter. This is most effective if done in the first method in which these parameters are all used. I would assume this is in createBattle()?

public function createBattle( $warrior ) {
    $this->battle = $warrior[ 'battle' ];
    $this->name = $warrior[ 'name' ];
    $this->type = $warrior[ 'type' ];

    //etc...
}

Don't forget to declare those properties at the top of your class. Declaring a property is equally as important as declaring your methods. If you don't declare a property or method it defaults to the public scope, but this wont always be the case. I have heard rumors of this being deprecated soon, which means no more default values and whole lot more warnings. So be prepared and code accordingly.

So, to answer your question: Not really, but a decent enough start. Just keep in mind the following principles: "Don't Repeat Yourself" (DRY), Single Responsibility, and, as Peter pointed out, SOLID, which is kind of the first two plus a few others all rolled into one. If your code follows these principles and utilizes the fundamental aspects of the OOP (Encapsulation, Inheritance, and Polymorphism), then you should be fine.

BTW: What tutorials are you using that is teaching you to use globals? I've become curious because this seems to be a growing trend recently, and if this is all coming from the same source I want to see what I can do about stopping it.

Answer 2

No, you can't bocouse you relying on a global object which is bad try dependency injection at you class constructor.

<?php 
include './database/config.php'; 

class DB { 

    private $_pdo;

    public function __construct(\PDO $pdo) {
        $this->_pdo = $pdo;
    }

    public function dataWarrior($battle,$name,$type){ 
        $stmt = $this->_pdo->prepare("SELECT battle_id,warrior_name,warrior_type,health,attack,defense,speed,evade 
        FROM battle WHERE battle_id = :battle AND warrior_name = :name AND warrior_type = :type LIMIT 1"); 

        $stmt->execute(array(":battle"=>$battle, ":name"=>$name, ":type"=>$type)); 

        return $stmt;
    } 
    public function checkBattleID($battle){ 
        $stmt = $this->_pdo->prepare("SELECT battle_id FROM battle WHERE battle_id=:battle LIMIT 1"); 

        $stmt->execute(array(":battle"=>$battle)); 

        return $stmt->rowCount() > 0; 
    } 
    public function createBattle($warrior){ 
        $stmt = $this->_pdo->prepare('INSERT INTO battle(battle_id,warrior_name,warrior_type, 
        health,attack,defense,speed,evade) VALUES(:battle,:name,:type,:health,:attack,:defense,:speed,:evade)'); 

        $stmt->execute(array(':battle'=>$warrior['battle'], ':name'=>$warrior['name'],  
        ':type'=>$warrior['type'], ':health'=>$warrior['health'], ':attack'=>$warrior['attack'], 
        ':defense'=>$warrior['defense'], ':speed'=>$warrior['speed'], ':evade'=>$warrior['evade']));

        //return battle?
    } 
    public function deleteBattle($battle){ 
        $stmt = $this->_pdo->prepare('DELETE FROM battle WHERE battle_id = :battle'); 
        $stmt->execute(array(':battle'=>$battle));

        //return true/false?
    } 
}

The try-catch block also unnecessary in your class if you only echos the error messages.

And you should create a new connection logic (do not put the new PDO stuff in a code file which is included every time when you need somewhere) becouse this type of connecting to database is not a solution in 2012.