Il metodo statico pubblico chiama il costruttore privato

(Risposta riscritta alla luce del commento, " Scusa, avrei dovuto menzionare che ho fatto un esempio molto semplice per mostrare l'elemento principale del design che mi ha confuso. Il metodo InsertAudit chiamava diverse altre istanze private metodi. ". Questo chiarimento cambia l'approccio statico da" profondamente confuso e confuso "ad essere una soluzione praticabile per evitare di aggirare i parametri.)

Dall'esempio che fornisci, il codice del tuo collega è profondamente confuso e confuso. Tuttavia, con la tua precisazione che il metodo InsertAudit chiamerebbe diversi altri metodi di istanze private, diventa molto più chiaro cosa sta succedendo. Tutto il codice statico che mostri può essere ridotto a quanto segue senza alcun cambio di funzione:

public class AuditInserter
{
    public static void Insert(DataContext dataContext,
                              Person person,
                              AuditableAction action)
    {
        dataContext.AuditTable.Insert(person, action);
    }
}

Ma rivisitiamo quel codice e aggiungiamo alcuni metodi privati per renderlo più rappresentativo del codice reale. Mi sono permesso di usare le funzionalità di sintassi in C # 7 per ridurre la dimensione del codice per brevità):

public class AuditInserter
{
    public static void Insert(DataContext dataContext,
                              Person person,
                              AuditableAction action)
        => new AuditInserter(dataContext, person).InsertAudit(action);

    private AuditInserter(DataContext dataContext, Person person)
        => (_dataContext, _person) = (dataContext, person);

    private readonly _dataContext;
    private readonly _person;

    private void InsertAudit(AuditableAction action)
    {
        DoSomethingFirst();
        DoTheInsert(action);
        DoSomethingAfter();
    }

    private void DoSomethingFirst()
        => // do something with _person and _dataContext here

    private void DoTheInsert(AuditableAction action)
        => _dataContext.AuditTable.Insert(_person, action);

    private void DoSomethingAfter()
        => // do something with _person and _dataContext here
}

Anche in questo caso il codice può essere semplificato da un punto di vista semplicemente rendendolo tutto statico: possiamo liberarci del costruttore e dei campi. Ma aumentiamo la complessità in un altro aspetto: ora i metodi hanno più parametri per consentire il passaggio di questi valori:

public class AuditInserter
{
    public static void Insert(DataContext dataContext,
                              Person person,
                              AuditableAction action)
    {
        DoSomethingFirst(dataContext, person);
        DoTheInsert(dataContext, person, action);
        DoSomethingAfter(dataContext, person);
    }

    private void DoSomethingFirst(DataContext dataContext,
                                  Person person)
        => // do something with person and dataContext here

    private void DoTheInsert(DataContext dataContext,
                             Person person,
                             AuditableAction action)
        => dataContext.AuditTable.Insert(person, action);

    private void DoSomethingAfter(DataContext dataContext,
                                  Person person)
        => // do something with person and dataContext here
}

Quale approccio prendi è una miscela di preferenze personali e quanti parametri hai nel vero esempio. Solo per questi due elementi, rimango con l'approccio completamente statico. Se fosse aumentato a cinque, sei o più, probabilmente adotterei qualcosa di simile a quello di un ex sviluppatore. Altre persone avranno altre preferenze personali e potrebbero anche solo adottare quel metodo di memorizzazione dei parametri per tutti i casi (come sembra aver fatto il tuo ex sviluppatore). La tua versione del codice si comporta in modo abbastanza diverso rispetto alla versione statica. Questo può essere mostrato cambiando il Main in:

public class MainProgram
{
    public static void Main()
    {
        var inserter = new AuditInserter(new DataContext, new Person);
        inserter.Insert(new AuditableAction);
        inserter.Insert(new AuditableAction);
    }
}

Ora stiamo usando lo stesso AuditInserter per inserire due azioni. Questa potrebbe essere una buona cosa, nel qual caso potresti passare ad usare il tuo approccio se trovi più facile lavorare con. Potrebbe anche essere un problema, nel qual caso lasciare le cose come sono (o passare ad un approccio completamente statico).

Una cosa su cui possiamo essere certi (a condizione che tu sveli altri importanti dettagli che rimossi per errore durante la semplificazione dell'esempio di codice) è che questo modello non è interamente correlato alle fabbriche.

Questo potrebbe essere un esempio del schema di fabbrica .

Puoi vedere piccole, ma significative differenze tra la tua implementazione e l'attuale implementazione: Nella tua implementazione è del tutto possibile creare un AuditInserter senza che contenga uno (e solo uno) AuditableAction .

• Nulla ti impedirà di omettere quel Insert(new AuditableAction);
• Inoltre niente ti impedirà di chiamare ripetutamente Insert() sullo stesso AuditInserter .

Ovviamente, puoi archiviare lo stesso richiedendo semplicemente AuditableAction come parte del costruttore per AuditInserter , ma a volte sono motivi per cui non desideri farlo (ad es. la funzione Insert() può lanciare un'eccezione, ma si desidera evitare il costruttore di AuditInserter per lanciarne uno, oppure è necessario fare più cose con l'oggetto appena costruito che non appartiene al costruttore e / o deve essere eseguito in un certo ordine ).

Allo stesso modo, potrebbero esserci diversi sapori / sottotipi di AuditInserter e la funzione di fabbrica può fornire il tipo corretto in base ai tuoi input (ad esempio FinancialAuditInserter se il AuditableAction è relegato alle finanze o a HrAuditInserter se è correlato all'ora, ecc ...).

Alla fine, tutto dipende da come viene utilizzato - ogni modello di progettazione può essere abusato, e solo nel contesto dell'uso nell'applicazione è possibile determinare se è ragionevole o meno.

Il design è un po 'confuso. L'obiettivo era nascondere l'implementazione e la fase di creazione dell'oggetto. Ma l'implementazione confonde gli sviluppatori in quanto l'obiettivo non è chiaro. Ti consiglio di utilizzare Metodi di estensione , che è un approccio più semplice e conosciuto. Non è necessario introdurre un ulteriore livello di complessità che è la classe "AuditInserter".

Il metodo può apparire come

public static class DataContextExtension
{
    public static void InsertAudit(this DataContext context, Person person, AuditableAction action)
    {
        context.AuditTable.Insert(person, action);
    }
}

E l'utilizzo può essere simile a

public class MainProgram
{
    public static void Main()
    {
        new DataContext().InsertAudit(
        new Person(),
        new AuditableAction());
    }
}

Sono d'accordo sul fatto che l'uso di un metodo statico in questo modo sia fonte di confusione e che l'IMO debba essere rielaborato o almeno scoraggiato dal passare a un nuovo codice. Userò metodi statici di fabbrica su classi di contenitori di dati dove voglio assicurarmi che la classe sia costruita in uno stato valido, mai con qualcosa del genere in cui ci siano dipendenze. Sebbene sia possibile cablare contenitori IoC per utilizzare i delegati per richiamare metodi statici, ciò vanifica in parte alcuni vantaggi dell'utilizzo di un contenitore. (codifica extra)

Ad esempio se ho un DTO di tipo "risultato" che avrebbe due stati che rappresentano una chiamata riuscita, o uno fallito (con dettagli di errore di convalida) userò comunemente un costruttore privato con un metodo factory statico di successo per inizializzare un istanza con i risultati o un metodo statico Failure per inizializzare un'istanza con i motivi dell'errore. Semplici contenitori di dati possono fungere da fabbrica, oppure è possibile utilizzare un modello di fabbrica più puro, ma le classi non possono essere "bloccate" abbastanza facilmente per garantire che rappresentino sempre uno stato valido per il loro scopo.