Как избежать нарушения SRP в классе для управления кэшированием?

12

Примечание. Пример кода написан на c #, но это не должно иметь значения. Я поставил c # в качестве тега, потому что не могу найти более подходящий. Это о структуре кода.

Я читаю Чистый код и пытаюсь стать лучшим программистом.

Я часто сталкиваюсь с трудностями следования принципу единой ответственности (классы и функции должны выполнять только одно), особенно в функциях. Может быть, моя проблема в том, что «одна вещь» не очень четко определена, но все же ...

Пример: у меня есть список Пушистиков в базе данных. Нам не важно, что такое Пушистик. Я хочу класс, чтобы восстановить пушистики. Тем не менее, пушистые могут меняться в соответствии с некоторой логикой. В зависимости от логики, этот класс будет возвращать данные из кэша или получать последние данные из базы данных. Можно сказать, что он управляет пушистиками, и это одно. Для простоты, скажем, загруженные данные хороши в течение часа, а затем их необходимо перезагрузить.

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()кажется, хорошо для меня. Пользователь просит несколько пушистиков, мы их предоставляем. Собираюсь восстановить их из БД, если это необходимо, но это можно считать частью получения пушистых копий (конечно, это несколько субъективно).

NeedsReload()тоже кажется правильным. Проверяет, нужно ли перезагрузить пушистики. UpdateNextLoad в порядке. Обновляет время для следующей перезагрузки. это определенно одна вещь.

Однако я чувствую, что LoadFluffies()нельзя описать как одну вещь. Он получает данные из базы данных и планирует следующую перезагрузку. Трудно утверждать, что вычисление времени для следующей перезагрузки является частью получения данных. Тем не менее, я не могу найти лучший способ сделать это (переименование функции LoadFluffiesAndScheduleNextLoadможет быть лучше, но это только делает проблему более очевидной).

Есть ли элегантное решение, чтобы действительно написать этот класс в соответствии с SRP? Я слишком педантичен?

Или, может быть, мой класс на самом деле не делает только одну вещь?

вороной
источник
3
Основано на «написано в C #, но это не должно иметь значения», «Речь идет о структуре кода», «Пример:… Нам все равно, что такое Fluffy», «Чтобы упростить его, скажем так…», это не просьба о пересмотре кода, а вопрос об общем принципе программирования.
200_успех
@ 200_success спасибо, и извините, я думал, что это будет достаточно для CR
ворон
1
Такой пушистый!
Матье Гиндон
2
В будущем вам будет лучше использовать «виджет» вместо «пушистый» для будущих подобных вопросов, так как виджет понимается как неконкретный заменитель примеров.
whatsisname
1
Я знаю, что это только пример кода, но используйте его, DateTime.UtcNowчтобы избежать перехода на летнее время или даже изменения текущего часового пояса.
Марк Херд

Ответы:

23

Если бы этот класс был таким же тривиальным, как кажется, тогда не было бы необходимости беспокоиться о нарушении SRP. Так что, если 3-строчная функция имеет 2 строки, выполняющие одно, а еще 1 строку, выполняющую другое действие? Да, эта тривиальная функция нарушает ПСП, и что с того? Какая разница? Нарушение SRP становится проблемой, когда все становится сложнее.

Ваша проблема в данном конкретном случае, скорее всего, связана с тем, что класс более сложен, чем несколько строк, которые вы нам показали.

В частности, проблема, скорее всего, заключается в том, что этот класс не только управляет кэшем, но также, вероятно, содержит реализацию GetFluffiesFromDb()метода. Таким образом, нарушение SRP относится к классу, а не к нескольким тривиальным методам, показанным в опубликованном вами коде.

Итак, вот предложение о том, как обрабатывать все виды дел, которые попадают в эту общую категорию, с помощью шаблона Decorator .

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

и он используется следующим образом:

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

Обратите внимание, как CachingFluffiesProvider.GetFluffies()не боится содержать код, который выполняет проверку и обновление времени, потому что это тривиальные вещи. Этот механизм выполняет и обрабатывает SRP на уровне проектирования системы, где это имеет значение, а не на уровне крошечных отдельных методов, где это все равно не имеет значения.

Майк Накис
источник
1
+1 за признание того, что пушистость, кэширование и доступ к базе данных на самом деле являются тремя обязанностями. Можно даже попытаться сделать интерфейс FluffiesProvider и декораторы общими (IProvider <Fluffy>, ...), но это может быть YAGNI.
Роман Райнер
Честно говоря, если существует только один тип кэша, и он всегда извлекает объекты из базы данных, это ИМХО сильно перегружено (даже если «настоящий» класс может быть более сложным, как мы можем видеть в примере). Абстракция только ради абстракции не делает код чище или более понятным.
Док Браун
Проблема @DocBrown - отсутствие контекста вопроса. Мне нравится этот ответ, потому что он показывает способ, который я использовал снова и снова в более крупных приложениях, и, поскольку на него легко писать тесты, мне также нравится мой ответ, потому что это только небольшое изменение и дает что-то ясное без какого-либо чрезмерного проектирования, так как в настоящее время стоит, без контекста почти все ответы здесь хороши:]
stijn
1
FWIW, класс, который я имел в виду, когда задавал вопрос, является более сложным, чем FluffiesManager, но не слишком. Может быть, около 200 строк. Я не задавал этот вопрос, потому что я обнаружил какие-либо проблемы с моим дизайном (пока?), Просто потому, что не смог найти способ строго соблюдать SRP, и это может быть проблемой в более сложных случаях. Таким образом, отсутствие контекста несколько задумано. Я думаю, что этот ответ отличный.
ворон
2
@stijn: ну, я думаю, что ваш ответ сильно недооценен. Вместо того, чтобы добавлять ненужную абстракцию, вы просто сокращаете и называете обязанности по-разному, что всегда должно быть первым выбором, прежде чем складывать три уровня наследования в такую ​​простую проблему.
Док Браун
6

Сам твой класс кажется мне подходящим, но ты прав в том, LoadFluffies()что не совсем то, что имя рекламирует. Одним простым решением было бы изменить имя и переместить явную перезагрузку из GetFluffies в функцию с соответствующим описанием. Что-то вроде

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

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

выглядит чистым для меня (также потому что, как говорит Патрик: он состоит из других крошечных функций, подчиняющихся SRP), и особенно ясным, что иногда так же важно.

Стейн
источник
1
Мне нравится простота в этом.
ворон
6

Я верю, что ваш класс делает одну вещь; это кэш данных с таймаутом. LoadFluffies кажется бесполезной абстракцией, если вы не вызываете ее из нескольких мест. Я думаю, что было бы лучше взять две строки из LoadFluffies и поместить их в условие NeedsReload в GetFluffies. Это сделало бы реализацию GetFluffies намного более очевидной и по-прежнему чистым кодом, поскольку вы составляете подпрограммы с одной ответственностью для достижения единой цели - кэшированного извлечения данных из базы данных. Ниже приведен обновленный метод get fluffies.

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

    return m_Cache;
}
Патрик Гоули
источник
Хотя это довольно хороший первый ответ, имейте в виду, что код «результата» часто является хорошим дополнением.
Фонд Моники судебный процесс
4

Ваши инстинкты верны. Ваш класс, пусть и маленький, делает слишком много. Вы должны разделить логику кэширования обновления по времени в полностью общий класс. Затем создайте конкретный экземпляр этого класса для управления Fluffies, что-то вроде этого (не скомпилировано, рабочий код оставлен в качестве упражнения для читателя):

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

Дополнительным преимуществом является то, что теперь очень легко протестировать TimedRefreshCache.

Кевин Клайн
источник
1
Я согласен, что если логика обновления становится более сложной, чем в примере, было бы неплохо преобразовать ее в отдельный класс. Но я не согласен с тем, что класс в этом примере делает слишком много.
Док Браун
@ Кевин, я не опытный в TDD. Не могли бы вы рассказать, как бы вы протестировали TimedRefreshCache? Я не считаю это «очень легким», но это может быть моим недостатком опыта.
ворон
1
Мне лично не нравится твой ответ из-за его сложности. Это очень обобщенно и очень абстрактно и может быть лучше в более сложных ситуациях. Но в этом простом случае это «просто много». Пожалуйста, взгляните на ответ Стийна. Какой хороший, короткий и читаемый ответ. Все сразу поймут это. Как вы думаете?
Дитер Мимкен
1
@raven Вы можете протестировать TimedRefreshCache, используя короткий интервал (например, 100 мс) и очень простой источник (например, DateTime.Now). Каждые 100 мсек кэш будет выдавать новое значение, а между тем он будет возвращать предыдущее значение.
Кевин Клайн
1
@DocBrown: проблема в том, что, как написано, это невозможно проверить. Логика синхронизации (тестируемая) связана с логикой базы данных, которая затем подвергается проверке. После создания шва для имитации вызова базы данных вы на 95% пути к общему решению. Я обнаружил, что создание этих маленьких классов, как правило, окупается, потому что они в конечном итоге используются больше, чем ожидалось.
Кевин Клайн
1

Ваш класс в порядке, SRP - это класс, а не функция, весь класс отвечает за предоставление «Пушистиков» из «Источника данных», поэтому вы свободны во внутренней реализации.

Если вы хотите расширить механизм кэширования, вы можете создать класс, отвечающий за просмотр источника данных.

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
источник
По словам дяди Боба: функции должны делать одно. ОНИ ДОЛЖНЫ СДЕЛАТЬ ЭТО ХОРОШО. ОНИ ДОЛЖНЫ СДЕЛАТЬ ЭТО ТОЛЬКО. Чистый код с.35.
ворон