PHPMT / PseudoORM

App de persistência para fins didáticos
GNU General Public License v2.0
12 stars 7 forks source link

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

Open jackmakiyama opened 7 years ago

jackmakiyama commented 7 years ago

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.

malukenho commented 7 years ago

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

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 commented 7 years ago

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 commented 7 years ago

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

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

malukenho commented 7 years ago

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.

jackmakiyama commented 7 years ago

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:

  1. 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.

  2. 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

  3. Uma contra PR pode ser feita ad hoc como uma forma opcional a essa questão de fazer "micro" PRs

  4. "Tá passando nos testes" não quer dizer nada $this->assertTrue(true); sempre passa, e não testa nada.

  5. 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