Utilizzo del parametro opzionale da eseguire nel blocco try?

L'aspetto di ciò che direi è il pattern da evitare, è l'uso di un argomento flag .

Il bool cambia completamente il metodo, quindi rendi due metodi:

public void DoSomethingSafely() 
{
    try
    {
        DoSomethingDangerous();
    }
    catch
    {
        // Log error & carry on
    }
}

public void DoSomethingDangerous()
{
    // whatever it does
}

Per prima cosa, c'è il rischio di

Masking errors without taking meaningful action ?

Probabilmente no, dal momento che l'eccezione viene passata al chiamante (quindi il chiamante è responsabile di un'azione significativa), oppure viene registrata l'eccezione, che è probabilmente l'azione desiderata qui.

C'è un segno di

use of error handling for control flow ?

La risposta è IMHO anche no, questo sarebbe solo il caso se DoSomethingDangerous lancia un'eccezione per una situazione non eccezionale, ma non c'è alcuna indicazione per questo nel codice presentato nella domanda.

Quindi, se questi due punti non rappresentano il problema, c'è un altro problema?

Suppongo che l'intenzione dell'autore fosse di rendere il codice riutilizzabile in due contesti diversi:

• in cui le eccezioni sono autorizzate a comparire nel chiamante, che ad esempio potrebbe comportare la visualizzazione di una finestra di messaggio nell'interfaccia utente, che interrompe qualsiasi elaborazione esterna

• quello in cui le eccezioni non devono interferire con il controllo esterno del flusso (ad esempio, in un'elaborazione batch asincrona)

Finora questo potrebbe essere un requisito ragionevole. Ma è possibile migliorare qualcosa? Ovviamente, la cosa ovvia qui è evitare la duplicazione del codice (già suggerita da @ErikLippert in un commento). Se hai bisogno di mantenere la semantica, dovrebbe apparire così:

public void DoSomething(bool safe=true) 
{
   try
   {
       DoSomethingDangerous();
   }
   catch(Exception ex)
   {
       if(!safe)
          throw ex;

       // Log error & carry on
   }
}

L'altro problema di manutenibilità che vedo qui è che la gestione delle eccezioni ora avviene a due diversi livelli di astrazione, cosa che eviterei ogni volta che sia possibile, poiché può facilmente diventare soggetta a errori. Risolvere ciò, tuttavia, richiede di rifattorizzare il codice prima come suggerito da @DavidArno, quindi di rifattorizzare il codice esterno per chiamare DoSomethingSafely solo nel contesto di elaborazione batch e DoSomethingDangerous solo in "modalità UI".

Ciò tuttavia può comportare un sacco di refactoring. Se ci sono diversi livelli intermedi tra il "controllore batch" o "controller UI" esterno e il metodo mostrato, può diventare davvero difficile evitare la duplicazione del codice aggiuntiva all'interno del livello chiamante. Il motivo è che probabilmente sarai costretto ad implementare due diverse versioni del codice chiamante, una per ogni contesto. Una di queste versioni utilizzerà DoSomethingSafely in un modo specifico e una che utilizza DoSomethingDangerous in modo simile, ma con alcune differenze in termini di gestione degli errori e forse di altro contesto. Quindi, per prendere una decisione se questo è l'approccio giusto, è necessario prestare attenzione al codice chiamante.

Infine, ecco un altro approccio che utilizza l'iniezione delle dipendenze. Lo stesso "Logger" potrebbe essere un oggetto iniettato (forse non dovrebbe chiamarsi "Logger", ma qualcosa come "ExceptionProcessor"):

class MyClass
{
    IExceptionProcesser exProc;  // gets injected through the constructor

    public void DoSomething() 
    {
      try
      {
         DoSomethingDangerous();
      }
      catch(Exception ex)
      {
         // Log error, or rethrow
         exProc.Process(ex);   
      }
   }
}

In questo modo, la parametrizzazione del comportamento dell'eccezione non viene più eseguita da un parametro booleano alla funzione, ma passando un altro programma di registrazione nella classe. Ciò si adatta in genere meglio alla necessità di far sì che la funzione si comporti diversamente in contesti diversi e evita anche l'uso di un argomento flag come parametro di funzione.

Ho usato questo genere di cose in alcuni punti. Di solito lo inquadro in modo opposto (qualcosa è permesso fallire, il default è lanciare); in particolare, quando esegue un'app, esegue alcune operazioni di risoluzione e non è chiaro se il target sia pronto o meno. Una volta che tutto è configurato, diventa un errore difficile e veloce se la dipendenza non è disponibile.

L'altra cosa da considerare con questo modello è il refactoring. Se vedessi questo codice, presumo che fosse originariamente

try { DoSomethingDangerous(); } catch { // log and continue }

e ciò stava causando problemi (o era altrimenti indesiderabile). Ma cambiare quel comportamento sarebbe un cambiamento semantico e dirompente per i consumatori. Avendo il flag safe di default consente di testare il cambiamento con i consumatori per assicurarsi che non sarà molto fastidioso cambiarlo completamente. Ho visto questo genere di cose anche con flag di funzionalità o un valore di configurazione simile piuttosto che con il parametro di funzione.

Tuttavia, considererei questa cosa un po 'puzzolente e di solito evitata.

La mia opzione è vicina a quella di David Arno, ma prenderei in considerazione l'uso dei decoratori. Quindi potrebbe andare come

    interface A
    {
        void doSomething();
    }

    class UnsafeA : A
    {
        public void doSomething()
        {
            throw new Exception();
        }
    }

    class SafeA : A
    {
        private A origin;

        public SafeA(A a)
        {
            origin = a;
        }

        public void doSomething()
        {
            try {
                origin.doSomething();
            } catch (Exception e) {
                // handle
            }
        }
    }

Quindi potresti spostare la logica if più vicina al punto di ingresso del programma (che è sempre una buona cosa, imho) e creare un'istanza di una classe basata sul valore safe . Oltre a ciò, penso che questo approccio sia più SRP-ish.

Miglioramento dell'approccio OP

La mia lamentela principale è l'uso della parola "sicuro" (la chiamata può ancora fallire, semplicemente non lo saprai) e che è troppo facile per il chiamante commettere un errore, perché il default (sicuro = true) è fare qualcosa che è non sono consigliati .

Il modo in cui l'avrei scritto sarebbe stato:

public bool DoSomething(bool swallowExceptions = false) 
{
   try
   {
       DoSomethingDangerous();
       return true;
   }
   catch(System.Exception exception)
   {
       LogTheError(exception);
       if (!swallowExceptions) throw;
       return false;
   }
}

.. evitando così il codice duplicato e assicurando che tutte le eccezioni ingerite siano almeno registrate. Anche restituire un valore booleano per indicare il successo è utile e mantenere lo schema dimostrato da metodi come TryParse . Il chiamante non deve usarlo, ovviamente.

Inoltre, soprattutto, il chiamante è obbligato a impostare swallowExceptions su true se lo desidera. In questo modo possiamo essere ragionevolmente sicuri che sia apposta.

Inoltre, come ho detto, la chiamata non è "sicura" e quel nome non dice molto al chiamante, ma chiamarlo "swallowExceptions" è abbastanza chiaro, IMO.

Un altro modo per farlo è dividerlo in due metodi, usando la convenzione Try per rendere chiaro al chiamante cosa sta succedendo:

public void DoSomething()
{
    //Do something
}

public bool TryDoSomething() 
{
   try
   {
       DoSomething();  //Calls the other function
       return true;
   }
   catch(System.Exception exception)
   {
       LogTheError(exception);
       return false;
   }
}

In questo modo non hai bisogno di un metodo separato, privato DoSomethingDangerous . Hai appena DoSomething e una chiamata che lo avvolge.

Un approccio generalizzabile

D'altra parte, sembra inutile scrivere tutte queste cose per ogni metodo che vuoi rendere "sicuro". Potrebbe essere fatto in modo più generico in questo modo:

class Try
{
    static public bool Run(Action action)
    {
        try
        {
            action();
            return true;
        }
        catch
        {
            return false;
        }
    }
}

Quindi chiameresti DoSomethingDangerous in uno dei seguenti modi:

Normale:

DoSomethingDangerous();

"Safe":

bool ok = Try.Run(DoSomethingDangerous);

o (se è necessario passare argomenti)

bool ok = Try.Run(() => DoSomethingDangerous("Hello"));

Se usi questo metodo, non dovrai scimmiottare con i prototipi per i metodi che vuoi essere "sicuro". Si termina la chiamata in Try.Run() . Ed è improbabile che un programmatore lo faccia per caso.

Un motivo per cui eviterei questo è perché espone i dettagli del metodo interno attraverso l'interfaccia. Il chiamante deve sapere che cosa fa il metodo, non se sta andando a mangiare eccezioni. Se il chiamante desidera gestire le eccezioni o ignorarle, può determinarlo o lasciare che l'eccezione diventi un punto in cui viene effettuata tale determinazione.

L'uso di una tale bandiera viola l'SRP in un certo senso. La classe dovrebbe essere responsabile solo per ciò che fa, non per sapere cosa vuole fare il chiamante con le sue eccezioni. Questo è particolarmente vero dal momento che è banalmente facile per il chiamante gestire cosa - se non altro - con un'eccezione sollevata dal metodo.

Ed è solo una questione di tempo prima che qualcuno capisca che il metodo contiene più operazioni che potrebbero generare eccezioni e portarlo al livello successivo:

public void DoSomethingDangerous(
    bool safe = true, 
    bool throwNullReference = false, 
    bool handeIoException = true)