Review of object-oriented skills with War game

Here are my code samples: https://github.com/arthaeus/war

This particular sample is the card game 'War'. The interviewer off-handedly asked me: "what if I wanted to play War with an Uno deck of cards?" I threw an Uno deck implementation in there as well, so don't be confused.

I have been told that my main() function is too procedural, and also I have been told that the output logic should be separated. Do you agree with these assessments? Do you see anything else that will make me a better coder? Also, am I botching the factory pattern?

Edit

Here is some of the code that I would like to have reviewed:

Here is main.php:

```
class war implements IGame
{

/**
* warStats will hold statistics about the current game.
*/
public static $warStats = null;
public static $config = null;

private $IPlayers = array();
private $IDeck = array();

const MAX_WAR_WINS = 100;
const MAX_TURNS = 10000;

public function __construct()
{

/**
* Initialize the config
*/
self::$config = new config();
self::$config->buildConfig();
}

public static function addWarStat( stdClass $warStat )
{

if( !self::$warStats )
{
self::$warStats = array();
if( !array_key_exists( self::$warStats[$warStat->playerName] , self::$warStats ) )
{
self::$warStats[$playerName];
}
}

self::$warStats[$warStat->playerName]['wins']++;
if( self::$warStats[$warStat->playerName]['wins'] == self::MAX_WAR_WINS )
{
echo "GAME IS OVER BECAUSE " . $warStat->playerName . " has won " . self::MAX_WAR_WINS . " wars. \n Here are the standings: \n";
print_r( self::$warStats );
exit;
}
return true;
}

/**
* Play a turn
*/
public function playTurn( ITurn $ITurn )
{

}

public function main()
{

$availablePlayers = array(
"frodo",
"hank_hill",
"mum_ra",
"gandalf",
"steve_urkel",
"aragorn",

Erm, right... I don't mean to be rude, but OO in PHP isn't too different from OO in any other language. The SOLID principles apply there, too. You seem to be writing code as if you had a phobia of all things SOLID stands for. Is the main method too procedural. having a method called main is ok. What you're doing in the main method is too procedural.

Do you have to separate the output from the logic. Of course! Why would you bother with OO, if you don't separate things, you could just as well write one class, create an instance and use that class' scope instead of the global scope. But why bother with that class in the first place?

Are you botching the factory pattern? You're using the factory pattern?... (yes, I'm afraid you are)

But let's start with something basic, yet important:

Even though PHP isn't standardized just yet, there is an unofficial coding standard, which can be found here. All major players (Zend included) subscribe to this standard, as should you. Classes start with an Upper-Case, yours don't. Fix that. But that's just a cosmetic issue...

Factory:

You have an awful lot of static's in your code. I'm editing this answer, because I just noticed that this is to implement the factory pattern. Don't. Last time I applied for a job as PHP dev, I actually got a high-five, because I set off on a rant about why statics are, essentially, as bad as using eval or global. They have their use-cases, but in PHP, I've only ever really needed them 2, or 3 times in the past 5 years. Tops. Read about the D in SOLID, and learn to write tests. You'll soon find yourself hating statics and singletons as much as the next man.

Even though a Factory can be handy (as can a Registry). They're really just globals in drag.

Main:

For some reason, you also define a public function main in an object. I can understand where that might come from. Other languages (Java, C, C++, Python...) require a main or __main__ function to be defined somewhere. The thing is: these are other languages. It's a bit like using a double negative, because some languages use double negation (Afrikaans, French). Just because some of the more popular languages require a main function, doesn't mean that all languages need this.

Having a main function isn't all that bad, nor is it "too procedural". It's what you're doing in that method that is just not OO at all.

In OOP, a class represents a single entity, and therefore, it should have one (and only one) task. A class can be responsible for rendering output, or interacting with the database, or processing the (raw) request data. That's all fine, but what a single class can't do is handle the response and insert data in the db. Even worse would be if that same class were then to echo output. That's a gross violation of the Single Responsibility Principle.

Which brings us to your third question...

Separation of output

Your class echoes, creates instances, performs all sorts of things in a single method. If that weren't bad enough, it also wraps bits and pieces into a try..catch, only to echo $e->getMessage(); and carry on as if nothing happened!

The way you should think of Exceptions is, they are things that are thrown outside of the normal flow of the code, because there's something odd going on. A class that inserts data into a DB doesn't know (nor does it need to know) where that data comes from or what it means. If the insert fails, an exception is thrown, and the code that called the insert method should deal with the Exception. Not the code that just passed the data on to the DB. If the code that called the insert method can't handle the exception, let it go. Its an error, don't hush it up and try to get by, let the script die, it's to prevent any further damage from being done.

Imagine I were to give you this piece if truly horrid code, and you had to debug it:

class MyDB
{
    private $connection = null;
    private $statements = array();
    public function __construct()
    {
        $this->connection = new PDO();
    }
    public function saveStuff(array $rows)
    {
        if (!isset($this->statements['saveStuff']))
        {
            $stmt = $this->connection->prepare('INSERT INTO tbl (name, score, status) (?,?,?)';
            $this->statements['saveStuff'] = $stmt;
        }
        $this->connection->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
        $i = 1;
        foreach($rows as $row)
        {
            try
            {
                $this->connection->execute($this->statements['saveStuff'], $row);
            }
            catch(PDOException $e)
            {
                echo 'Insert failed for row #', $i, ' ', $e->getMessage();
            }
        }
        return true;
    }
}

Used like this:

$db = new MyDB;
$ok = $db->saveStuff(
    array(
        array(1,2,3),
        array('a',2,3),
        array('asc', 'x', true)
    )
);
if ($ok === true)
{
    setcookie('dataSet',1);
}

The call

StackExchange Code Review Q#29878, answer score: 6