Fixes #35: Removendo contantes de configuração. #37

jackmakiyama · 2016-12-29T03:37:10Z

Foi removido as contantes que tinha caminhos de pastas fisicos e no seu
lugar foi adicionado na AppFactory as namespace hardcoded, gerando um
debito tecnico, mas removendo essas contantes.

Foi removido as contantes que tinha caminhos de pastas fisicos e no seu lugar foi adicionado na AppFactory as namespace hardcoded, gerando um debito tecnico, mas removendo essas contantes.

enieber · 2016-12-29T10:59:54Z

src/Factory/AppFactory.php

-            throw new Exception($e->getMessage());
-        }
+        $class = get_class($objeto);
+        $classShortName = (new \ReflectionClass($objeto))->getShortName();


no caso dessas ReflectionClass a função vai pegar o nome da classe ao invés de precisar fazer um require_once ?

malukenho · 2017-05-10T03:00:49Z

src/Factory/AppFactory.php

-        } catch (Exception $e) {
-            throw new Exception($e->getMessage());
-        }
+        $class = get_class($objeto);


Ya don't need to get the name of class, just use ReflectionObject instead.

malukenho · 2017-05-10T03:01:08Z

src/Factory/AppFactory.php

+        $class = get_class($objeto);
+        $classShortName = (new \ReflectionClass($objeto))->getShortName();
+
+        $classDAO = '\\PseudoORM\\DAO\\' . $classShortName . 'DAO';


Should a DI be used instead?

malukenho · 2017-05-10T03:02:00Z

src/Factory/AppFactory.php

+            : 'Generic'
+        ;
+
+        $repository = '\\PseudoORM\\DAO\\' . $entityName . 'DAO';


Repositories should be configured in some kind of mapping file — xml? —

malukenho · 2017-05-10T03:02:25Z

src/Factory/AppFactory.php

        return self::$instance;
    }

    public static function getRepository(EntidadeBase $objeto)


Return type?

Why is it static?

malukenho · 2017-05-10T03:02:33Z

src/Factory/AppFactory.php

@@ -1,8 +1,7 @@
 <?php
 namespace PseudoORM\Factory;


malukenho · 2017-05-10T03:06:57Z

src/Factory/AppFactory.php

+
+        $repository = '\\PseudoORM\\DAO\\' . $entityName . 'DAO';
+
+        return new $repository($class);


what if $repository isn't a valid class?

malukenho · 2017-05-10T03:07:37Z

src/Factory/AppFactory.php

+
+        $classDAO = '\\PseudoORM\\DAO\\' . $classShortName . 'DAO';
+
+        $entityName = class_exists($classDAO)


What if my repository is an anonymous class?

malukenho · 2017-05-10T03:08:29Z

test/functional/check-environment.phpt

 define("SCHEMA", '');
 define('ENCODING', "SET NAMES 'utf8';");
 define("DB_DSN", "pgsql:host=".DB_HOST.";port=".DB_PORT.";dbname=".DB_NAME.";");
 define("SHOW_SQL_ERROR", PDO::ERRMODE_EXCEPTION);


Maybe mark this constants to be removed in the future as well? @todo ?

marquei como @todo

malukenho · 2017-05-10T03:10:47Z

Pelo o que eu pude ler getRepository está fazendo muito mais do que deveria.

Pega dados da entidade
Resolve mapa entre entidade e repositório
Instancia repositório com metadados — $class?
Retorna instancia de determinado repositório

Considere que cada um desses passo poderia ser melhor escrito/testado isoladamente.

Nota: Você pode ter um facade englobando toda essa lógica.

/cc @jackmakiyama

jackmakiyama · 2017-05-10T03:18:16Z

Pelo o que eu pude ler getRepository está fazendo muito mais do que deveria.

Com certeza, o projeto é bem legado, a ideia era dar uma normalizada pra entrar num ciclo de refatoração.

Essa factory eu penso que pode ser removida e trabalhar melhor com composição.

Veja um exemplo onde uma normalização iria facilitar na refatoração aqui.

EvandroMohr · 2017-06-18T16:04:14Z

@malukenho e @jackmakiyama dei uma refatorada no método getRepository separando um pouco mais as responsabilidades.

Classes anônimas não são suportadas ainda, quem sabe mais pra frente.
A chamada estática é apenas conveniência, nada impede que seja instanciada ou mesmo chamada por um singleton. Inclusive há traços de um singleton antigo que ainda permanece na classe por falta de direcionamento de como isso vai ficar. Não decidimos ainda.

Acho que o próximo passo é dar uma refatorada na classe de DAO, ela está fazendo muito mais do que deveria.

…odos

…ntes, removendo parâmetro booleano

malukenho · 2017-06-19T05:52:56Z

A chamada estática é apenas conveniência, nada impede que seja instanciada ou mesmo chamada por um singleton. Inclusive há traços de um singleton antigo que ainda permanece na classe por falta de direcionamento de como isso vai ficar. Não decidimos ainda.

IMHO, deve ser instanciado provavelmente. Se preciso, fica a cargo do cliente usar algum tipo de service manager, ou afim.

malukenho · 2017-06-19T05:54:00Z

src/DAO/GenericDAO.php

+    {
+        $script = $this->geraScriptDeCriacaoDoBancoDeDados($creator);
+        try {
+            $dbh = new PDO(DB_DSN, DB_USERNAME, DB_PASSWORD, array( PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION ));


Que tal passar essa instancia do PDO via parametro?

malukenho · 2017-06-19T05:54:38Z

src/DAO/GenericDAO.php

+    public function criaBancoDeDados(IDatabaseCreator $creator)
+    {
+        $script = $this->geraScriptDeCriacaoDoBancoDeDados($creator);
+        try {


Remove the try, let it fail instead.

malukenho · 2017-06-19T05:55:25Z

src/DAO/IGenericDAO.php

+     * Retorna script de criação do banco de dados.
     */
-    public function generate(IDataBaseCreator $creator, $create = false);
+    public function geraScriptDeCriacaoDoBancoDeDados(IDataBaseCreator $creator);


English or Portuguese? let's avoid mix languages in here.

malukenho · 2017-06-19T05:55:52Z

src/Entity/EntidadeBase.php

+
+    public function getClassShortName()
+    {
+        return (new \ReflectionClass($this))->getShortName();


what about proxy this ReflectionClass?

malukenho · 2017-06-19T05:56:20Z

src/Factory/AppFactory.php

    }
-}
+
+}


malukenho · 2017-06-19T05:58:48Z

src/DAO/GenericDAO.php

     * {@inheritDoc}
     */
-    public function generate(IDataBaseCreator $creator, $create = false)
+    public function geraScriptDeCriacaoDoBancoDeDados(IDatabaseCreator $creator)


can this method be removed? it doesn't do too much now...

malukenho · 2017-06-19T05:59:10Z

src/DAO/IGenericDAO.php

    /**
-     * Gera script de criação do banco de dados e permite a criação automatica.
-     * @param bolean $create True to create database automatically | False To print script in screen
+     * Retorna script de criação do banco de dados.


malukenho · 2017-06-19T05:59:27Z

src/DAO/IGenericDAO.php

+    /**
+     * Cria banco de dados
+     */
+    public function criaBancoDeDados(IDataBaseCreator $creator);


@return void?

malukenho · 2017-06-19T05:59:33Z

src/Entity/EntidadeBase.php

        }
    }
+
+    public function getClassShortName()


malukenho · 2017-06-19T06:00:18Z

src/Factory/AppFactory.php

-        } catch (Exception $e) {
-            throw new Exception($e->getMessage());
-        }
+        $repository = '\\PseudoORM\\DAO\\' . $objeto->getClassShortName() . 'DAO';


Wouldn't it be in a config/map instead?

jackmakiyama · 2017-06-20T01:49:55Z

Apenas como registro do que o @malukenho falou no Telegram:

Não tem porque dá merge em uma PR sem ter "terminado" ou deixar pra depois fazer melhor. Temos algumas boas razões pra isso:

Se trata de um projeto educacional, não tendo necessidade de atender a clientes, PO, sprints, etc... Então, a PR pode esperar até que esteja 100% para ser integrada a base de código principal.
Justamente por se tratar de um projeto educational, a liberdade de experimentar/comentar/compartilhar/etc... deve ser exaltada e discutida excesivamente até que todos entrem em um minimo acordo, ou alguma das partes cedam
Uma contra PR pode ser feita ad hoc como uma forma opcional a essa questão de fazer "micro" PRs
"Tá passando nos testes" não quer dizer nada $this->assertTrue(true); sempre passa, e não testa nada.
Ainda falta o minimo de ajustes em CS
Bom, nada ha se levar a sério, é só o que eu penso sobre o merge precoce

Removendo contantes de configuração.

c6b06c2

Foi removido as contantes que tinha caminhos de pastas fisicos e no seu lugar foi adicionado na AppFactory as namespace hardcoded, gerando um debito tecnico, mas removendo essas contantes.

enieber reviewed Dec 29, 2016

View reviewed changes

EvandroMohr approved these changes Dec 29, 2016

View reviewed changes

Merge branch 'master' into feature/removes-config-contants

ffc48f9

malukenho suggested changes May 10, 2017

View reviewed changes

malukenho reviewed May 10, 2017

View reviewed changes

Ajustando classe com sugestoes do @malukenho

f319875

EvandroMohr added 6 commits June 18, 2017 13:12

Refatorando para separar responsabilidades

859d7a5

Refatorando para separar responsabilidades, corrigindo chamada de mét…

17c9a30

…odos

Refatorando método de geração do banco de dados para 2 métodos difere…

483a99c

…ntes, removendo parâmetro booleano

Update check-environment.phpt

949e131

Marcando como @todo para remover isso mais pra frente

5ab5ae5

Update check-environment.phpt

0c83bb4

malukenho suggested changes Jun 19, 2017

View reviewed changes


		$repository = '\\PseudoORM\\DAO\\' . $entityName . 'DAO';

		return new $repository($class);


		$classDAO = '\\PseudoORM\\DAO\\' . $classShortName . 'DAO';

		$entityName = class_exists($classDAO)

                   }
-              }
+              }

Fixes #35: Removendo contantes de configuração. #37

Are you sure you want to change the base?

Fixes #35: Removendo contantes de configuração. #37

Uh oh!

Conversation

jackmakiyama commented Dec 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

malukenho commented May 10, 2017

Uh oh!

jackmakiyama commented May 10, 2017

Uh oh!

EvandroMohr commented Jun 18, 2017

Uh oh!

malukenho commented Jun 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackmakiyama commented Jun 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants