Ci sono troppi parametri in questo costruttore? [duplicare]

Non c'è niente di gravemente sbagliato nel primo pezzo di codice. Sebbene avere più di mezza dozzina di dipendenze possa indicare una violazione dell'SRP, non c'è nulla di intrinsecamente sbagliato nell'avere un grande costruttore.

In effetti, preferirei di gran lunga vedere un costruttore con un numero elevato di argomenti rispetto a un metodo. I costruttori sono per ottenere le nostre anatre in fila in modo che la nostra classe possa fare ciò che deve fare.

Il tuo codice sostitutivo è in realtà più pericoloso in quanto nasconde le vere dipendenze della classe e richiede che venga costruito un altro oggetto solo per poter costruire questo oggetto.

Quanto è più semplice il tuo codice rispetto al primo?

Che aspetto del costruttore di Magento nel tuo esempio? Avrebbe preso gli stessi parametri del costruttore effettivo dal primo pezzo di codice, e l'unica differenza sarebbe che invece di una classe, ora hai due classi.

Il refactoring dovrebbe migliorare il codice, non spostare la parte difficile in una classe diversa.

D'altra parte, se si raggruppano insieme argomenti correlati logicamente e li si sostituisce con una classe, può avere senso. Guardando il primo pezzo di codice, non vedo nessun candidato per il raggruppamento. Il codice sembra abbastanza leggibile.

Il primo approccio che hai menzionato è assolutamente perfetto e non c'è bisogno di refactoring del codice. L'iniezione delle dipendenze è molto utile per scrivere casi di test sulla tua classe. Perché puoi facilmente prendere in giro la funzionalità di classe iniettata mentre stai scrivendo i casi di test secondo le tue necessità.

Il secondo approccio che hai menzionato è sbagliato. Perché stai passando l'intero oggetto framework Magento alla tua classe. Ciò significa che stai esponendo tutte le funzionalità indesiderate all'interno della tua classe (nessuna incapsulazione!).

Se stai pensando di passare le variabili a questa classe ti suggerirò sicuramente di creare una classe wrapper per questo. Ma qui stai passando gli oggetti quindi non c'è bisogno di una classe wrapper. Se lo facciamo, aumenterà la complessità del tuo codice.

Avere una classe base con 6 dipendenze lo sta spingendo ma è accettabile in alcune circostanze, specialmente se è raro dover estendere la classe. Il framework PHP Symfony 2 ha una classe di sicurezza con un sacco di dipendenze. L'estensione è doloroso, ma è necessario farlo solo quando si implementa un sistema di autenticazione completamente nuovo.

D'altra parte, l'esempio di Magento sembra più simile a una classe di costrutto e si assume che l'estensione sia abbastanza frequente. Symfony 2 ha anche una classe controller di base con metodi utili come getUser, createForm, render ecc. La classe base impiegherebbe circa 8 dipendenze nel suo costruttore. Sarebbe molto doloroso e molto fragile se uno sviluppatore dovesse supportare diverse dozzine di classi di controller nella propria applicazione e tenere traccia di tutte queste dipendenze. Ci sarebbero anche problemi di prestazioni in quanto tutte queste dipendenze dovrebbero essere create (o almeno proxy) per ciascun controller.

Quindi Symfony 2 fa qualcosa che penso funzioni bene. Inietta il contenitore di iniezione delle dipendenze e lo utilizza per accedere a questi servizi di supporto secondo necessità. Allo stesso tempo, il costruttore è riservato ai servizi specifici del controllore. Quindi hai qualcosa come:

class PersonController extends BaseController
{
    private $container; // Actually in BaseController
    private $personRepository;

    // Also in base controller
    public function setContainer($container) { $this->container = $container; }

    // Nice easy constructor
    public function __construct($personRepository)
    {
        $this->personRepository;
    }

    // An action method
    public function showPersonAction($request)
    {
        $person = $this->personRepository->find($request->get('id'));

        // render uses a templating service from the container
        return $this->render(['person' => $person],'person.template.html');
    }
}
$controller = new PersonController($container->get('person_repository');
$controller->setContainer($container);
$response = $controller->showPersonAction($request);

Penso che questo sia un buon compromesso. Nessun grande costruttore. Facile da estendere e leggere. È necessario ricordare di iniettare il controller, ma di solito viene gestito dal controller anteriore.

Sembra che il tuo software possa trarre vantaggio da Dependency Injection. In questo modo, il costruttore genitore potrebbe avere le sue dipendenze fornite direttamente ad esso senza dover gravare su tutte le classi figlie con costruttori che fanno poco più che passare argomenti finché non raggiungono il costruttore che effettivamente ne ha bisogno.