3
\$\begingroup\$

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",
      "chris_davis",
      "samwise",
      "famous_dave"
    );

    /**
     * Create the IPlayers that will play this game of war.  Add then to the IGame
     */
    $player0 = new warIPlayer();
    $player0Name = $availablePlayers[rand( 0 , 8)];
    $player0->setName( $player0Name );

    $player1 = new warIPlayer();

    $player1Name = $availablePlayers[rand( 0 , 8)];

    while( $player1Name == $player0Name )
    {
      $player1Name = $availablePlayers[rand( 0 , 8)];
    }

    $player1->setName( $player1Name );

    $this->addIPlayer( $player0 );
    $this->addIPlayer( $player1 );

    /**
     * Initialize the warStats
     */
    self::$warStats[$player0Name] = null;
    self::$warStats[$player1Name] = null;
    self::$warStats[$player0Name]['wins'] = null;
    self::$warStats[$player1Name]['wins'] = null;



    /**
     * Create the IDeck of ICards for this game.  Just a standard deck
     * Shuffle and deal the ICards
     */

    $IDeckSettings = self::$config->getSetting( 'IDeck' );
    $IDeckType     = $IDeckSettings->IDeckClass->value;

    $deckOfCards = IDeckFactory::getInstance( $IDeckType );
    $deckOfCards->buildIDeck();
    $deckOfCards->shuffleIDeck();

    $this->setIDeck( $deckOfCards );

    /**
     * The IGame is responsible for dealing out the cards in whatever way the cards are dealt for this game
     */
    $this->dealICards();

    /**
     * The ICards have been dealt.  Play turns
     */

    $keepGoing = true;

    //how many turns have been played.  This variable will be incremented by one after each turn is played.  the incrementing will be done by the following while loop
    $turnCount = 0;
    while( $keepGoing )
    {

      echo "PLAYING TURN $turnCount out of " . self::MAX_TURNS . "\n\n";
      /**
       * create a new turn, and give the turn the information about this IGame (players, their cards, etc)
       */
      $ITurn = new normalWarITurn();
      $ITurn->setIGame( $this );

      /**
       * Play the turn
       */
      $keepGoing = $ITurn->play();
      $turnCount++;
      if( $turnCount == self::MAX_TURNS )
      {
          echo "The maximum number of turns has been met.  The game is a draw \n";
          print_r( self::$warStats );
          echo "AT THE END OF THE GAME, THE CARD COUNTS ARE: \n$player0Name = " . count( $player0->getICardCollection()->getICards() ) . " \nTO \n$player1Name = " . count( $player1->getICardCollection()->getICards() ) . "\n\n";
          exit;
      }
    }

    if( count( $player0->getICardCollection()->getICards() ) == 0 )
    {
      echo $player0->getName() . " has lost! and " . $player1->getName() . " has won! \n";
    }
    else
    {
      echo $player1->getName() . " has lost! and " . $player0->getName() . " has won! \n";
    }
  }

  /**
   * Deal the ICards to the IPlayers
   * In the game of war, alternate.. deal one to one, and then the next to the other until the deck is empty
   */
  public function dealICards()
  {

    /**
     * Get the IPlayers
     */
    $IPlayers = $this->IPlayers;

    /**
     * Get the IDeck
     */
    $IDeck    = $this
      ->IDeck
      ->getICardCollection();

    /**
     * While the IDeck is not empty, deal the ICards to the IPlayers
     */
    $count = 0;
    while( $IDeck->getState() != "EMPTY" )
    {
      try
      {
        /**
         * Get an ICard from the top of the IDeck
         */
        $ICard = $IDeck->getICard();
      }
      catch( NoICardException $e )
      {
        echo $e->getMessage();
      }

      /**
       * If the counter is even, assign the ICard to IPlayer[0], else assign to IPlayer[1]
       */
      if( $count % 2 == 0 )
      {
        $IPlayers[0]->setICard( $ICard );
      }
      else
      {
        $IPlayers[1]->setICard( $ICard );
      }

      $count++;
    }

    /**
     * Set the IPlayers hands to full state
     */
    $IPlayers[0]
      ->getICardCollection()
      ->setState( "FULL" );

    $IPlayers[1]
      ->getICardCollection()
      ->setState( "FULL" );
    return true;
  }

  /** 
   * A game consists of an IPlayer.  Here are functions pertaining to the IPlayer
   */
  public function addIPlayer( IPlayer $IPlayer )
  {
    $this->IPlayers[] = $IPlayer;
    return true;
  }

  public function removeIPlayer( IPlayer $IPlayer )
  {

  }

  /** 
   * Returns a player object named $name
   */
  public function getIPlayerByName( $name )
  {
    return $this->IPlayers[$IPlayer->getIPlayerByName()];
  }

  /** 
   * Will return all IPlayers
   */
  public function getIPlayers()
  {
    return $this->IPlayers;
  }

  /** 
   * A game consists of an IDeck (deck of cards).  Here are functions pertaining to the IDeck
   */
  public function getIDeck()
  {
    return $this->IDeck;
  }

  public function setIDeck( IDeck $IDeck )
  {
    $this->IDeck = $IDeck;
    return;
  }

}

$w = new war();
$w->main();

?>

Here are my interfaces:

/**
 * Written specifically for the card game war.  The abstract concepts should be applicable to any game that is played
 * with a standard deck of playing cards (not card games like uno or old maid)
 */

interface ICardCollection
{
  /**
   * The IDeck is in the empty state
   */
  const STATE_EMPTY = "EMPTY";

  /**
   * The IDeck is not empty, but not full
   */
  const STATE_NOT_EMPTY = "NOT_EMPTY";

  /**
   * Full deck of ICards
   */
  const STATE_FULL = "FULL";


  /**
   * Adds one ICard to the ICardCollection
   */
  public function setICard( ICard $ICard );

  /**
   * Pops one ICard from the ICardCollection
   */
  public function getICard();

  /**
   * Set all ICards for this collection.  Current collection will be overwritten.
   */
  public function setICards( $ICards );

  /**
   * Append ICards to the already existing ICards array
   */
  public function addICards( $ICards );

  /**
   * Get all ICards from this collection
   */
  public function getICards();

}

/**
 * An IDeck is made up of ICards.  The IDeck will always have a property called ICards which will be a container for ICards
 */
interface IDeck extends ICardCollection
{

  /**
   * An IDeck of cards needs to be responsible for providing functionality for building a deck of cards.  
   * The IDeck may be standard playing cards, old maid cards, uno cards, etc.  Each is different, and each must be built differently
   * The buildDeck function (for the war IDeck) will be a facade in front of helper functions that will ultimately build an IDeck of ICards
   */
  public function buildIDeck();

  /**
   * The IDeck will be set to its default state.  The IDeck will contain all ICards, and they will not be
   * shuffled.
   */
  public function resetIDeck();

  /**
   * After this function is called, the container of ICards will be in a random order
   */
  public function shuffleIDeck( stdClass $options = null );

}

/**
 *
 * The ITurn is responsible for invoking the rules of the IGame.  The ITurn will return whatever the IGame is expecting it to return.  In war, after a turn
 * is played, the ITurn will return the array of ICards that were played during this turn, and also who is the winner.
 *
 * In card games, the players take turns.  Different games will implement a turn differently
 * Working under the assumption that a game is comprised of turns.  
 * Working under the assumption that a turn will have player(s)
 * Working under the assumption that in a card game, a turn has cards.  (When a player plays the turn, they make the cards that they play a part of the turn).
 *
 */
interface ITurn
{

  /**
   * States to tell whether a turn is still going on, or if the turn is over.
   */
  const STATE_TURN_ACTIVE = "TURN_ACTIVE";
  const STATE_TURN_OVER   = "TURN_OVER";


  public function setIGame( IGame $IGame );
  public function getIGame();

  /**
   * the play function will take the cards from the turn into consideration, and invoke the rules of a particular game.  
   * 
   * Also:
   *
   * Different card games have different things that happen upon winning a turn.  For war, the goal is
   * to get all of the cards.  In other games, you probably don't want to end up with all of the cards.  The ITurn
   * should return whatever it needs to return to the IGame, and the IGame is responsible for interfacing with the 
   * IPlayers.  The abstract ITurn class will have an abstract method called win() 
   */
  public function play();

}

/**
 * Interface for a player.  Will provide template for generic player behavior
 */
interface IPlayer
{

  /**
   * This function will pop and return a card from the player's hand
   *
   * The ITurn will use this function to interact with the IPlayer.
   */
  public function getICard();

  /**
   * Will add one single ICard to the players hand
   */
  public function setICard( ICard $ICard );

  /**
   * Will add multiple ICards to the players hand
   */
  public function setICards( $ICards = array() );

  public function setName( $name );

  public function getName();

}

/**
 * This will be the driver for the program
 *
 * Working under the assumption that if a game exists, and it is being played, it will contain player(s)
 * Working under the assumption that most card games have turns and these turns involve players.
 */
interface IGame
{

  /**
   * States to tell whether a game is still going on, or if the game is over.
   */
  const STATE_GAME_ACTIVE = "GAME_ACTIVE";
  const STATE_GAME_OVER   = "GAME_OVER";

  /**
   * A game consists of an IPlayer.  Here are functions pertaining to the IPlayer
   */
  public function addIPlayer( IPlayer $IPlayer );
  public function removeIPlayer( IPlayer $IPlayer );

  /**
   * Returns a player object named $name
   */
  public function getIPlayerByName( $name );

  /**
   * Will return all IPlayers
   */
  public function getIPlayers();

  /**
   * A game consists of an IDeck (deck of cards).  Here are functions pertaining to the IDeck
   */
  public function getIDeck();
  public function setIDeck( IDeck $IDeck );

  /**
   * Deal the ICards to the IPlayers
   */
  public function dealICards();

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

}

interface ICard
{


  /**
   * Returns the value of this ICard
   */
  public function getValue();

}

?>

Here is an abstract deck of cards:

abstract class abstractIDeck extends abstractBase implements IDeck
{

  protected $ICardCollection = null;

  public function __construct()
  {
    $this->ICardCollection = new ICardCollection();
  }

  /**
   * After this function is called, the container of ICards will be in a random order
   */
  public function shuffleIDeck( stdClass $options = null )
  {
    /**
     * To shuffle the IDeck, get the ICards from the collection, shuffle them, and then set them back
     */

    $ICards = $this->ICardCollection->getICards(); 

    shuffle( $ICards );

    $this
      ->ICardCollection
      ->setICards( $ICards );

    return true;
  }

  /**
   * The IDeck will be set to its default state.  The IDeck will contain all ICards, and they will not be
   * shuffled.
   */
  public function resetIDeck()
  {
    $this->ICardCollection = new ICardCollection();
    $this->buildIDeck();
    return true;
  }

  /**
   * Will pop and return an ICard from the IDeck.  If the IDeck is empty, return false
   */
  public function getICard( stdClass $options = null )
  {
    /**
     * Shift off one card, and return it.
     */

    try
    {
      $returnCard = $this
        ->ICardCollection
        ->getICard();
    }
    catch( NoICardsException $e )
    {

    }

    return $returnCard;
  }

  /**
   * Will add an ICard to the IDeck.
   */
  public function setICard( ICard $ICard )
  {
    $this
      ->ICardCollection
      ->setICard( $ICard );

    return true;
  }


  /** 
   * Set all ICards for this collection.  Current collection will be overwritten.
   */
  public function setICards( $ICards )
  {
    $this
      ->ICardCollection
      ->setICards( $ICards );
  }

  /** 
   * Append ICards to the already existing ICards array
   */
  public function addICards( $ICards )
  {
    $this
      ->ICardCollection
      ->addICards( $ICards );
  }

  /** 
   * Get all ICards from this collection
   */
  public function getICards()
  {
    return $this
      ->ICardCollection
      ->getICards();
  }

  /**
   * Get the full ICardCollection object
   */
  public function getICardCollection()
  {
    return $this->ICardCollection;
  }


}

?>

And here is a standard deck of cards extending the abstract deck:

/**
 * Class to represent a standard deck of playing cards (hearts,spades,diamonds,clubs. 2 through ace)
 */
class standardIDeck extends abstractIDeck implements IDeck
{

  /**
   * Values for the ace card should be 1 or 14
   */
  const ACE_VALUE = 15;
  /**
   * An IDeck of cards extending abstractDeck needs to be responsible for providing functionality for building a deck of cards.  
   * The IDeck may be standard playing cards, old maid cards, uno cards, etc.  Each is different, and each must be built differently
   * The buildDeck function (for the war IDeck) will be a facade in front of helper functions that will ultimately build and return
   * an IDeck of cards
   */

  public function __construct()
  {
    abstractIDeck::__construct();
  }

  public function buildIDeck()
  {


    $suits = array( "hearts" , "diamonds" , "spades" , "clubs" );

    /**
     * For a standard deck of cards, create each of the 4 suits
     */
    foreach( $suits as $suit )
    {
      /**
       * If the ace value is not set properly, an exception will be raised
       */
      try
      {
        $this->buildSuit( $suit );
      }
      catch(InvalidAceValueException $e)
      {
        /**
         * the exception handler will create an ace card with a value of 14 if the ACE_VALUE is not properly set
         */
        $aceCard = $e->handleException();
        $this
          ->ICardCollection
          ->setICard( $aceCard );
      }
    }

    $this
      ->ICardCollection
      ->setState( self::STATE_FULL );

    return true;
  }

  private function buildSuit( $suit )
  {

    /**
     * Create all cards, except for the ace.
     */
    $ICards = array();
    for( $cardValue = 2; $cardValue < 14; $cardValue++ )
    {
      /**
       * give the appropriate title to the face cards
       */
      if( $cardValue > 10 && $cardValue < 14 )
      {
        switch( $cardValue )
        {
          case "11":
            $title = "jack of " . $suit;
            break;
          case "12":
            $title = "queen of " . $suit;
            break;
          case "13":
            $title = "king of " . $suit;
            break;
        }
      }
      else
      {
        /**
         * The title of a regular number card is just its number.  The title for a card worth 11 is Jack, 12 = Queen, etc
         */
        $title = $cardValue . " of $suit";
      }
      /**
       * create the card, and then insert it into the returnCards array
       */
      $ICard = new standardICard( $suit , $cardValue , $title );

      $ICards[] = $ICard;

    }

    $this
      ->ICardCollection
      ->addICards( $ICards );


    /**
     * Take care of creating the ace
     */
    if( self::ACE_VALUE != 1 && self::ACE_VALUE != 14 )
    {
      throw new InvalidAceValueException( "Ace value must be 1 or 14.  Check the const at the top of standardIDeck.php.  I will default to the value for ace as being set to 14" );
    }

    $ICard = new standardICard( $suit , self::ACE_VALUE , "ace of " . $suit );
    $this->ICardCollection->setICard( $ICard );

    return true;
  }

}

class InvalidAceValueException extends \Exception
{
  /**
   * handle the exception by setting ace to high value
   */
  public function handleException()
  {
    $traceArray = $this->getTrace();
    $suit = $traceArray[0]['args'][0];
    return new standardICard( $suit , 14 , "ace of " . $suit );
  }
}


?>
\$\endgroup\$
0

2 Answers 2

6
\$\begingroup\$

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 to saveStuff will always return true, so the cookie will always be set, or will it? If the saveStuff method caught a PDOException, it'll echo the error, and the headers will be sent. I can't set a cookie anymore. In this case, though, the problems are pretty obvious, but the larger your codebase becomes, the harder it'll be to debug code like this. Imagine there being 20-some instances involved, then it'll be up to you to uncover which one of these instances is causing the headers to be sent before the cookie was sent. Have fun and try not to cry...
Ok, dry those tears, and just remove the try...catch block. What you'll get now is an error-page saying uncaught exception on line X. You can then look at the stack trace, see what data came from where, and what exactly it was that caused everything to go pear-shaped. There's no insert's being performed after the one that went wrong... in this case, a "crash" is the cleaner alternative (google Worse is Better)

Other than that, you are using type-hints, which is a good thing, you are using getters and setters, that allow for some injection. Again, nothing wrong with that, although, you might want to consider implementing the fluent interface, also known as chainable methods:

public function setSomeDependency( SomeDependency $foo )
{
    $this->dependency = $foo;
    return $this;
}
//so you can use it like so:

$instance->setSomeDependency($instanceofDepend)
         ->setSomethigElse('like the name')
         ->getEverything();

There are still a lot of issues I haven't discussed, though, for example:

$player0Name = $availablePlayers[rand( 0 , 8)];
//a few lines later:
$player1Name = $availablePlayers[rand( 0 , 8)];

There is a 1/8 chance that both variables will be assigned the same name here. That's not what you want, is it?
The number of available names, by the way, is probably a global state per request. If there is a max of 8 concurrent players, then the names that are taken should be unset, so that no two players can possibly get the same name. Now this, perhaps, a valid use-case for a static variable:

private static $availablePlayers = array();
protected function getAvailableName()
{
    if (self::$availablePlayers)
    {
        return array_pop(self::$availablePlayers);
    }
    throw new RuntimeException('All 8 player names are taken, gameroom is full');
}

Of course, I don't actually know what you're doing here, nor do I know where the names are comming from, and if the number of players is to be limited, but I do know that a 1/8 chance of 2 variables being assigned the same value is not what you want, nor does it make sense (to me) to have 2 variables, called $player0 and $player1.
To me, a list (or array) just makes more sense:

$players = array(
    new Player($this->getAvailableName()),
    new Player($this->getAvailableName())
);

All things considered, I'd bin this code, and start again. Keep asking yourself these simple questions:

  • Is my class doing more than 1 thing? If it is, split into multiple classes.
  • Do I have to scroll to read the entire method? If so, there's something wrong with it
  • Am I sure that, after calling a method, nothing, except for the instance (perhaps) has changed (no headers, no pass-by-reference)? If not, your code is smelly
  • Am I keeping things simple? If not Keep It Simple, Stupid
  • Can I write a test, without running into the static wall of death? If you can't Inject, don't use singletons.
\$\endgroup\$
3
  • \$\begingroup\$ Thank you for the critique. I am reading through what you wrote, and I'm going to try to figure out how to make this better. \$\endgroup\$
    – Chris D.
    Commented Aug 18, 2013 at 21:42
  • \$\begingroup\$ +1 from me for a very detailed review, but I have to say I dislike phrases like "too procedural" because they smell of dogmatism and don't contain any concrete information. There should be no problem writing code procedural-style when it's the best fit for the problem. \$\endgroup\$
    – idoby
    Commented Sep 2, 2013 at 14:59
  • \$\begingroup\$ @busy_wait: considering the title, this question is about good use of OO principles. procedural programming is right out, then. Besides: the phrase "too procedural" is something I got from the initial question. It's not something I would say \$\endgroup\$ Commented Sep 3, 2013 at 6:55
2
\$\begingroup\$

ugh. I don't even know where to start...

The code you wrote is not OO. In no way at all.

Let's answer you questions first:

I have been told that my main() function is too procedural

You have a 'War' class with a main() function that does everything. This is soooo java main() where you have to write your controller in the main(). But in pp you don't have to encapsulate a cotroller in a class and cramp everything in a main()

I have been told that the output logic should be separated

What if ou want to change the way a 'jack of spades' is displayed? You wil now have to dive into your business logic code... That simply asks for problems

Then overall: Using the words 'class' and 'function' doesn't mean you are writing proper OO code. When writing good code always think SOLID:http://en.wikipedia.org/wiki/SOLID_%28object-oriented_design%29

So, what will make you a better programmer? SOLID, always start there. Then read about the different patterns and when they should be used. A good tut from ibm: http://www.ibm.com/developerworks/library/os-php-designptrns/

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for looking at this for me! I'm going to rewrite this with SOLID in mind. I appreciate the honesty. \$\endgroup\$
    – Chris D.
    Commented Aug 18, 2013 at 22:02
  • \$\begingroup\$ @ChrisD. reading it again now, maybe I was a bit harsh. But you are welcome! \$\endgroup\$
    – Pinoniq
    Commented Aug 19, 2013 at 7:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.