Comment éviter de violer le SRP dans une classe pour gérer la mise en cache?

12

Remarque: l' exemple de code est écrit en c #, mais cela n'a pas d'importance. J'ai mis c # comme balise parce que je ne peux pas en trouver de plus approprié. Il s'agit de la structure du code.

Je lis Clean Code et j'essaie de devenir un meilleur programmeur.

J'ai souvent du mal à suivre le principe de responsabilité unique (les classes et les fonctions ne devraient faire qu'une seule chose), spécialement dans les fonctions. Peut-être que mon problème est qu'une "chose" n'est pas bien définie, mais quand même ...

Un exemple: j'ai une liste de Fluffies dans une base de données. Peu nous importe ce qu'est un Fluffy. Je veux qu'un cours récupère les peluches. Cependant, les peluches peuvent changer selon une logique. Selon une logique, cette classe retournera les données du cache ou obtiendra la dernière de la base de données. On pourrait dire qu'il gère les peluches, et c'est une chose. Pour faire simple, disons que les données chargées sont bonnes pendant une heure, puis elles doivent être rechargées.

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()me semble bien. L'utilisateur demande des peluches, nous les lui fournissons. Aller les récupérer de la base de données si nécessaire, mais cela pourrait être considéré comme une partie de l'obtention des peluches (bien sûr, c'est quelque peu subjectif).

NeedsReload()semble bien aussi. Vérifie si nous devons recharger les peluches. UpdateNextLoad est très bien. Met à jour l'heure du prochain rechargement. c'est certainement une seule chose.

Cependant, je pense que LoadFluffies()ce qui ne peut pas être décrit comme une seule chose. Il récupère les données de la base de données et planifie le prochain rechargement. Il est difficile de prétendre que le calcul du temps pour le prochain rechargement fait partie de l'obtention des données. Cependant, je ne peux pas trouver une meilleure façon de le faire (renommer la fonction en LoadFluffiesAndScheduleNextLoadpeut être meilleur, mais cela rend le problème plus évident).

Existe-t-il une solution élégante pour vraiment écrire cette classe selon le SRP? Suis-je trop pédant?

Ou peut-être que ma classe ne fait pas vraiment une seule chose?

corbeau
la source
3
Basé sur "écrit en C #, mais cela ne devrait pas avoir d'importance", "Il s'agit de la structure du code", "Un exemple: ... Peu nous importe ce qu'est un Fluffy", "Pour faire simple, disons ...", ce n'est pas une demande de révision de code, mais une question sur un principe de programmation général.
200_success
@ 200_success merci, et désolé, je pensais que ce serait suffisant pour CR
raven
1
Très douces!
Mathieu Guindon
2
À l'avenir, vous feriez mieux de "widget" au lieu de moelleux pour de futures questions similaires, comme un widget est considéré comme un stand non particulier pour des exemples.
whatsisname
1
Je sais que ce n'est qu'un exemple de code, mais utilisez-le DateTime.UtcNowafin d'éviter les changements d'heure d'été, ou même un changement dans le fuseau horaire actuel.
Mark Hurd

Réponses:

23

Si cette classe était vraiment aussi triviale qu'elle semble l'être, alors il n'y aurait pas besoin de s'inquiéter de violer le SRP. Que se passe-t-il si une fonction à 3 lignes a 2 lignes qui font une chose et une autre 1 ligne qui fait autre chose? Oui, cette fonction triviale viole le SRP, et alors quoi? On s'en fout? La violation du SRP commence à devenir un problème lorsque les choses se compliquent.

Votre problème dans ce cas particulier vient très probablement du fait que la classe est plus compliquée que les quelques lignes que vous nous avez montrées.

Plus précisément, le problème réside très probablement dans le fait que cette classe gère non seulement le cache, mais contient également probablement l'implémentation de la GetFluffiesFromDb()méthode. Ainsi, la violation du SRP est dans la classe, pas dans les quelques méthodes triviales qui sont affichées dans le code que vous avez publié.

Donc, voici une suggestion sur la façon de gérer toutes sortes de cas qui entrent dans cette catégorie générale, à l'aide du modèle de décorateur .

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

et il est utilisé comme suit:

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

Notez comment CachingFluffiesProvider.GetFluffies()n'a pas peur de contenir le code qui effectue le temps de vérification et de mise à jour, car c'est une chose triviale. Ce que ce mécanisme fait, c'est d'aborder et de gérer le SRP au niveau de la conception du système, là où cela compte, pas au niveau de minuscules méthodes individuelles, où cela n'a pas d'importance de toute façon.

Mike Nakis
la source
1
+1 pour avoir reconnu que les peluches, la mise en cache et l'accès à la base de données sont en fait trois responsabilités. Vous pourriez même essayer de rendre l'interface FluffiesProvider et les décorateurs génériques (IProvider <Fluffy>, ...) mais cela pourrait être YAGNI.
Roman Reiner
Honnêtement, s'il n'y a qu'un seul type de cache et qu'il tire toujours des objets de la base de données, c'est à mon humble avis surdimensionné (même si la classe "réelle" pourrait être plus complexe comme nous pouvons le voir dans l'exemple). L'abstraction juste pour l'abstraction ne rend pas le code plus propre ou plus maintenable.
Doc Brown
Le problème @DocBrown est le manque de contexte à la question. J'aime cette réponse car elle montre une manière que j'ai utilisée maintes et maintes fois dans des applications plus grandes et parce qu'il est facile d'écrire des tests contre, j'aime aussi ma réponse parce que ce n'est qu'un petit changement et donne quelque chose de clair sans aucune conception excessive, car il se trouve actuellement, sans contexte à peu près toutes les réponses ici sont bonnes:]
stijn
1
FWIW, la classe que j'avais en tête quand j'ai posé la question est plus compliquée que FluffiesManager, mais pas trop. 200 lignes peut-être. Je n'ai pas posé cette question parce que j'ai trouvé un problème avec ma conception (encore?), Juste parce que je n'ai pas pu trouver un moyen de se conformer strictement au SRP, et cela pourrait être un problème dans des cas plus complexes. Donc, le manque de contexte est quelque peu intentionnel. Je pense que cette réponse est excellente.
corbeau
2
@stijn: eh bien, je pense que votre réponse est largement sous-évaluée. Au lieu d'ajouter une abstraction inutile, il vous suffit de couper et de nommer les responsabilités différemment, ce qui devrait toujours être le premier choix avant d'empiler trois couches d'héritage à un problème aussi simple.
Doc Brown
6

Votre classe me semble bien, mais vous avez raison, ce LoadFluffies()n'est pas exactement ce que le nom annonce. Une solution simple serait de changer le nom et de déplacer le rechargement explicite de GetFluffies, dans une fonction avec une description appropriée. Quelque chose comme

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

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

me semble propre (aussi parce que, comme le dit Patrick: il est composé d'autres minuscules fonctions obéissant à SRP), et surtout aussi clair, ce qui est parfois tout aussi important.

stijn
la source
1
J'aime la simplicité de tout ça.
corbeau
6

Je crois que votre classe fait une chose; c'est un cache de données avec un délai d'attente. LoadFluffies semble être une abstraction inutile à moins que vous ne l'appeliez de plusieurs endroits. Je pense qu'il serait préférable de prendre les deux lignes de LoadFluffies et de les mettre dans le conditionnel NeedsReload dans GetFluffies. Cela rendrait l'implémentation de GetFluffies beaucoup plus évidente et est toujours du code propre, car vous composez des sous-programmes à responsabilité unique pour atteindre un seul objectif, une récupération en cache des données de la base de données. Vous trouverez ci-dessous la méthode mise à jour de get fluffies.

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

    return m_Cache;
}
Patrick Goley
la source
Bien que ce soit une assez bonne première réponse, veuillez garder à l'esprit que le code "résultat" est souvent un bon ajout.
Fund Monica's Lawsuit
4

Vos instincts sont corrects. Votre classe, si petite soit-elle, en fait trop. Vous devez séparer la logique de mise en cache d'actualisation temporisée en une classe complètement générique. Ensuite, créez une instance spécifique de cette classe pour gérer Fluffies, quelque chose comme ça (non compilé, le code de travail est laissé comme exercice pour le lecteur):

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 avantage supplémentaire est qu'il est désormais très facile de tester TimedRefreshCache.

Kevin Cline
la source
1
Je suis d'accord que si la logique de rafraîchissement devient plus compliquée que dans l'exemple, ce pourrait être une bonne idée de la refactoriser dans une classe séparée. Mais je ne suis pas d'accord pour dire que la classe dans l'exemple, telle qu'elle est, en fait trop.
Doc Brown
@kevin, je n'ai pas d'expérience en TDD. Pourriez-vous nous expliquer comment testeriez-vous TimedRefreshCache? Je ne le vois pas comme "très facile", mais cela pourrait être mon manque d'expertise.
corbeau
1
Personnellement, je n'aime pas votre réponse à cause de sa complexité. Il est très générique et très abstrait et peut être préférable dans des situations plus compliquées. Mais dans ce cas simple, c'est «tout simplement trop». Veuillez consulter la réponse de stijn. Quelle réponse agréable, courte et lisible. Tout le monde le comprendra immédiatement. Qu'est-ce que tu penses?
Dieter Meemken
1
@raven Vous pouvez tester TimedRefreshCache en utilisant un intervalle court (comme 100 ms) et un producteur très simple (comme DateTime.Now). Toutes les 100 ms, le cache produira une nouvelle valeur, entre les deux, il renverra la valeur précédente.
Kevin Cline du
1
@DocBrown: Le problème est que tel qu'il est écrit, il n'est pas testable. La logique de synchronisation (testable) est couplée à la logique de la base de données, qui peut alors être largement moquée. Une fois une couture créée pour simuler l'appel de la base de données, vous êtes à 95% du chemin vers la solution générique. J'ai trouvé que la construction de ces petites classes est généralement payante car elles finissent par être réutilisées plus que prévu.
Kevin Cline du
1

Votre classe va bien, SRP concerne une classe et non une fonction, toute la classe est responsable de fournir les "Fluffies" de la "Data Source" afin que vous soyez libre dans l'implémentation interne.

Si vous souhaitez étendre le mécanisme de cahing, vous pouvez créer une classe respnsible pour regarder la source de données

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
la source
Selon l'oncle Bob: LES FONCTIONS DEVRAIENT FAIRE UNE CHOSE. ILS DEVRAIENT LE FAIRE BIEN. ILS DOIVENT LE FAIRE SEULEMENT. Code propre p.35.
corbeau