¿Cómo evitar violar el SRP en una clase para administrar el almacenamiento en caché?

12

Nota: El código de muestra está escrito en C #, pero eso no debería importar. Puse c # como etiqueta porque no puedo encontrar una más adecuada. Esto se trata de la estructura del código.

Estoy leyendo Clean Code e intento convertirme en un mejor programador.

A menudo me encuentro luchando por seguir el Principio de Responsabilidad Única (las clases y las funciones deberían hacer solo una cosa), especialmente en las funciones. Quizás mi problema es que "una cosa" no está bien definida, pero aún así ...

Un ejemplo: tengo una lista de Fluffies en una base de datos. No nos importa lo que sea un Fluffy. Quiero una clase para recuperar pelusas. Sin embargo, las pelusas pueden cambiar según alguna lógica. Dependiendo de alguna lógica, esta clase devolverá los datos del caché u obtendrá lo último de la base de datos. Podríamos decir que maneja pelusas, y eso es una cosa. Para simplificarlo, supongamos que los datos cargados son válidos durante una hora y luego deben volver a cargarse.

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 parece bien El usuario solicita algunas pelusas, nosotros se las proporcionamos. Ir a recuperarlos de la base de datos si es necesario, pero eso podría considerarse parte de obtener las esponjosas (por supuesto, eso es algo subjetivo).

NeedsReload()Parece correcto también. Comprueba si necesitamos recargar las pelusas. UpdateNextLoad está bien. Actualiza el tiempo para la próxima recarga. Eso es definitivamente una sola cosa.

Sin embargo, siento que LoadFluffies()no se puede describir como una sola cosa. Está obteniendo los datos de la base de datos, y está programando la próxima recarga. Es difícil argumentar que calcular el tiempo para la próxima recarga es parte de obtener los datos. Sin embargo, no puedo encontrar una mejor manera de hacerlo (cambiar el nombre de la función LoadFluffiesAndScheduleNextLoadpuede ser mejor, pero solo hace que el problema sea más obvio).

¿Existe una solución elegante para escribir realmente esta clase de acuerdo con el SRP? ¿Estoy siendo demasiado pedante?

¿O tal vez mi clase no está haciendo solo una cosa?

cuervo
fuente
3
Basado en "escrito en C #, pero eso no debería importar", "Esto se trata de la estructura del código", "Un ejemplo: ... No nos importa lo que es un Fluffy", "Para simplificar, digamos ...", Esta no es una solicitud de revisión de código, sino una pregunta sobre un principio general de programación.
200_success
@ 200_success gracias, y lo siento, pensé que esto sería adecuado para CR
cuervo
1
¡Tan esponjoso!
Mathieu Guindon
2
En el futuro, sería mejor con "widget" en lugar de esponjoso para futuras preguntas similares, ya que se entiende que un widget es un sustituto no particular para los ejemplos.
cuál es el
1
Sé que es solo un código de ejemplo, pero úselo DateTime.UtcNowpara evitar cambios de horario de verano o incluso un cambio en la zona horaria actual.
Mark Hurd el

Respuestas:

23

Si esta clase fuera realmente tan trivial como parece ser, entonces no habría necesidad de preocuparse por violar el SRP. Entonces, ¿qué pasa si una función de 3 líneas tiene 2 líneas haciendo una cosa y otra 1 línea haciendo otra cosa? Sí, esta función trivial viola el SRP, ¿y qué? ¿A quien le importa? La violación del SRP comienza a convertirse en un problema cuando las cosas se vuelven más complicadas.

Su problema en este caso particular probablemente se deba al hecho de que la clase es más complicada que las pocas líneas que nos ha mostrado.

Específicamente, el problema probablemente radica en el hecho de que esta clase no solo administra el caché, sino que también contiene la implementación del GetFluffiesFromDb()método. Entonces, la violación del SRP está en la clase, no en los pocos métodos triviales que se muestran en el código que publicó.

Entonces, aquí hay una sugerencia sobre cómo manejar todo tipo de casos que caen dentro de esta categoría general, con la ayuda del Patrón Decorador .

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

y se usa de la siguiente manera:

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

Tenga en cuenta que CachingFluffiesProvider.GetFluffies()no tiene miedo de contener el código que verifica y actualiza el tiempo, porque eso es algo trivial. Lo que hace este mecanismo es abordar y manejar el SRP a nivel de diseño del sistema, donde importa, no a nivel de pequeños métodos individuales, donde de todos modos no importa.

Mike Nakis
fuente
1
+1 por reconocer que las pelusas, el almacenamiento en caché y el acceso a la base de datos son en realidad tres responsabilidades. Incluso podría intentar hacer que la interfaz de FluffiesProvider y los decoradores sean genéricos (IProvider <Fluffy>, ...) pero esto podría ser YAGNI.
Roman Reiner
Honestamente, si solo hay un tipo de caché y siempre extrae objetos de la base de datos, en mi humilde opinión está excesivamente diseñado (incluso si la clase "real" podría ser más compleja como podemos ver en el ejemplo). La abstracción solo por abstracción no hace que el código sea más limpio o más fácil de mantener.
Doc Brown
El problema de @DocBrown es la falta de contexto para la pregunta. Me gusta esta respuesta porque muestra una forma en la que he usado una y otra vez en aplicaciones más grandes y porque es fácil escribir pruebas en contra, también me gusta mi respuesta porque es solo un pequeño cambio y produce algo claro sin un diseño excesivo. actualmente se encuentra, sin contexto, casi todas las respuestas aquí son buenas:]
stijn
1
FWIW, la clase que tenía en mente cuando hice la pregunta es más complicada que FluffiesManager, pero no demasiado. Unas 200 líneas, tal vez. No he hecho esta pregunta porque he encontrado algún problema con mi diseño (¿todavía?), Solo porque no pude encontrar una manera de cumplir estrictamente con el SRP, y eso podría ser un problema en casos más complejos. Entonces, la falta de contexto es algo intencionado. Creo que esta respuesta es genial.
cuervo
2
@stijn: bueno, creo que tu respuesta está muy poco votada. En lugar de agregar abstracciones innecesarias, simplemente corta y nombra las responsabilidades de manera diferente, que debería ser siempre la primera opción antes de acumular tres capas de herencia en un problema tan simple.
Doc Brown
6

Tu clase en sí me parece bien, pero tienes razón en eso LoadFluffies()no es exactamente lo que anuncia el nombre. Una solución simple sería cambiar el nombre y mover la recarga explícita de GetFluffies a una función con una descripción adecuada. Algo como

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

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

A mí me parece limpio (también porque, como dice Patrick: está compuesto de otras pequeñas funciones obedientes a SRP), y especialmente claro, que a veces es tan importante.

stijn
fuente
1
Me gusta la simplicidad en esto.
cuervo
6

Creo que tu clase está haciendo una cosa; Es un caché de datos con un tiempo de espera. LoadFluffies parece una abstracción inútil a menos que lo llame desde varios lugares. Creo que sería mejor tomar las dos líneas de LoadFluffies y ponerlas en el condicional NeedsReload en GetFluffies. Esto haría que la implementación de GetFluffies sea mucho más obvia y siga siendo un código limpio, ya que está componiendo subrutinas de responsabilidad única para lograr un solo objetivo, una recuperación en caché de datos de la base de datos. A continuación se muestra el método actualizado get fluffies.

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

    return m_Cache;
}
Patrick Goley
fuente
Si bien esta es una primera respuesta bastante buena, tenga en cuenta que el código de "resultado" a menudo es una buena adición.
Financia la demanda de Mónica el
4

Tus instintos son correctos. Tu clase, por pequeña que sea, está haciendo demasiado. Debe separar la lógica de almacenamiento en caché de actualización temporizada en una clase completamente genérica. Luego cree una instancia específica de esa clase para administrar Fluffies, algo como esto (no compilado, el código de trabajo se deja como ejercicio para el lector):

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

Una ventaja adicional es que ahora es muy fácil probar TimedRefreshCache.

Kevin Cline
fuente
1
Estoy de acuerdo en que si la lógica de actualización se vuelve más complicada que en el ejemplo, podría ser una buena idea refactorizarla en una clase separada. Pero no estoy de acuerdo con que la clase en el ejemplo, como es, haga demasiado.
Doc Brown
@kevin, no tengo experiencia en TDD. ¿Podría explicar cómo probaría TimedRefreshCache? No lo veo como "muy fácil", pero podría ser mi falta de experiencia.
cuervo
1
Personalmente no me gusta tu respuesta por su complejidad. Es muy genérico y muy abstracto y puede ser mejor en situaciones más complicadas. Pero en este caso simple es 'simplemente demasiado'. Por favor, eche un vistazo a la respuesta de stijn. Qué linda, breve y legible respuesta. Todos lo entenderán inmediatamente. ¿Qué piensas?
Dieter Meemken
1
@raven Puede probar TimedRefreshCache utilizando un intervalo corto (como 100 ms) y un productor muy simple (como DateTime.Now). Cada 100 ms, el caché producirá un nuevo valor, mientras tanto devolverá el valor anterior.
Kevin Cline
1
@DocBrown: El problema es que, tal como está escrito, no se puede probar. La lógica de temporización (comprobable) se combina con la lógica de la base de datos, que luego se burla. Una vez que se crea una costura para burlarse de la llamada a la base de datos, tiene el 95% del camino a la solución genérica. He descubierto que construir estas pequeñas clases generalmente vale la pena porque terminan siendo reutilizadas más de lo esperado.
Kevin Cline
1

Su clase está bien, SRP se trata de una clase, no de una función, toda la clase es responsable de proporcionar los "Fluffies" de la "Fuente de datos" para que tenga una implementación interna libre.

Si desea expandir el mecanismo de cahing, puede crear una clase respirable para ver la fuente de datos

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
fuente
Según el tío Bob: LAS FUNCIONES DEBEN HACER UNA COSA. Deben hacerlo bien. DEBEN HACERLO SOLO. Código limpio p.35.
cuervo