Come evitare di violare l'SRP in una classe per gestire la memorizzazione nella cache?

12

Nota: l'esempio del codice è scritto in c #, ma non dovrebbe essere importante. Ho inserito C # come tag perché non riesco a trovarne uno più appropriato. Riguarda la struttura del codice.

Sto leggendo Clean Code e sto cercando di diventare un programmatore migliore.

Spesso mi trovo a dover lottare per seguire il Principio di Responsabilità Unica (le classi e le funzioni dovrebbero fare solo una cosa), specialmente nelle funzioni. Forse il mio problema è che "una cosa" non è ben definita, ma comunque ...

Un esempio: ho un elenco di Fluffies in un database. Non ci importa cosa sia un Fluffy. Voglio una classe per recuperare i fluff. Tuttavia, i fluffies possono cambiare secondo una logica. A seconda della logica, questa classe restituirà i dati dalla cache o recupererà l'ultima dal database. Potremmo dire che gestisce i fluff, e questa è una cosa. Per semplificare, supponiamo che i dati caricati siano validi per un'ora, quindi devono essere ricaricati.

class FluffiesManager
{
    private Fluffies m_Cache;
    private DateTime m_NextReload = DateTime.MinValue;
    // ...
    public Fluffies GetFluffies()
    {
        if (NeedsReload())
            LoadFluffies();

        return m_Cache;
    }

    private NeedsReload()
    {
        return (m_NextReload < DateTime.Now);
    }

    private void LoadFluffies()
    {
        GetFluffiesFromDb();
        UpdateNextLoad();
    }

    private void UpdateNextLoad()
    {
        m_NextReload = DatTime.Now + TimeSpan.FromHours(1);
    }
    // ...
}

GetFluffies() sembra ok per me. L'utente chiede alcuni fluff, li forniamo. Andando a recuperarli dal DB, se necessario, ma ciò potrebbe essere considerato come una parte dell'ottenere i fluff (ovviamente, è un po 'soggettivo).

Anche

NeedsReload() sembra giusto. Controlla se abbiamo bisogno di ricaricare i fluff. UpdateNextLoad va bene. Aggiorna il tempo per la prossima ricarica. è sicuramente una cosa sola.

Tuttavia, ritengo che ciò che LoadFluffies() do non può essere descritto come una singola cosa. Sta ottenendo i dati dal database e sta pianificando il prossimo ricaricamento. È difficile sostenere che il calcolo del tempo per il prossimo ricaricamento è parte dell'ottenere i dati. Tuttavia, non riesco a trovare un modo migliore per farlo (rinominare la funzione in LoadFluffiesAndScheduleNextLoad potrebbe essere migliore, ma rende solo il problema più ovvio).

Esiste una soluzione elegante per scrivere davvero questa classe in base all'SRP? Sono troppo pedante?

O forse la mia classe non sta facendo proprio una cosa?

    
posta raven 21.11.2015 - 15:39
fonte

5 risposte

23

Se questa classe fosse davvero banale come sembra, allora non ci sarebbe bisogno di preoccuparsi di violare l'SRP. Quindi cosa succede se una funzione a 3 linee ha 2 linee che fanno una cosa, e un'altra 1 linea che fa un'altra cosa? Sì, questa funzione banale viola l'SRP, e allora? Che importa? La violazione dell'SRP inizia a diventare un problema quando le cose si complicano.

Il tuo problema in questo caso particolare deriva probabilmente dal fatto che la classe è più complicata delle poche righe che ci hai mostrato.

In particolare, il problema sta probabilmente nel fatto che questa classe non solo gestisce la cache, ma probabilmente contiene anche l'implementazione del metodo GetFluffiesFromDb() . Quindi, la violazione dell'SRP è nella classe, non nei pochi metodi banali mostrati nel codice che hai postato.

Quindi, ecco un suggerimento su come gestire tutti i tipi di casi che rientrano in questa categoria generale, con l'aiuto del Decorator modello .

/// Provides Fluffies.
interface FluffiesProvider
{
    Fluffies GetFluffies();
}

/// Implements FluffiesProvider using a database.
class DatabaseFluffiesProvider : FluffiesProvider
{
    public override Fluffies GetFluffies()
    {
        ... load fluffies from DB ...
        (the entire implementation of "GetFluffiesFromDb()" goes here.)
    }
}

/// Decorates FluffiesProvider to add caching.
class CachingFluffiesProvider : FluffiesProvider
{
    private FluffiesProvider decoree;
    private DateTime m_NextReload = DateTime.MinValue;
    private Fluffies m_Cache;

    public CachingFluffiesProvider( FluffiesProvider decoree )
    {
        Assert( decoree != null );
        this.decoree = decoree;
    }

    public override Fluffies GetFluffies()
    {
        if( DateTime.Now >= m_NextReload ) 
        {
             m_Cache = decoree.GetFluffies();
             m_NextReload = DatTime.Now + TimeSpan.FromHours(1);
        }
        return m_Cache;
    }
}

e viene utilizzato come segue:

FluffiesProvider provider = new DatabaseFluffiesProvider();
provider = new CachingFluffiesProvider( provider );
...go ahead and use provider...

Nota come CachingFluffiesProvider.GetFluffies() non ha paura di contenere il codice che controlla e aggiorna il tempo, perché è roba banale. Ciò che questo meccanismo fa è di indirizzare e gestire l'SRP a livello di progettazione del sistema, dove è importante, non a livello di piccoli metodi individuali, dove non importa comunque.

    
risposta data 21.11.2015 - 20:23
fonte
6

La tua stessa classe mi sembra soddisfacente, ma hai ragione in quanto LoadFluffies() non è esattamente ciò che il nome pubblicizza. Una soluzione semplice sarebbe quella di cambiare il nome e spostare il ricarico esplicito da GetFluffies, in una funzione con una descrizione appropriata. Qualcosa come

public Fluffies GetFluffies()
{
  MakeSureTheFluffyCacheIsUpToDate();
  return m_Cache;
}

private void MakeSureTheFluffyCacheIsUpToDate()
{
  if( !NeedsReload )
    return;
  GetFluffiesFromDb();
  SetNextReloadTime();
}

mi sembra pulito (anche perché, come dice Patrick, è composto da altre piccole funzioni obbedienti all'SRP), e soprattutto è chiaro che a volte è altrettanto importante.

    
risposta data 21.11.2015 - 18:39
fonte
6

Credo che la tua classe stia facendo una cosa; è una cache di dati con un timeout. LoadFluffies sembra un'astrazione inutile a meno che non lo chiami da più punti. Penso che sarebbe meglio prendere le due linee da LoadFluffies e inserirle in NeedsReload condizionale in GetFluffies. Ciò renderebbe l'implementazione di GetFluffies molto più ovvia ed è ancora un codice pulito, poiché si stanno componendo sottoprogrammi di singola responsabilità per raggiungere un singolo obiettivo, un recupero nella cache dei dati dal db. Di seguito è riportato il metodo aggiornato get fluffies.

public Fluffies GetFluffies()
{
    if (NeedsReload()) {
        GetFluffiesFromDb();
        UpdateNextLoad();
    }

    return m_Cache;
}
    
risposta data 21.11.2015 - 15:51
fonte
4

I tuoi istinti sono corretti. La tua classe, per quanto piccola, sta facendo troppo. È necessario separare la logica di caching di aggiornamento temporizzato in una classe completamente generica. Quindi crea un'istanza specifica di quella classe per la gestione di Fluffies, qualcosa di simile a questo (non compilato, il codice di lavoro è lasciato come esercizio per il lettore):

public class TimedRefreshCache<T> {
    T m_Value;
    DateTime m_NextLoadTime;
    Func<T> m_producer();
    public CacheManager(Func<T> T producer, Interval timeBetweenLoads) {
          m_nextLoadTime = INFINITE_PAST;
          m_producer = producer;
    }
    public T Value {
        get {
            if (m_NextLoadTime < DateTime.Now) {
                m_Value = m_Producer();
                m_NextLoadTime = ...;
            }
            return m_Value;
        }
    }
}

public class FluffyCache {
    private TimedRefreshCache m_Cache 
        = new TimedRefreshCache<Fluffy>(GetFluffiesFromDb, interval);
    private Fluffy GetFluffiesFromDb() { ... }
    public Fluffy Value { get { return m_Cache.Value; } }
}

Un ulteriore vantaggio è che ora è molto semplice testare TimedRefreshCache.

    
risposta data 21.11.2015 - 23:02
fonte
1

La tua classe va bene, SRP parla di una classe non di una funzione, l'intera classe è responsabile della fornitura dei "Fluffies" dalla "Fonte dei dati", quindi sei libero nell'implementazione interna.

Se desideri espandere il meccanismo di cahing, puoi creare classi rispettabili per la visualizzazione dell'origine dati

public class ModelWatcher
{

    private static Dictionary<Type, DateTime> LastUpdate;

    public static bool IsUpToDate(Type entityType, DateTime lastRead) {
        if (LastUpdate.ContainsKey(entityType)) {
            return lastRead >= LastUpdate[entityType];
        }
        return true;
    }

    //call this method whenever insert/update changed to any entity
    private void OnDataSourceChanged(Type changedEntityType) {
        //update Date & Time
        LastUpdate[changedEntityType] = DateTime.Now;
    }
}
public class FluffyManager
{
    private DateTime LastRead = DateTime.MinValue;

    private List<Fluffy> list;



    public List<Fluffy> GetFluffies() {

        //if first read or not uptodated
        if (list==null || !ModelWatcher.IsUpToDate(typeof(Fluffy),LastRead)) {
            list = ReadFluffies();
        }
        return list;
    }
    private List<Fluffy> ReadFluffies() { 
    //read code
    }
}
    
risposta data 22.11.2015 - 12:31
fonte