Single Responsibility, Api Methods, Error Logging e Helper Classes

Per quanto riguarda l'obiezione dell'architetto, non posso dire di non essere d'accordo, quella classe sta facendo un sacco di cose diverse: logging, gestione delle eccezioni e potenzialmente altri hook pre e post-processing (un nome come MethodSupport , che potrebbe significare praticamente qualsiasi cosa per chiunque diverso da te, incoraggerà gli sviluppatori a mantenere ciò che vogliono lì dentro). Nella sua attuale incarnazione, se voglio registrarmi, devo anche avere una gestione delle eccezioni speciale e viceversa-- Non posso avere l'uno o l'altro. Questo non è coerente con SRP.

Inoltre ... forse devo solo abituarmi ad esso ... ma chiedere a tutti di mantenere la logica principale del servizio in un'espressione lambda sembra piuttosto invasivo. Forse sono solo io.

Questo è uno schizzo che illustra una soluzione simile ma che è meno invasiva e mantiene SRP:

using (new TimedTransaction())
using (new ExceptionScope(this))
{
    //Main logic goes here.  No lambda BTW :)
}

La registrazione dei tempi di inizio e di fine si verifica in TimedTransaction :: ctor e Dispose, rispettivamente. È possibile passare i metadati nel costruttore, ma non sarebbe bello se si potesse annusare la classe e il nome del metodo da StackTrace durante la costruzione, invece? Meno digitazioni per tutti gli altri:)

public TimedTransaction()
{
    var trace = new StackTrace();
    _method = trace.GetFrame(1).GetMethod();
}

public string GetSignature() 
{
    var message = $"{_method.DeclaringType.Name}.{_method.Name}";
    return message;
}

Il costruttore di ExceptionScope sottoscrive l'evento OnException del controller e annulla l'annullamento della sottoscrizione; se è possibile gestire un'eccezione, imposta ExceptionHandled .

Tuttavia, le persone avranno bisogno di ricordarsi di aggiungerle per ogni singolo metodo. Gli attributi del filtro sarebbero migliori. Oppure puoi sovrascrivere ExecuteCore e chiamare ciò di cui hai bisogno prima e dopo l'esecuzione del metodo.

Per quanto posso dire, il tuo collega ti sta semplicemente chiedendo di dividere il comportamento principale della tua classe MethodSupport in modo che non stia facendo la registrazione, tuttavia non vedo alcuna causa per ripetere la registrazione o violare ASCIUTTO.

Ecco un possibile suggerimento per suddividere MethodSupport in due classi diverse:

In primo luogo, considera la creazione di un'interfaccia per il tuo MethodSupport con l'intenzione di iniettare la classe MethodSupport come dipendenza:

public interface IMethodSupport 
{
    string ClassName { get; }
    string MethodName { get; }
    string Dictionary<string, string> Parameters { get; }
    string GetSignature();
    HttpResponseMessage RunLogic(HttpRequestMessage request, Func<HttpResponseMessage> func);
}

Con un'implementazione che non ha alcuna registrazione:

public class MethodSupport : IMethodSupport
{
    public string ClassName { get; private set; }
    public string MethodName { get; private set; }
    public Dictionary<string, string> Parameters { get; private set; }

    public MethodSupport (string className, string methodName, Dictionary<string, string> parameters)
    {
        this.ClassName = className;
        this.MethodName = methodName;
        this.Parameters = parameters;
    }

    public string GetSignature() 
    {
        var parametersCsv = String.Join(", ", this.Parameters);
        var message = $"{this.ClassName}.{this.MethodName}({parametersCsv})";
        return message;
    }

    public HttpResponseMessage RunLogic(HttpRequestMessage request, Func<HttResponseMessage> func)
    {
        HttpResponseMessage response;

        try
        {
            response = func();
        }
        catch (Exception e)
        {
            response = ExceptionHelper.HandleException(request, this, e);
        }

    return response;
    }
}

In secondo luogo, considera una classe che esegue la registrazione e accetta IMethodSupport come dipendenza, quindi può supportare lo stesso comportamento nel mondo esterno senza una conoscenza esplicita dell'implementazione di MethodSupport ; garantire il comportamento di entrambi rimangono indipendenti l'uno dall'altro

public class LoggingMethodSupport
{
    private readonly IMethodSupport _methodSupport;


    public LoggingMethodSupport(IMethodSupport methodSupport)
    {
        _methodSupport = methodSupport;
    }

    public void LogBegin()
    {
        var message = _methodSupport.GetSignature() + " : Begin";
        Logger.Debug(message);
    }

    public void LogEnd()
    {
        var message = _methodSupport.GetSignature() + " : End";
        Logger.Debug(message);
    }

    public HttpResponseMessage RunLogic(HttpRequestMessage request, Func<HttResponseMessage> func)
    {
        this.LogBegin();
        var response = _methodSupport.RunLogic(request, func);
        this.LogEnd();
        return response;
    }    
}

Infine, puoi aggiornare l'utilizzo di questa classe come segue:

public class TestController : ApiController
{
    public HttpResponseMessage Get(int id)
    {
        // TODO - Use some IOC container to do this and decouple from the controller...?
        var methodSupport = new MethodSupport(this.ToString(), nameof(Get), new Dictionary<string, string> { { nameof(id), id.GetType() } });
        var loggingMethodSupport = new LoggingMethodSupport(methodSupport);
        var result = loggingMethodSupport.RunLogic(Request, () => 
        {
            //logic for the api method...
        });
        return result;
    }
}

Questo mi sembrerebbe di aderire più strettamente all'SRP come suggerito dal tuo collega, senza introdurre alcuna ripetizione (nessuna violazione di DRY necessaria).

Questo è eccessivo? Forse. È certamente possibile prendere l'SRP e gli altri principi SOLID 'un passo troppo lontano'; tuttavia, come sviluppatori, spetta a tutti noi utilizzare il nostro giudizio sul luogo in cui tracciare la linea - potrebbero esserci motivi perfettamente validi per farlo, specifici per il tuo progetto e per il team di sviluppo, che potrebbe essere oggetto di dibattito all'interno di la tua squadra. Altri progetti e altri team potrebbero decidere che attenersi rigorosamente a SOLID è meno importante per loro. Non ci sono risposte universalmente corrette.

Sembra che voi stiate spaccando un po 'i capelli. La singola responsabilità è un principio , non un mandato letterale. Eppure, buono per ascoltare il tuo Architetto. Un giorno sarai l'Architetto e vorresti che gli sviluppatori seguissero la tua visione.

Un pensiero è semplicemente rinominare RunLogic () in RunAndLogLogic (). Quindi, almeno il metodo sta facendo esattamente quello che dice.

Per diventare più elegante a riguardo, il metodo potrebbe richiedere un delegato OnStart opzionale (o una sua collezione) e un delegato OnEnd (o una raccolta di questi). Dovresti passare un delegato che registra un messaggio iniziale e un delegato che registra un messaggio finale.

L'idea di Attributo che hai menzionato richiederà ancora del codice per implementare l'Attributo, credo; violare la singola responsabilità. A MENO CHE tu non usi un Aspetto di registrazione (utilizzo PostSharp). Allora sei d'oro.