Quanto difensivamente dovrei programmare? [chiuso]

StackOverflow https://stackoverflow.com/questions/1053215

Domanda

stavo lavorando con una piccola routine che viene utilizzata per creare una connessione al database:

Prima 

public DbConnection GetConnection(String connectionName)
{
   ConnectionStringSettings cs= ConfigurationManager.ConnectionStrings[connectionName];
   DbProviderFactory factory = DbProviderFactories.GetFactory(cs.ProviderName);
   DbConnection conn = factory.CreateConnection();
   conn.ConnectionString = cs.ConnectionString;
   conn.Open();

   return conn;
}

Poi ho iniziato a esaminare la documentazione di .NET framework, per vedere quali sono i comportamenti documentati di varie cose e vedere se riesco a gestirli.

Ad esempio: 

ConfigurationManager.ConnectionStrings...

La documentazione dice che chiamando < strong> ConnectionStrings genera un ConfigurationErrorException se non è stato possibile recuperare la raccolta. In questo caso non c'è niente che io possa fare per gestire questa eccezione, quindi la lascerò andare.

La parte successiva è l'indicizzazione effettiva di ConnectionStrings per trovare connectionName : 

...ConnectionStrings[connectionName];

In questo caso la Documentazione ConnectionStrings afferma che la proprietà restituirà null se non è stato possibile trovare il nome della connessione. posso verificare che ciò accada e lanciare un'eccezione per consentire a qualcuno in alto di aver fornito una connessione non valida Nome: 

ConnectionStringSettings cs= 
      ConfigurationManager.ConnectionStrings[connectionName];
if (cs == null)
   throw new ArgumentException("Could not find connection string \""+connectionName+"\"");

ripeto lo stesso esercizio con: 

DbProviderFactory factory = 
      DbProviderFactories.GetFactory(cs.ProviderName);

Il metodo GetFactory non ha documentazione su cosa succede se una fabbrica per il ProviderName specificato non è stato trovato. Non è documentato per la restituzione di null, ma posso comunque essere difensivo e controllare per null: 

DbProviderFactory factory = 
      DbProviderFactories.GetFactory(cs.ProviderName);
if (factory == null) 
   throw new Exception("Could not obtain factory for provider \""+cs.ProviderName+"\"");

La prossima è la costruzione dell'oggetto DbConnection: 

DbConnection conn = factory.CreateConnection()

Ancora una volta la documentazione non dire cosa succede se non è possibile creare una connessione, ma di nuovo posso verificare la presenza di un oggetto null return: 

DbConnection conn = factory.CreateConnection()
if (conn == null) 
   throw new Exception.Create("Connection factory did not return a connection object");

Il prossimo è impostare una proprietà dell'oggetto Connection: 

conn.ConnectionString = cs.ConnectionString;

I documenti non dicono cosa succede se non è possibile impostare la stringa di connessione. Fa un'eccezione? Lo ignora? Come nella maggior parte delle eccezioni, se si è verificato un errore durante il tentativo di impostare ConnectionString di una connessione, non è possibile eseguire alcuna operazione per ripristinarla. Quindi non farò nulla.

E infine, aprendo la connessione al database: 

conn.Open();

Il Metodo aperto di DbConnection è astratto, quindi spetta a qualunque provider discenda da DbConnection decidere quali eccezioni generano. Inoltre, non c'è alcuna guida nella documentazione dei metodi Open astratta su cosa posso aspettarmi che accada in caso di errore. Se si è verificato un errore durante la connessione, so che non riesco a gestirlo - dovrò lasciarlo scorrere dove il chiamante può mostrare all'utente un'interfaccia utente e lasciarlo riprovare.

Dopo 

public DbConnection GetConnection(String connectionName)
{
   //Get the connection string info from web.config
   ConnectionStringSettings cs= ConfigurationManager.ConnectionStrings[connectionName];

   //documented to return null if it couldn't be found
    if (cs == null)
       throw new ArgumentException("Could not find connection string \""+connectionName+"\"");

   //Get the factory for the given provider (e.g. "System.Data.SqlClient")
   DbProviderFactory factory = DbProviderFactories.GetFactory(cs.ProviderName);

   //Undefined behaviour if GetFactory couldn't find a provider.
   //Defensive test for null factory anyway
   if (factory == null)
      throw new Exception("Could not obtain factory for provider \""+cs.ProviderName+"\"");

   //Have the factory give us the right connection object
   DbConnection conn = factory.CreateConnection();

   //Undefined behaviour if CreateConnection failed
   //Defensive test for null connection anyway
   if (conn == null)
      throw new Exception("Could not obtain connection from factory");

   //Knowing the connection string, open the connection
   conn.ConnectionString = cs.ConnectionString;
   conn.Open()

   return conn;
}

Sommario

Quindi la mia funzione a quattro righe è diventata 12 righe e ho richiesto 5 minuti di ricerca della documentazione. Alla fine ho riscontrato un caso in cui un metodo è autorizzato a restituire null. Ma in pratica tutto ciò che ho fatto è stato convertire un'eccezione di violazione dell'accesso (se provo a chiamare metodi su riferimento null) in InvalidArgumentException .

Prendo anche due possibili casi in cui potrebbero esserci null oggetti di ritorno; ma di nuovo ho scambiato solo un'eccezione con un'altra.

Sul lato positivo, ha riscontrato due problemi e ha spiegato cosa è successo nel messaggio di eccezione, piuttosto che cose brutte che accadono lungo la strada (cioè il dollaro si ferma qui)

Ma ne vale la pena? Questo è eccessivo? Questa programmazione difensiva è andata male?

È stato utile?

Soluzione

Controllare manualmente una configurazione e generare un'eccezione non è meglio che lasciare che il framework lanci l'eccezione se manca la configurazione. Stai solo duplicando i controlli delle condizioni preliminari che avvengono comunque all'interno dei metodi del framework e ti rendono il codice dettagliato senza alcun vantaggio. (In realtà potresti rimuovere informazioni lanciando tutto come classe di eccezione di base. Le eccezioni generate dal framework sono in genere più specifiche.)

Modifica: questa risposta sembra alquanto controversa, quindi un po 'di elaborazione: programmazione difensiva significa " preparare l'inatteso " (o " essere paranoici ") e uno dei modi per farlo è quello di effettuare molti controlli preliminari. In molti casi si tratta di una buona pratica, tuttavia, come per tutte le pratiche, il costo dovrebbe essere valutato rispetto ai benefici.

Ad esempio, non fornisce alcun vantaggio nel lanciare un " Impossibile ottenere la connessione dalla quotazione di fabbrica &; eccezione, poiché non dice nulla sul perché non è stato possibile ottenere il provider - e la riga successiva genererà comunque un'eccezione se il provider è nullo. Pertanto, il costo del controllo dei presupposti (in termini di tempo di sviluppo e complessità del codice) non è giustificato.

D'altra parte, il controllo per verificare che sia presente la configurazione della stringa di connessione potrebbe essere giustificato, poiché l'eccezione può aiutare lo sviluppatore a risolvere il problema. L'eccezione nulla che otterrai nella riga successiva non dice comunque il nome della stringa di connessione mancante, quindi il tuo controllo delle condizioni preliminari fornisce un valore. Se il tuo codice fa parte di un componente, ad esempio, il valore è piuttosto grande, poiché l'utente del componente potrebbe non sapere quali configurazioni richiede il componente.

Una diversa interpretazione della programmazione difensiva è che non dovresti solo rilevare condizioni di errore, ma dovresti anche provare a recuperare da qualsiasi errore o eccezione che può verificarsi. Non credo sia una buona idea in generale.

Fondamentalmente dovresti gestire solo le eccezioni che puoi fare . Le eccezioni da cui non è possibile ripristinare in ogni caso, devono essere passate al gestore di livello superiore. In un'applicazione Web il gestore di livello superiore probabilmente mostra solo una pagina di errore generica. Ma non c'è molto da fare nella maggior parte dei casi, se il database è off-line o manca una configurazione cruciale.

Alcuni casi in cui questo tipo di programmazione difensiva ha senso, è se si accetta l'input dell'utente e tale input può causare errori. Se, ad esempio, l'utente fornisce un URL come input e l'applicazione tenta di recuperare qualcosa da tale URL, è piuttosto importante verificare che l'URL appaia corretto e gestire qualsiasi eccezione che potrebbe derivare dalla richiesta. Ciò consente di fornire un feedback prezioso all'utente.

Altri suggerimenti

Beh, dipende da chi è il tuo pubblico.

Se stai scrivendo un codice di libreria che ti aspetti di essere usato da molte altre persone, che non ti parleranno di come usarlo, non è eccessivo. Apprezzeranno il tuo impegno.

(Detto questo, se lo stai facendo, ti suggerisco di definire eccezioni migliori rispetto a System.Exception, per rendere più facile per le persone che vogliono catturare alcune delle tue eccezioni ma non altre.)

Ma se lo userai da solo (o tu e il tuo amico), ovviamente è eccessivo, e probabilmente ti farà male alla fine rendendo il tuo codice meno leggibile.

Vorrei poter far codificare la mia squadra in questo modo. La maggior parte delle persone non capisce nemmeno il punto della programmazione difensiva. Il meglio che fanno è racchiudere l'intero metodo in un'istruzione try catch e lasciare che tutte le eccezioni vengano gestite dal blocco di eccezioni generico!

Tanto di cappello a te Ian. Posso capire il tuo dilemma. Ho passato lo stesso me stesso. Ma quello che hai fatto probabilmente ha aiutato qualche sviluppatore diverse ore a battere la tastiera.

Ricorda, quando usi un'API framework .net, cosa ti aspetti da essa? Cosa sembra naturale? Fai lo stesso con il tuo codice.

So che ci vuole tempo. Ma poi la qualità ha un costo.

PS: non devi davvero gestire tutti gli errori e generare un'eccezione personalizzata. Ricorda che il tuo metodo verrà utilizzato solo da altri sviluppatori. Dovrebbero essere in grado di capire autonomamente le eccezioni quadro comuni. Quello non vale la pena.

Il tuo " prima di " esempio ha la particolarità di essere chiaro e conciso.

Se qualcosa non va, alla fine verrà generata un'eccezione dal framework. Se non puoi fare nulla per l'eccezione, potresti anche lasciarlo propagare nello stack di chiamate.

Ci sono volte, tuttavia, quando un'eccezione viene lanciata in profondità all'interno del framework che in realtà non fa luce su quale sia il vero problema. Se il tuo problema è che non hai una stringa di connessione valida, ma il framework genera un'eccezione come & Quot; uso non valido di null, & Quot; quindi a volte è meglio cogliere l'eccezione e riproporla con un messaggio più significativo.

Controllo molto gli oggetti null, poiché ho bisogno di un oggetto reale su cui operare, e se l'oggetto è vuoto, l'eccezione generata sarà obliqua, per non dire altro. Ma cerco oggetti null solo se so che è quello che accadrà. Alcune fabbriche di oggetti non restituiscono oggetti null; generano invece un'eccezione e il controllo di null sarà inutile in questi casi.

Non credo che scriverei nessuna di quella logica di controllo con riferimento null - almeno, non nel modo in cui l'hai fatta.

I miei programmi che ottengono le impostazioni di configurazione dal file di configurazione dell'applicazione controllano tutte queste impostazioni all'avvio. Generalmente creo una classe statica per contenere le impostazioni e fare riferimento alle proprietà di quella classe (e non a ConfigurationManager) altrove nell'applicazione. Ci sono due ragioni per questo.

Innanzitutto, se l'applicazione non è configurata correttamente, non funzionerà. Preferirei saperlo al momento in cui il programma legge il file di configurazione piuttosto che in futuro quando provo a creare una connessione al database.

In secondo luogo, il controllo della validità della configurazione non dovrebbe realmente riguardare gli oggetti che si basano sulla configurazione. Non ha senso farti inserire controlli ovunque nel codice se hai già eseguito quei controlli in anticipo. (Ci sono eccezioni a questo, certamente - per esempio, applicazioni di lunga durata in cui è necessario poter cambiare la configurazione mentre il programma è in esecuzione e avere tali cambiamenti riflessi nel comportamento del programma; in programmi come questo è necessario vai su GetFactory ogni volta che hai bisogno di un'impostazione.)

Non farei neanche il controllo del riferimento null sulle chiamate CreateConnection e NullReferenceException. Come scriveresti un caso di prova per esercitare quel codice? Non puoi, perché non sai come rendere nulli questi metodi - non sai nemmeno che è possibile rendere nulli quei metodi. Quindi hai sostituito un problema - il tuo programma può generare un <=> in condizioni che non capisci - con un altro, più significativo: in quelle stesse misteriose condizioni, il tuo programma eseguirà codice che non hai testato.

La mia regola dei pollici è:

  

non intercettare se il messaggio di   l'eccezione generata è rilevante per il   chiamante.

Pertanto, NullReferenceException non ha un messaggio rilevante, vorrei verificare se è null e generare un'eccezione con un messaggio migliore. ConfigurationErrorException è rilevante, quindi non la prendo.

L'unica eccezione è se il " contratto " di GetConnection non recupera la stringa di connessione necessariamente in un file di configurazione.

In questo caso GetConnection DOVREBBE avere un contratto con un'eccezione personalizzata che dice che la connessione non può essere recuperata, quindi puoi racchiudere ConfigurationErrorException nella tua eccezione personalizzata.

L'altra soluzione è specificare che GetConnection non può essere lanciato (ma può restituire null), quindi si aggiunge un " ExceptionHandler " alla tua classe.

Manca la documentazione del metodo. ; -)

Ogni metodo ha alcuni parametri di input e output definiti e un comportamento risultante definito. Nel tuo caso qualcosa del tipo: & Quot; Restituisce una valida connessione aperta in caso di successo, altrimenti restituisce null (o genera una XXXException, come preferisci). Tenendo presente questo comportamento, ora puoi decidere quanto difensivo dovresti programmare.

  • Se il tuo metodo deve esporre informazioni dettagliate sul perché e cosa non ha funzionato, allora fallo come hai fatto tu, controlla e prendi tutto e tutto e restituisci le informazioni appropriate.

  • Ma, se sei solo interessato a un DBConnection aperto o solo null (o qualche eccezione definita dall'utente) in caso di fallimento, allora avvolgi tutto in un tentativo / catch e restituisci null (o qualche eccezione) in caso di errore e l'oggetto altro.

Quindi direi che dipende dal comportamento del metodo e dall'output previsto.

In generale, le eccezioni specifiche del database devono essere rilevate e rilanciate come qualcosa di più generale, come (ipotetico) DataAccessFailure eccezione. Il codice di livello superiore nella maggior parte dei casi non ha bisogno di sapere che stai leggendo i dati dal database.

Un altro motivo per intercettare rapidamente questi errori è che spesso includono alcuni dettagli del database nei loro messaggi, come " Nessuna tabella di questo tipo: ACCOUNTS_BLOCKED " oppure " Chiave utente non valida: 234234 " ;. Se questo si propaga all'utente finale, è male in diversi modi:

  1. confusione.
  2. Potenziale violazione della sicurezza.
  3. Imbarazzante per l'immagine della tua azienda (immagina un cliente che legge un messaggio di errore con una grammatica grezza).

Lo avrei codificato esattamente come il tuo primo tentativo.

Tuttavia, l'utente di quella funzione proteggerebbe l'oggetto connessione con un blocco USING.

Non mi piace davvero tradurre eccezioni come fanno le altre versioni in quanto rende molto difficile scoprire perché si è rotto (Database inattivo? Non si dispone dell'autorizzazione per leggere il file di configurazione, ecc.).

La versione modificata non aggiunge molto valore, a condizione che l'applicazione abbia un AppDomain.UnexpectedException gestore che scarica la catena exception.InnerException e tutte le tracce dello stack in un file di registro di qualche tipo (o anche meglio, acquisisce un minidump ), quindi chiama Environment.FailFast.

Da queste informazioni, sarà ragionevolmente semplice individuare ciò che è andato storto, senza dover complicare il codice del metodo con un ulteriore controllo degli errori.

Nota che è preferibile gestire try/catch (Exception x) e chiamare finally invece di avere un livello superiore main perché con quest'ultimo, la causa originale del problema sarà probabilmente oscurata da ulteriori eccezioni.

Questo perché se si rileva un'eccezione, tutti i blocchi try/catch aperti verranno eseguiti e probabilmente verranno generate più eccezioni, che nasconderanno l'eccezione originale (o peggio, elimineranno i file nel tentativo di ripristinare uno stato, forse i file sbagliati, forse anche i file importanti). Non dovresti mai prendere un'eccezione che indica uno stato di programma non valido che non sai come gestire, anche in un blocco <=> funzione <=> di livello superiore. Gestire <=> e chiamare <=> è un altro (e più desiderabile) bollitore di pesce, perché interrompe l'esecuzione di <=> blocchi e se si sta tentando di interrompere il programma e registrare alcune informazioni utili senza causare ulteriori danni , sicuramente non vuoi eseguire i tuoi <=> blocchi.

Non dimenticare di controllare OutOfMemoryExceptions ... sai, potrebbe accadere.

Le modifiche di Iain mi sembrano sensate.

Se sto usando un sistema e lo uso in modo improprio, voglio la massima informazione sull'uso improprio. Per esempio. se dimentico di inserire alcuni valori in una configurazione prima di chiamare un metodo, desidero un InvalidOperationException con un messaggio che dettaglia il mio errore, non un KeyNotFoundException / NullReferenceException.

È tutto basato sul contesto IMO. Ho visto alcuni messaggi di eccezione abbastanza impenetrabili ai miei tempi, ma altre volte l'eccezione predefinita proveniente dal framework va benissimo.

In generale, penso che sia meglio sbagliare con cautela, specialmente quando stai scrivendo qualcosa che è pesantemente usato da altre persone o in genere nel profondo del grafico della chiamata in cui l'errore è più difficile da diagnosticare.

Cerco sempre di ricordare che come sviluppatore di un pezzo di codice o sistema, sono in una posizione migliore per diagnosticare i guasti rispetto a qualcuno che lo sta semplicemente usando. A volte capita che alcune righe di controllo del codice + un messaggio di eccezione personalizzato possano risparmiare cumulativamente ore di tempo di debug (e anche rendere la tua vita più semplice poiché non ti trascini sulla macchina di qualcun altro per eseguire il debug del loro problema).

Ai miei occhi, il tuo " dopo " il campione non è davvero difensivo. Perché difensivo sarebbe quello di controllare le parti sotto il tuo controllo, che sarebbe l'argomento connectionName (controllare per null o vuoto e lanciare un ArgumentNullException).

Perché non dividere il metodo che hai dopo aver aggiunto tutta la programmazione difensiva? Hai un sacco di blocchi logici distinti che giustificano metodi separati. Perché? Perché allora incapsuli la logica che appartiene insieme e il tuo metodo pubblico risultante collega semplicemente quei blocchi nel modo giusto.

Qualcosa del genere (modificato nell'editor SO, quindi nessun controllo di sintassi / compilatore. Potrebbe non essere compilato ;-)) 

private string GetConnectionString(String connectionName)
{

   //Get the connection string info from web.config
   ConnectionStringSettings cs= ConfigurationManager.ConnectionStrings[connectionName];

   //documented to return null if it couldn't be found
   if (cs == null)
       throw new ArgumentException("Could not find connection string \""+connectionName+"\"");
   return cs;
}

private DbProviderFactory GetFactory(String ProviderName)
{
   //Get the factory for the given provider (e.g. "System.Data.SqlClient")
   DbProviderFactory factory = DbProviderFactories.GetFactory(ProviderName);

   //Undefined behaviour if GetFactory couldn't find a provider.
   //Defensive test for null factory anyway
   if (factory == null)
      throw new Exception("Could not obtain factory for provider \""+ProviderName+"\"");
   return factory;
}

public DbConnection GetConnection(String connectionName)
{
   //Get the connection string info from web.config
   ConnectionStringSettings cs = GetConnectionString(connectionName);

   //Get the factory for the given provider (e.g. "System.Data.SqlClient")
   DbProviderFactory factory = GetFactory(cs.ProviderName);


   //Have the factory give us the right connection object
   DbConnection conn = factory.CreateConnection();

   //Undefined behaviour if CreateConnection failed
   //Defensive test for null connection anyway
   if (conn == null)
      throw new Exception("Could not obtain connection from factory");

   //Knowing the connection string, open the connection
   conn.ConnectionString = cs.ConnectionString;
   conn.Open()

   return conn;
}

PS: questo non è un refactor completo, ha fatto solo i primi due blocchi.

Autorizzato sotto: CC-BY-SA insieme a attribuzione
Non affiliato a StackOverflow
scroll top