Wie vermeide ich, dass die SRP in einer Klasse verletzt wird, um das Caching zu verwalten?

12

Hinweis: Das Codebeispiel ist in c # geschrieben, aber das sollte keine Rolle spielen. Ich habe c # als Tag angegeben, weil ich kein passenderes finde. Hier geht es um die Codestruktur.

Ich lese Clean Code und versuche, ein besserer Programmierer zu werden.

Ich habe oft Mühe, das Prinzip der Einzelverantwortung zu befolgen (Klassen und Funktionen sollten nur eines tun), insbesondere in Funktionen. Vielleicht ist mein Problem, dass "eine Sache" nicht genau definiert ist, aber immer noch ...

Ein Beispiel: Ich habe eine Liste von Fluffies in einer Datenbank. Es ist uns egal, was ein Fluffy ist. Ich möchte, dass eine Klasse Flaumiges wiederherstellt. Fluffies können sich jedoch nach einer bestimmten Logik ändern. Je nach Logik gibt diese Klasse die Daten aus dem Cache zurück oder ruft die neuesten Daten aus der Datenbank ab. Wir könnten sagen, dass es mit Flusen umgeht, und das ist eine Sache. Nehmen wir zum Vereinfachen an, dass geladene Daten eine Stunde lang gültig sind und dann neu geladen werden müssen.

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()scheint mir in Ordnung zu sein. Der Benutzer bittet um einige Flaumigkeiten, wir stellen sie zur Verfügung. Ich werde sie bei Bedarf aus der Datenbank wiederherstellen, aber das kann als Teil des Erhalts der Fluffies angesehen werden (natürlich ist das etwas subjektiv).

NeedsReload()scheint auch richtig. Überprüft, ob die Fluffies neu geladen werden müssen. UpdateNextLoad ist in Ordnung. Aktualisiert die Zeit für das nächste Neuladen. Das ist definitiv eine einzige Sache.

Ich fühle jedoch, was LoadFluffies()nicht als eine einzige Sache beschrieben werden kann. Es ruft die Daten aus der Datenbank ab und plant das nächste Neuladen. Es ist schwer zu argumentieren, dass das Berechnen der Zeit für das nächste Neuladen Teil des Erhalts der Daten ist. Ich kann jedoch keinen besseren Weg finden, dies zu tun (das Umbenennen der Funktion in ist LoadFluffiesAndScheduleNextLoadmöglicherweise besser, aber es macht das Problem nur offensichtlicher).

Gibt es eine elegante Lösung, um diese Klasse wirklich nach dem SRP zu schreiben? Bin ich zu pedantisch?

Oder macht meine Klasse nicht wirklich nur eine Sache?

Rabe
quelle
3
Basierend auf "geschrieben in C #, aber das sollte keine Rolle spielen", "Hier geht es um die Codestruktur", "Ein Beispiel: ... Es ist uns egal, was ein Fluffy ist", "Um es einfach zu machen, sagen wir ...", Dies ist keine Aufforderung zur Codeüberprüfung, sondern eine Frage zu einem allgemeinen Programmierprinzip.
200_success
@ 200_success danke, und sorry, ich dachte , das für CR angemessen wäre
raven
1
So flauschig!
Mathieu Guindon
2
Zukünftig sind Sie mit "Widget" besser dran als mit "Flauschig" für zukünftige ähnliche Fragen, da ein Widget als nicht spezifisches Beispiel verstanden wird.
Whatsisname
1
Ich weiß, es ist nur ein Beispielcode, aber verwenden DateTime.UtcNowSie ihn, um Umstellungen auf Sommerzeit oder sogar eine Änderung der aktuellen Zeitzone zu vermeiden.
Mark Hurd

Antworten:

23

Wenn diese Klasse wirklich so trivial wäre, wie es scheint, dann müsste man sich keine Sorgen über eine Verletzung der SRP machen. Was ist, wenn bei einer 3-Zeilen-Funktion zwei Zeilen eine und eine weitere Zeile eine andere Aufgabe haben? Ja, diese triviale Funktion verstößt gegen das SRP, und was nun? Wen interessiert das? Die Verletzung der SRP wird zu einem Problem, wenn die Dinge komplizierter werden.

Ihr Problem in diesem speziellen Fall ergibt sich höchstwahrscheinlich aus der Tatsache, dass die Klasse komplizierter ist als die wenigen Zeilen, die Sie uns gezeigt haben.

Insbesondere liegt das Problem höchstwahrscheinlich in der Tatsache, dass diese Klasse nicht nur den Cache verwaltet, sondern wahrscheinlich auch die Implementierung der GetFluffiesFromDb()Methode enthält. Die Verletzung der SRP liegt also in der Klasse, nicht in den wenigen trivialen Methoden, die in dem von Ihnen veröffentlichten Code angezeigt werden.

Hier ist ein Vorschlag, wie alle Arten von Fällen, die in diese allgemeine Kategorie fallen, mithilfe des Decorator-Musters behandelt werden können .

/// 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;
    }
}

und es wird wie folgt verwendet:

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

Beachten Sie, dass Sie CachingFluffiesProvider.GetFluffies()keine Angst haben, den Code zu enthalten, mit dem die Zeit überprüft und aktualisiert wird, denn das ist trivial. Mit diesem Mechanismus wird die SRP auf der Ebene des Systemdesigns angesprochen und gehandhabt, auf der es darauf ankommt, nicht auf der Ebene winziger Einzelmethoden, auf der es sowieso nicht darauf ankommt.

Mike Nakis
quelle
1
+1 für das Erkennen, dass Fluffies, Caching und Datenbankzugriff tatsächlich drei Verantwortlichkeiten sind. Sie könnten sogar versuchen, die FluffiesProvider-Schnittstelle und die Dekoratoren generisch zu machen (IProvider <Fluffy>, ...), aber dies könnte YAGNI sein.
Roman Reiner
Ganz ehrlich, wenn es nur einen Cache-Typ gibt, der immer Objekte aus der Datenbank abruft, ist dies IMHO stark überarbeitet (auch wenn die "echte" Klasse möglicherweise komplexer ist, wie wir im Beispiel sehen können). Abstraktion nur der Abstraktion zuliebe macht Code nicht sauberer oder wartbarer.
Doc Brown
@ DocBrown Problem ist der Mangel an Kontext zu der Frage. Diese Antwort gefällt mir, da sie eine Art und Weise zeigt, die ich in größeren Anwendungen immer wieder verwendet habe, und weil es einfach ist, Tests dagegen zu schreiben, gefällt mir auch meine Antwort, weil es sich nur um eine kleine Änderung handelt und etwas Klares ergibt, ohne es zu überarbeiten Derzeit steht, ohne Kontext so ziemlich alle Antworten hier sind gut:]
Stijn
1
FWIW, die Klasse, an die ich gedacht habe, als ich die Frage gestellt habe, ist komplizierter als FluffiesManager, aber nicht übertrieben. Etwa 200 Zeilen vielleicht. Ich habe diese Frage nicht gestellt, weil ich (noch?) Ein Problem mit meinem Design gefunden habe, nur weil ich keine Möglichkeit gefunden habe, die SRP strikt einzuhalten, und das könnte in komplexeren Fällen ein Problem sein. Der Mangel an Kontext ist also etwas beabsichtigt. Ich finde diese Antwort großartig.
Rabe
2
@stijn: naja, ich denke deine antwort ist stark unterbewertet. Anstatt unnötige Abstraktionen hinzuzufügen, schneiden Sie die Verantwortlichkeiten einfach ab und benennen sie anders. Dies sollte immer die erste Wahl sein, bevor Sie drei Vererbungsebenen auf ein so einfaches Problem legen.
Doc Brown
6

Ihre Klasse selbst scheint mir in Ordnung zu sein, aber Sie haben Recht damit, dass LoadFluffies()nicht genau das stimmt, was der Name verspricht. Eine einfache Lösung wäre, den Namen zu ändern und das explizite Neuladen von GetFluffies in eine Funktion mit einer entsprechenden Beschreibung zu verschieben. Etwas wie

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

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

sieht für mich sauber aus (auch weil, wie Patrick sagt: es besteht aus anderen winzigen SRP-gehorsamen Funktionen) und vor allem auch klar, was manchmal genauso wichtig ist.

stijn
quelle
1
Mir gefällt die Einfachheit darin.
Rabe
6

Ich glaube, deine Klasse tut eine Sache; Es ist ein Datencache mit einer Zeitüberschreitung. LoadFluffies scheint eine nutzlose Abstraktion zu sein, es sei denn, Sie rufen sie von mehreren Stellen aus auf. Ich denke, es wäre besser, die beiden Zeilen aus LoadFluffies zu nehmen und sie in die NeedsReload-Bedingung in GetFluffies zu setzen. Dies würde die Implementierung von GetFluffies viel offensichtlicher machen und ist immer noch sauberer Code, da Sie Subroutinen mit einer einzigen Verantwortung erstellen, um ein einziges Ziel zu erreichen, nämlich einen zwischengespeicherten Abruf von Daten aus der Datenbank. Nachfolgend finden Sie die aktualisierte Get Fluffies-Methode.

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

    return m_Cache;
}
Patrick Goley
quelle
Dies ist zwar eine ziemlich gute erste Antwort, bedenken Sie jedoch, dass der Code "result" häufig eine gute Ergänzung darstellt.
Fund Monica's Lawsuit
4

Ihre Instinkte sind richtig. Ihre Klasse, so klein sie auch sein mag, tut zu viel. Sie sollten die Cache-Logik für die zeitgesteuerte Aktualisierung in eine vollständig generische Klasse unterteilen. Erstellen Sie dann eine bestimmte Instanz dieser Klasse zum Verwalten von Fluffies (nicht kompiliert, Arbeitscode bleibt als Übung für den Leser übrig):

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; } }
}

Ein zusätzlicher Vorteil ist, dass es jetzt sehr einfach ist, TimedRefreshCache zu testen.

Kevin Cline
quelle
1
Ich stimme zu, dass es eine gute Idee sein könnte, die Aktualisierungslogik in eine separate Klasse umzugliedern, wenn sie komplizierter wird als im Beispiel. Ich bin jedoch anderer Meinung, dass die Klasse in dem Beispiel so wie sie ist zu viel tut.
Doc Brown
@ Kevin, ich bin nicht in TDD erfahren. Könnten Sie näher erläutern, wie Sie TimedRefreshCache testen würden? Ich sehe es nicht als "sehr einfach" an, aber es könnte mein Mangel an Fachwissen sein.
Rabe
1
Ich persönlich mag Ihre Antwort nicht, weil sie komplex ist. Es ist sehr allgemein und sehr abstrakt und kann in komplizierteren Situationen am besten sein. Aber in diesem einfachen Fall ist es einfach zu viel. Bitte werfen Sie einen Blick auf die Antwort von stijn. Was für eine schöne, kurze und lesbare Antwort. Jeder wird es sofort verstehen. Was denkst du?
Dieter Meemken
1
@raven Sie können TimedRefreshCache mit einem kurzen Intervall (wie 100 ms) und einem sehr einfachen Produzenten (wie DateTime.Now) testen. Alle 100 ms erzeugt der Cache einen neuen Wert, dazwischen wird der vorherige Wert zurückgegeben.
Kevin Cline
1
@DocBrown: Das Problem ist, dass es wie geschrieben nicht testbar ist. Die Zeitsteuerungslogik (testbar) ist mit der Datenbanklogik gekoppelt, die dann viel verspottet wird. Sobald eine Naht erstellt wurde, die den Datenbankaufruf nachahmt, sind Sie zu 95% auf dem Weg zur generischen Lösung. Ich habe festgestellt, dass sich das Bauen dieser kleinen Klassen in der Regel auszahlt, weil sie mehr als erwartet wiederverwendet werden.
Kevin Cline
1

Ihre Klasse ist in Ordnung, SRP ist eine Klasse, keine Funktion. Die gesamte Klasse ist für die Bereitstellung der "Fluffies" aus der "Datenquelle" verantwortlich, sodass Sie bei der internen Implementierung frei sind.

Wenn Sie den Cahing-Mechanismus erweitern möchten, können Sie eine Klasse erstellen, die für das Überwachen der Datenquelle zuständig ist

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
    }
}
yousif.aljamri
quelle
Laut Onkel Bob: FUNKTIONEN SOLLTEN EINE SACHE TUN. SIE SOLLTEN ES GUT MACHEN. SIE SOLLTEN ES NUR TUN. Clean Code S.35.
Rabe