Le persone spesso si concentrano sugli odori del codice nelle implementazioni. Tuttavia, penso che dobbiamo anche esaminare gli odori nelle nostre astrazioni: questi sono spesso visti meglio illustrando consumo / utilizzo piuttosto che concentrarsi esclusivamente sull'implementazione.

Non hai mostrato l'utilizzo nel contesto.

@Ewan sta relazionando sul fatto che questo tipo di codice è visto nella persistenza, in particolare nella deserializzazione. In tal caso, l'obiettivo dell'astrazione dovrebbe essere quello di associare sia la serializzazione che la deserializzazione insieme in modo che solo una parte di codice (come suggerisce @Ewan, il repository di dati) debba conoscere il mapping tra stringa e sottoclasse. Questo per cui non abbiamo più pezzi separati di codice da qualche parte ciascuno usando la propria copia della stringa "U", per esempio.

Tuttavia, è anche possibile che tu stia facendo qualcos'altro, come chiamare ... makeShip("U"); . Se questo è il caso, e stai cercando di evitare di chiamare direttamente new UFO() , allora potresti forse creare un metodo di produzione separato dedicato alla creazione di tipi di UFO in modo da non dover passare la "U" in un posto e poi testare per "U" in un altro.

Ci sono un certo numero di cose che puoi fare per rendere questo un po 'meglio:

• Utilizza un'istruzione switch anziché clausole else-if concatenate
• O forse ancora meglio, usa un dizionario di shipType alla funzione generatore.
• Modifica shipType da una stringa a un enum.

Ma sono tutte piccole modifiche, e hai ragione che questo sembra un odore di codice. È impossibile dare altri consigli senza vedere il contesto più ampio, ma sospetterei una delle due cose:

1. Il pattern factory è stato forzato qui dove non è appropriato.
2. La factory ha bisogno di più contesto per prendere una decisione.

Sì, per me è un odore di codice. Un grande se blocco che associa le stringhe magiche alle classi hard coded.

Ogni volta che vedi qualcosa di simile in genere perché stai generando oggetti da una qualche forma di dati persistenti, un database o un file da qualche parte ha tipo nave: "U", velocità: 12, armatura: 6 e vuoi creare un'istanza della classe appropriata.

Tuttavia, questa mappatura dovrebbe essere incapsulata nel tuo repository di dati. Fa parte della codifica dei dati in quel formato specifico. Non dovresti vedere queste classi di fabbrica che galleggiano intorno alla tua app.

Inoltre, con nella classe repository dovresti evitare le stringhe magiche nominando le tue tabelle o digita le stringhe come il nome della classe.

Puoi anche evitare l'istruzione if con una mappa di nomi di classi noti a funzioni che popolano una nuova istanza della classe corretta con i dati. (essenzialmente uguale a una fabbrica che conosco, ma più ordinato)

Prima di iniziare devo dire che questo esempio probabilmente non esisterà mai in quella forma nell'app reale (almeno non dovrebbe). Ecco perché (IMHO):

1) Dirigi direttamente l'implementazione dal codice cliente:

Prima di tutto si passa una stringa per rendere il metodoEnemyShip - è pratica prettamente errata passare le informazioni sul tipo come una stringa MA la cosa triste di quell'esempio è che il tuo input è una sorta di indicatore diretto per l'implementazione. Intendo dire che il tuo shipType sta puntando all'implementazione di EnemyShip di un rapporto uno a uno tra le stringhe di input e le classi di spedizione.

Se ottieni argomenti di input dal "mondo esterno" (DB, API, interfaccia utente, ecc.), potrebbe essere ragionevole fornire una sorta di mappatura o una semplice fabbrica (come questa dal tuo esempio) che seleziona il metodo di produzione corretto (o basta istanziare una classe specifica in base a "input esterno".

class PaymentProvider {
  private creators: { [key: string]: PaymentFactoryMethod };
  // ...
  public getPayment(paymentSignature: string): Payment | undefined {
    const createPayment = this.creators[paymentSignature];
    return createPayment ? createPayment() : undefined;
  }
}

MA se vuoi fornire questo stabilimento per uso generale, Suppongo che potresti finire per passarlo a uno dei tuoi corsi. Quindi potresti voler fare qualcosa del tipo:

EnemyShip enemy = factory.makeEnemyShip("U")
// or what's worse:
this.enemy = this.factory.makeEnemyShip("U")

Per me questo codice viola una delle regole più importanti del pattern factory:
Lascia che il codice client non sia a conoscenza dell'implementazione fornita dalla fabbrica.

Tuttavia definire la fabbrica in questo modo ha ancora dei vantaggi, penso che il principale sia l'incapsulamento della logica che sta dietro la creazione della nave nemica (potrebbe essere complessa).

A mio parere, in generale, è meglio passare fabbrica come dipendenza e utilizzare il metodo Factory Method o Abstract Factory, e ciò che è più importante quando si tratta di fabbriche considerare se ne hai davvero bisogno.

2) Il tuo codice viola il principio di apertura / chiusura

Come hai notato, questo codice ha predefinito tutte le possibili implementazioni di EnemyShip - che è spesso (anche molto spesso - ma non sempre) una cattiva idea. Perché:

• è difficile aggiungere un nuovo tipo - devi refactoring questo codice
• non è dinamico

Correzione per questo problema di istruzione "if" codificato è Map o Dictionary. Al momento non è possibile aggiungere nuove implementazioni durante il runtime - Se si disponesse di un dizionario (o di una mappa) che associa tipi o predicati a particolari classi, si registrerebbe facilmente e si annullerebbero la registrazione di nuove fabbriche. TUTTAVIA Avresti comunque bisogno di un posto per configurare questo stabilimento.

Per riassumere

Penso che l'esempio che hai fornito sia una sorta di materiale didattico che impara a sbarazzarsi della parola chiave new piuttosto che fornisce un vero e proprio modello di app utile.