C'è mai un motivo per fare tutto il lavoro di un oggetto in un costruttore?

48

Consentitemi di premettere questo dicendo che questo non è il mio codice né il codice dei miei colleghi. Anni fa, quando la nostra azienda era più piccola, avevamo alcuni progetti di cui avevamo bisogno, di cui non avevamo la capacità, quindi sono stati esternalizzati. Ora, non ho nulla contro l'outsourcing o gli appaltatori in generale, ma il codice base che hanno prodotto è una massa di WTF. Detto questo, funziona (soprattutto), quindi suppongo che sia nella top 10% dei progetti in outsourcing che ho visto.

Poiché la nostra azienda è cresciuta, abbiamo cercato di portare più del nostro sviluppo in casa. Questo particolare progetto mi è caduto sulle ginocchia, quindi ho dovuto esaminarlo, ripulirlo, aggiungere test, ecc.

C'è un modello che vedo ripetuto molto e sembra così terribilmente terribile che mi sono chiesto se forse c'è una ragione e io semplicemente non la vedo. Il modello è un oggetto senza metodi o membri pubblici, solo un costruttore pubblico che fa tutto il lavoro dell'oggetto.

Ad esempio, (il codice è in Java, se è importante, ma spero che questa sia una domanda più generale):

public class Foo {
  private int bar;
  private String baz;

  public Foo(File f) {
    execute(f);
  }

  private void execute(File f) {
     // FTP the file to some hardcoded location, 
     // or parse the file and commit to the database, or whatever
  }
}

Se ti stai chiedendo, questo tipo di codice viene spesso chiamato nel modo seguente:

for(File f : someListOfFiles) {
   new Foo(f);
}

Ora, mi è stato insegnato molto tempo fa che gli oggetti istanziati in un ciclo sono generalmente una cattiva idea e che i costruttori dovrebbero fare un minimo di lavoro. Guardando questo codice sembra che sarebbe meglio abbandonare il costruttore e rendere execute un metodo statico pubblico.

Ho chiesto all'appaltatore perché è stato fatto in questo modo, e la risposta che ho ricevuto è stata "Possiamo cambiarlo se vuoi". Il che non è stato di grande aiuto.

Ad ogni modo, c'è sempre un motivo per fare qualcosa del genere, in qualsiasi linguaggio di programmazione, o è solo un'altra sottomissione al Daily WTF?

    
posta Kane 02.08.2012 - 15:36
fonte

12 risposte

59

Ok, andando in fondo alla lista:

Mi è stato insegnato molto tempo fa che gli oggetti istanziati in un ciclo sono generalmente una cattiva idea

Non in tutte le lingue che ho usato.

In C è una buona idea dichiarare le variabili in anticipo, ma è diverso da ciò che hai detto. Potrebbe essere leggermente più veloce se dichiari gli oggetti sopra il loop e li riutilizzi, ma ci sono molte lingue in cui questo aumento di velocità non ha senso (e probabilmente alcuni compilatori là fuori che fanno l'ottimizzazione per te :)).

In generale, se hai bisogno di un oggetto all'interno di un ciclo, creane uno.

I costruttori dovrebbero fare un minimo di lavoro

I costruttori dovrebbero istanziare i campi di un oggetto e fare qualsiasi altra inizializzazione necessaria per rendere l'oggetto pronto per l'uso. Questo in genere significa che i costruttori sono piccoli, ma ci sono scenari in cui questa sarebbe una notevole quantità di lavoro.

c'è sempre un motivo per fare qualcosa del genere, in qualsiasi linguaggio di programmazione, o è solo un'altra sottomissione al Daily WTF?

È una sottomissione al Daily WTF. Certo, ci sono cose peggiori che puoi fare con il codice. Il problema è che l'autore ha un grosso fraintendimento su quali sono le classi e su come usarle. In particolare, ecco quello che vedo che è sbagliato con questo codice:

  • Uso improprio delle classi: la classe agisce fondamentalmente come una funzione. Come hai detto, dovrebbe essere sostituito con una funzione statica, oppure la funzione dovrebbe essere implementata nella classe che la sta chiamando. Dipende da cosa fa e da dove viene utilizzato.
  • Sovraccarico delle prestazioni: a seconda della lingua, la creazione di un oggetto può essere più lenta rispetto al richiamo di una funzione.
  • Confusione generale: Generalmente il programmatore confonde come utilizzare questo codice. Senza vederlo usato, nessuno saprebbe come l'autore intendesse usare il codice in questione.
risposta data 02.08.2012 - 15:49
fonte
26

Per me, è un po 'una sorpresa quando si ottiene che un oggetto fa molto lavoro nel costruttore, quindi su questo suggerisco di non farlo (principio di minima sorpresa).

Un tentativo di buon esempio :

Non sono sicuro che questo utilizzo sia un anti-pattern, ma in C # ho visto il codice che era sulla falsariga di (un esempio un po 'inventato):

using (ImpersonationContext administrativeContext = new ImpersonationContext(ADMIN_USER))
{
    // Perform actions here under the administrator's security context
}
// We're back to "normal" privileges here

In questo caso, la classe ImpersonationContext fa tutto il suo lavoro nel costruttore e nel metodo Dispose. Si avvantaggia dell'istruzione C # using , che è abbastanza ben compresa dagli sviluppatori C # e garantisce quindi che il setup che si verifica nel costruttore venga eseguito il rollback una volta fuori dall'istruzione using , anche se un'eccezione si verifica. Questo è probabilmente il miglior utilizzo che ho visto per questo (cioè "lavoro" nel costruttore), anche se non sono ancora sicuro che lo considererei "buono". In questo caso, è abbastanza auto-esplicativo, funzionale e di successo.

Un esempio di modello valido e simile sarebbe il schema di comando . Sicuramente non esegui la logica nel costruttore, ma è simile nel senso che l'intera classe equivale a una chiamata al metodo (compresi i parametri necessari per invocare il metodo). In questo modo, le informazioni sull'invocazione del metodo possono essere serializzate o passate per un utilizzo successivo, il che è estremamente utile. Forse i tuoi appaltatori stavano tentando qualcosa di simile, anche se ne dubito strongmente.

Commenti sul tuo esempio:

Direi che è un anti-pattern molto preciso. Non aggiunge nulla, va contro ciò che ci si aspetta ed esegue un'elaborazione eccessivamente lunga nel costruttore (FTP nel costruttore? WTF, anzi, anche se suppongo che le persone siano conosciute per chiamare i servizi Web in questo modo).

Faccio fatica a trovare la citazione, ma il libro di Grady Booch "Object Solutions" ha un aneddoto su un team di sviluppatori C che passano al C ++. A quanto pare avevano frequentato un corso di formazione e alla dirigenza era stato detto che dovevano assolutamente fare cose "orientate agli oggetti". Portato a capire cosa c'è che non va, l'autore ha usato uno strumento di metrica del codice e ha scoperto che il numero medio di metodi per classe era esattamente 1, ed erano varianti della frase "Fallo". Devo dire, non ho mai incontrato questo particolare problema prima, ma a quanto pare può essere un sintomo di persone costrette a creare codice orientato agli oggetti, senza capire veramente come farlo, e perché.

    
risposta data 02.08.2012 - 16:05
fonte
13

No.

Se tutto il lavoro è fatto nel costruttore significa che l'oggetto non è mai stato necessario in primo luogo. Molto probabilmente, l'appaltatore stava semplicemente usando l'oggetto per la modularità. In tal caso, una classe non istanziata con un metodo statico avrebbe ottenuto lo stesso vantaggio senza creare istanze di oggetti superflue.

C'è solo un caso in cui è accettabile eseguire tutto il lavoro in un costruttore di oggetti. Questo è quando lo scopo dell'oggetto è specificamente quello di rappresentare un'identità unica a lungo termine, piuttosto che eseguire un lavoro. In tal caso, l'oggetto sarebbe tenuto in giro per ragioni diverse dall'eseguire un lavoro significativo. Ad esempio, un'istanza singleton.

    
risposta data 02.08.2012 - 19:32
fonte
6

Ho sicuramente creato molte classi che fanno molto lavoro per calcolare qualcosa nel costruttore, ma l'oggetto è immutabile, quindi rende disponibili i risultati di tale calcolo tramite le proprietà, e quindi non fa mai altro. È normale immutabilità in azione.

Penso che la cosa strana qui sia mettere il codice con effetti collaterali in un costruttore. Costruire un oggetto e buttarlo via senza accedere a proprietà o metodi sembra strano (ma almeno in questo caso è abbastanza ovvio che sta succedendo qualcos'altro).

Sarebbe meglio se la funzione fosse spostata su un metodo pubblico, e quindi avere un'istanza della classe iniettata, quindi fare in modo che il ciclo for richiami ripetutamente il metodo.

    
risposta data 02.08.2012 - 21:09
fonte
3

Is there ever a reason to do all an object's work in a constructor?

No.

  • Un costruttore non dovrebbe avere effetti collaterali.
    • Qualunque cosa in più dell'inizializzazione del campo privato dovrebbe essere vista come un effetto collaterale.
    • Un costruttore con effetti collaterali rompe il Principio di Responsabilità Singola (SRP) e funziona in modo contrario allo spirito della programmazione orientata agli oggetti (OOP).
  • Un costruttore dovrebbe essere leggero e non dovrebbe mai fallire.
    • Ad esempio, rabbrividisco sempre quando vedo un blocco try-catch all'interno di un costruttore. Un costruttore non dovrebbe lanciare eccezioni o errori di registro.

Si potrebbe ragionevolmente mettere in discussione queste linee guida e dire: "Ma io non seguo queste regole, e il mio codice funziona bene!" A questo, vorrei rispondere, "Beh, potrebbe essere vero, finché non lo è."

  • Le eccezioni e gli errori all'interno di un costruttore sono molto inaspettati. A meno che non gli venga detto di farlo, i futuri programmatori non saranno inclini a circondare queste chiamate del costruttore con un codice difensivo.
  • Se qualcosa non funziona, la traccia dello stack generata può essere difficile da analizzare. La parte superiore della traccia dello stack potrebbe puntare alla chiamata del costruttore, ma molte cose accadono nel costruttore e potrebbe non puntare alla LOC effettiva che ha fallito.
    • Ho analizzato molte tracce dello stack .NET dove questo era il caso.
risposta data 09.09.2012 - 17:13
fonte
2

Mi sembra che la persona che sta facendo questo stia cercando di aggirare la limitazione di Java che non consente di passare le funzioni in giro come cittadini di prima classe. Ogni volta che devi passare una funzione, devi inserirla in una classe con un metodo come apply o simile.

Quindi penso che abbia appena usato una scorciatoia e usato direttamente una classe come sostituto di una funzione. Ogni volta che si desidera eseguire la funzione, basta istanziare la classe.

Non è sicuramente un buon modello, almeno perché stiamo cercando di capirlo, ma immagino che potrebbe essere il modo meno verboso di fare qualcosa di simile alla programmazione funzionale in Java.

    
risposta data 02.08.2012 - 16:52
fonte
1

Un caso mi viene in mente dove il costruttore / distruttore fa tutto il lavoro:

Blocchi. Normalmente non interagisci mai con un lucchetto durante la sua vita. L'uso dell'architettura di C # è utile per gestire tali casi senza fare esplicito riferimento al distruttore. Tuttavia, non tutti i blocchi sono implementati in questo modo.

Ho anche scritto quello che equivaleva a un bit di codice del costruttore per correggere un bug di libreria di runtime. (In realtà, la correzione del codice avrebbe causato altri problemi.) Non c'era un distruttore perché non era necessario annullare la patch.

    
risposta data 26.09.2014 - 01:25
fonte
1

Per me la regola empirica non è fare nulla nel costruttore, tranne che per inizializzare le variabili membro. La prima ragione è che aiuta a seguire il principio SRP, in quanto solitamente un lungo processo di inizializzazione è un'indicazione che la classe fa più di quanto dovrebbe fare e l'inizializzazione dovrebbe essere eseguita da qualche parte al di fuori della classe. La seconda ragione è che in questo modo vengono passati solo i parametri necessari, quindi si crea un codice meno accoppiato. Un costruttore con un'inizializzazione complicata di solito ha bisogno di parametri che sono usati solo per costruire un altro oggetto, ma che non sono usati dalla classe originale.

    
risposta data 02.08.2012 - 17:03
fonte
1

Qui andrò contro corrente - nelle lingue dove tutto deve essere in una classe E non puoi avere una classe statica, puoi imbatterti nella situazione in cui hai un po 'di funzionalità isolata che deve essere messo da qualche parte e non si adatta a nient'altro. Le tue scelte sono una classe di utilità che copre vari frammenti di funzionalità non correlati, o una classe che fondamentalmente fa proprio questa cosa.

Se hai solo un bit come questo, la tua classe statica è la tua classe specifica. Quindi, o hai un costruttore che fa tutto il lavoro, o una singola funzione che fa tutto il lavoro. Il vantaggio di fare tutto nel costruttore è che accade solo una volta, ti permette di usare campi privati, proprietà e metodi senza preoccuparti che vengano riutilizzati o thread. Se hai un costruttore / metodo potresti essere tentato di far passare i parametri al singolo metodo, ma se utilizzi campi privati o proprietà che presentano potenziali problemi di threading.

Anche con una classe di utilità, molte persone non pensano di avere classi statiche nidificate (e questo potrebbe non essere possibile a seconda della lingua).

Fondamentalmente questo è un modo relativamente comprensibile di isolare il comportamento dal resto del sistema, all'interno di ciò che è consentito dalla lingua, consentendo comunque di sfruttare il maggior numero possibile di linguaggio.

Francamente avrei risposto molto come il tuo appaltatore, è un piccolo aggiustamento per trasformarlo in due chiamate, non c'è molto da raccomandare l'uno sull'altro. Quali dis / advatanges ci sono, sono probabilmente più ipotetici che reali, perché provare a giustificare la decisione quando non ha importanza e probabilmente non è stata tanto decisa quanto l'azione predefinita?

    
risposta data 09.09.2012 - 21:37
fonte
1

Ci sono schemi comuni nei linguaggi in cui funzioni e classi sono molto più la stessa cosa, come Python, in cui si usa fondamentalmente un oggetto per funzionare con troppi effetti collaterali o con più oggetti con nome restituiti.

In questo caso, il grosso, se non tutto il lavoro può essere fatto nel costruttore, poiché l'oggetto stesso è solo un involucro attorno a un singolo fascio di lavoro, e non vi è alcuna buona ragione per inserirlo in un separato funzione. Qualcosa come

var parser = XMLParser()
var x = parser.parse(string)
string a = x.LookupTag(s)

in realtà non è migliore di

 var x = ParsedXML(string)
 string a = x.LookupTag(s)

Invece di avere direttamente gli effetti collaterali a titolo definitivo, puoi chiamare l'oggetto per applicare l'effetto collaterale sulla stecca, che ti dà una migliore visibilità. Se avrò un brutto effetto collaterale su un oggetto, potrò fare tutto il mio lavoro e poi farmi passare l'oggetto che farò male e farmi del male. Identificare l'oggetto e isolare la chiamata in questo modo è più chiaro che passare più oggetti di questo tipo in una funzione contemporaneamente.

x.UpdateView(v)

È possibile effettuare più valori di ritorno tramite i metodi di accesso all'oggetto. Tutto il lavoro è fatto, ma voglio le cose tutte nel loro contesto, e non voglio davvero passare più riferimenti nella mia funzione.

var x = new ComputeStatistics(inputFile)
int max = x.MaxOptions;
int mean = x.AverageOption;
int sd = x.StandardDeviation;
int k = x.Kurtosis;
...

Quindi, come programmatore congiunto di Python / C #, questo non mi sembra affatto strano.

    
risposta data 25.09.2014 - 19:46
fonte
-1

Sono d'accordo con AndyBursh. Sembra un problema di scarsa conoscenza.

Tuttavia è importante citare framework come NHibernate che richiedono solo il codice nel costruttore (quando si creano le classi di mappe). Tuttavia questa non è una limitazione del framework, è solo ciò di cui hai bisogno. Quando si tratta di riflessione, il costruttore è l'unico metodo che esisterà sempre (anche se potrebbe essere privato) e questo potrebbe essere utile se devi chiamare un metodo di una classe in modo dinamico.

    
risposta data 03.08.2012 - 01:16
fonte
-4

"I was taught long ago that instantiated objects in a loop is generally a bad idea"

Sì, forse stai ricordando perché abbiamo bisogno di StringBuilder.

Ci sono due scuole di Java.

Il costruttore è stato promosso da primo , che ci insegna a riutilizzare (perché condivisione = identità in efficienza). Tuttavia, all'inizio del 2000 ho iniziato a leggere che creare oggetti in Java è così economico (al contrario di C ++) e GC è così efficiente che il riutilizzo è inutile e l'unica cosa che conta è la produttività del programmatore. Per la produttività, deve scrivere codice gestibile, il che significa modularizzazione (ovvero restringere l'ambito). Così,

questo è male

byte[] array = new byte[kilos or megas]
for_loop:
  use(array)

questo è buono

for_loop:
  byte[] array = new byte[kilos or megas]
  use(array)

Ho posto questa domanda quando è esistito forum.java.sun e le persone hanno convenuto che preferirebbero dichiarare la matrice il più stretta possibile.

Il moderno programmatore java non esita mai a dichiarare una nuova classe o creare un oggetto. È incoraggiato a farlo. Java offre tutte le ragioni per farlo, con la sua idea che "tutto è un oggetto" e una creazione economica.

Inoltre, lo stile di programmazione aziendale moderno è, quando è necessario eseguire un'azione, creare una classe speciale (o meglio ancora una struttura) che eseguirà questa semplice azione. Buoni IDE Java, che aiutano qui. Generano automaticamente tonnellate di schifezze, come la respirazione.

Quindi, penso che ai tuoi oggetti manchi il modello Builder o Factory. Non è corretto creare istanze direttamente, per costruttore. Si consiglia una classe di produzione speciale per ogni oggetto.

Ora, quando entra in gioco la programmazione funzionale, la creazione di oggetti diventa ancora più semplice. Creerai oggetti ad ogni passo come respirando, senza nemmeno accorgertene. Quindi, mi aspetto che il tuo codice diventerà ancora più "efficiente" nella nuova era

"do not do anything in your constructor. It is for initialization only"

Personalmente, non vedo alcun motivo nelle linee guida. Lo considero solo un altro metodo. Il codice di factoring è necessario solo quando è possibile condividerlo.

    
risposta data 09.09.2012 - 11:14
fonte