up vote 1 down vote favorite

I implemented a MVC Framework with Generic DAO and I would like know if is correct. I did not have developed yet the MySQLDAO.

My controller class

class ClienteController {
  public function __construct() {  }

  public function actionSubmit(){
      $objCliente = new Cliente();
      $objCliente->setNome($_POST['nome']);
      $objCliente->setCodigo($_POST['codigo']);
      $objCliente->getDAOFactory()->insert();
  }
}

DAO Class for client, persistence.

class ClienteDAO implements IRecord {
  const TABLE = 'cliente';

  private $campos = array(
                         "codigo" => "cliCod",
                         "nome"   => "cliNom"
                         );

  private  $DAOFactory;

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

  public function getDAOFactory() {
      return $this->DAOFactory->getInstance();
  }
}

Model class

class Cliente extends ClienteDAO {
  private $codigo;
  private $nome;

  public function __construct(){  }

  public function getCodigo() {
      return $this->codigo;
  }

  public function setCodigo( $codigo ) {
      if ( $codigo > 0 ) {
          $this->codigo = $codigo;
      }
  }

  public function getNome() {
      return $this->nome;
  }

  public function setNome( $nome ) {
      $this->nome = $nome;
  }
}

Database class

class MySQLDao implements iDAO {
    public function insert() {

    }

    public function update() {

    }

    public function delete() {

    }

    public function limit() {

    }

    public function orderBy() {

    }

    public function findAll() {

    }

    public function addColumns() {

    }

    public function addGroupBy() {

    }

    public function addOrderBy() {

    }

    public function addHaving() {

    }

    public function addCustomizedHaving() {

    }

    public function addColumSubQuery() {

    }

    public function addAnd() {

    }

    public function addOr() {

    }

    public function checkExistanceColumn() {

    }
}

Factory class

class DAOFactory {
  private $objeto;
  private $factory;

  public function __construct($obj, $banco = null) {
      $this->factory = $banco;
      $this->objeto = $obj;
  }

  public function getFactory() {
      return $this->factory;
  }

  public function setFactory($factory) {
      $this->factory = $factory;
  }

  public static function getInstance() {
      if ($this->getFactory() == null) {
          return new MySQLDAO($this->getObjeto());
      }
  }

  public function getObjeto() {
      return $this->objeto;
  }
}

Interfaces

interface IDAO {
  public function insert();
  public function update();
  public function delete();
  public function limit();
  public function orderBy();
  public function findAll();
  public function addColumns();
  public function addGroupBy();
  public function addOrderBy();
  public function addHaving();
  public function addCustomizedHaving();
  public function addColumSubQuery();
  public function addAnd();
  public function addOr();
  public function checkExistanceColumn();
}

interface IRecord {
  public function getDAOFactory();
}
share|improve this question
Welcome to Code Review! I'm not familiar with DAO, but I think that we need more explanation to understand what you want to do. Also, make sure your code works before submitting it here. Have a good day. – Quentin Pradet Sep 17 '12 at 8:32

2 Answers

up vote 1 down vote

Some recommendations:

  • Instead of creating monolithic DAOs, you should split it in domain objects and data mappers. This way you would separate the entity-specific business logic from storage.

  • Do not ever use active record as default solution in any code. It has very specific and limited use-case, and when you exceed those limitation, your code becomes convoluted and hackish. Active records are good for two things: fast prototyping (creating temporary throw-away code) and implementation extremely simple domain entities with almost no business logic (glorified value objects with hard-coded storage).

  • Look into use of PDO. It should provide you with a bit more flexible storage infrastructure when dealing with SQL databases.

  • You currently have domain business logic from model layer leaking in the controller. This is kinda bad. Controller should only change the state of model layer. It should not know how to create and save entities. You might benefit from reading this answer.

  • Watch every lecture, that is listed here. Your code seems to suffer from tight and violations of Law of Demeter. You won't be able to understand everything, that is talked about, at first (at least , I didn't). If that happens, watch the listing till the end ad start again. Each time you will understand more of it.

share|improve this answer
Thanks teresko, I will follow your suggestions. Do you don't like of pattern DAO? – Filipi Santana de Assis Aug 31 '12 at 12:38
I have no problems with DAO pattern (as defined here and here) as such. But you are not implementing DAO there. – tereško Aug 31 '12 at 13:41
Is Generic DAO, why don't will be? – Filipi Santana de Assis Aug 31 '12 at 14:05
up vote 0 down vote

I don't feel that this post really qualifies as an answer as I don't know enough about the main subjects in the OP's post, DAO and the factory pattern. I am unfamiliar with both, but there are a few things unrelated that I noticed and couldn't hurt to be pointed out. I would have just posted this as a comment, but it was too large.

There is no need to declare a class constructor or inherited method unless you are actually going to do something with it. To clarify on the inherited method, you only need to redefine the method if you are adding or manipulating the data passed to it or returned by it. I only mention this last bit because of how frequently I see people trying to do this. It does not appear to be an issue here, just figured I'd cover it.

You should not pass raw user input to your code. Sanitize and validate it. Check out PHP's filter_input() for a simple example.

$nome = filter_input( INPUT_POST, 'nome', FILTER_SANITIZE_STRING );
//etc...

if( ! $nome || ! $codigo ) {
    throw new Exception( 'Not enough information to process this request.' );
}

$objCliente->setNome( $nome );
//etc...

The $codigo property in your Cliente class should be instantiated as an integer if you plan on comparing it to zero before allowing it to be set. Your current setter probably only works because of PHP's loose type conversion, but you shouldn't rely too heavily on that.

Your MySQLDao class, if none of its methods are going to be defined, should be an abstract class or another interface. Probably another interface since all of the methods appear to be public and empty. Just remember, an interface extends another interface, not implements it.

share|improve this answer
Thanks for answer, but this model is conceptual. I would like know "if the concept is correct". So, if is confused, please! I explain again. – Filipi Santana de Assis Aug 30 '12 at 19:18

Your Answer

 
or
required, but never shown
discard

By posting your answer, you agree to the privacy policy and terms of service.

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