Jak uniknąć naruszenia SRP w klasie w celu zarządzania buforowaniem?

12

Uwaga: Próbka kodu jest napisana w języku c #, ale to nie powinno mieć znaczenia. Umieściłem c # jako tag, ponieważ nie mogę znaleźć bardziej odpowiedniego. Chodzi o strukturę kodu.

Czytam Clean Code i staram się zostać lepszym programistą.

Często staram się przestrzegać zasady pojedynczej odpowiedzialności (klasy i funkcje powinny robić tylko jedną rzecz), szczególnie w funkcjach. Może moim problemem jest to, że „jedna rzecz” nie jest dobrze zdefiniowana, ale nadal…

Przykład: Mam listę puchu w bazie danych. Nie obchodzi nas, czym jest Puszysty. Chcę, żeby klasa odzyskała puszyste. Jednak puchary mogą się zmieniać zgodnie z pewną logiką. W zależności od logiki ta klasa zwróci dane z pamięci podręcznej lub pobierze najnowsze dane z bazy danych. Można powiedzieć, że zarządza puszystymi, i to jedno. Aby to uprościć, załóżmy, że załadowane dane są dobre przez godzinę, a następnie należy je ponownie załadować.

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()wydaje mi się w porządku. Użytkownik prosi o niektóre puszyste, zapewniamy je. W razie potrzeby odzyskanie ich od DB, ale można to uznać za część zdobywania puchu (to oczywiście nieco subiektywne).

NeedsReload()wydaje się również słuszne. Sprawdza, czy musimy ponownie załadować puchaty. UpdateNextLoad jest w porządku. Aktualizuje czas następnego przeładowania. to zdecydowanie jedna rzecz.

Czuję jednak, że tego, LoadFluffies()czego nie można opisać jako jednej rzeczy. Pobiera dane z bazy danych i planuje następne przeładowanie. Trudno argumentować, że obliczenie czasu do następnego przeładowania jest częścią pozyskiwania danych. Nie mogę jednak znaleźć lepszego sposobu na zrobienie tego (zmiana nazwy funkcji LoadFluffiesAndScheduleNextLoadmoże być lepsza, ale sprawia to, że problem jest bardziej oczywisty).

Czy istnieje eleganckie rozwiązanie, aby naprawdę napisać tę klasę według SRP? Czy jestem zbyt pedantyczny?

A może moja klasa tak naprawdę nie robi tylko jednej rzeczy?

kruk
źródło
3
Na podstawie „napisanego w C #, ale to nie powinno mieć znaczenia”, „Chodzi o strukturę kodu”, „Przykład:… Nie obchodzi nas, czym jest Fluffy”, „Aby uprościć, powiedzmy…”, nie jest to prośba o sprawdzenie kodu, ale pytanie o ogólną zasadę programowania.
200_success
@ 200_success dziękuję i przepraszam, myślałem, że to będzie odpowiednie dla CR
kruk
1
Taki puszysty!
Mathieu Guindon
2
W przyszłości lepiej byłoby używać „widżetu” zamiast puszystych w przypadku podobnych pytań w przyszłości, ponieważ widżet jest rozumiany jako niespecyficzny stojak na przykłady.
whatsisname
1
Wiem, że to tylko przykładowy kod, ale używaj go, DateTime.UtcNowaby uniknąć zmiany czasu na letni lub nawet zmiany bieżącej strefy czasowej.
Mark Hurd

Odpowiedzi:

23

Jeśli ta klasa naprawdę była tak trywialna, jak się wydaje, to nie trzeba się martwić o naruszenie SRP. A co jeśli funkcja 3-liniowa ma 2 linie robiące jedną rzecz, a inna 1 linia robi inną rzecz? Tak, ta trywialna funkcja narusza SRP i co z tego? Kogo to obchodzi? Naruszenie SRP staje się problemem, gdy sprawy stają się bardziej skomplikowane.

Twój problem w tym konkretnym przypadku najprawdopodobniej wynika z faktu, że klasa jest bardziej skomplikowana niż kilka linii, które nam pokazałeś.

W szczególności problem najprawdopodobniej polega na tym, że ta klasa nie tylko zarządza pamięcią podręczną, ale prawdopodobnie także zawiera implementację GetFluffiesFromDb()metody. Naruszenie SRP ma miejsce w klasie, a nie w kilku trywialnych metodach pokazanych w opublikowanym kodzie.

Oto sugestia, jak radzić sobie z wszelkiego rodzaju przypadkami, które należą do tej ogólnej kategorii, za pomocą Wzoru Dekoratora .

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

i jest używany w następujący sposób:

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

Zauważ, że CachingFluffiesProvider.GetFluffies()nie boję się zawrzeć kodu, który sprawdza czas i aktualizuje, ponieważ to trywialne rzeczy. Mechanizm ten zajmuje się i radzi sobie z SRP na poziomie projektowania systemu, tam gdzie ma to znaczenie, a nie na poziomie drobnych indywidualnych metod, gdzie i tak nie ma to znaczenia.

Mike Nakis
źródło
1
+1 za rozpoznanie, że puszyste, buforowanie i dostęp do bazy danych to tak naprawdę trzy obowiązki. Możesz nawet spróbować uczynić interfejs FluffiesProvider i dekoratorami ogólnymi (IProvider <Fluffy>, ...), ale może to być YAGNI.
Roman Reiner
Szczerze mówiąc, jeśli istnieje tylko jeden typ pamięci podręcznej i zawsze pobiera obiekty z bazy danych, jest to IMHO mocno przeprojektowane (nawet jeśli „prawdziwa” klasa może być bardziej złożona, jak widać na przykładzie). Abstrakcja tylko ze względu na abstrakcję nie czyni kodu czystszym ani łatwiejszym w utrzymaniu.
Doc Brown
Problemem @DocBrown jest brak kontekstu do pytania. Podoba mi się ta odpowiedź, ponieważ pokazuje sposób, w jaki wielokrotnie używałem w większych aplikacjach, a ponieważ łatwo jest pisać testy, podoba mi się również moja odpowiedź, ponieważ jest to tylko niewielka zmiana i daje coś wyraźnego, bez nadmiernego projektowania, ponieważ obecnie stoi, bez kontekstu prawie wszystkie odpowiedzi tutaj są dobre:]
stijn
1
FWIW, klasa, którą miałem na myśli, gdy zadałem pytanie, jest bardziej skomplikowana niż FluffiesManager, ale nie przesadnie. Może jakieś 200 linii. Nie zadałem tego pytania, ponieważ znalazłem jakiś problem z moim projektem (jeszcze?), Tylko dlatego, że nie mogłem znaleźć sposobu na ścisłe przestrzeganie SRP, a może to być problem w bardziej złożonych przypadkach. Zatem brak kontekstu jest w pewnym sensie zamierzony. Myślę, że ta odpowiedź jest świetna.
kruk
2
@stijn: myślę, że twoja odpowiedź jest mocno niedoceniona. Zamiast dodawania niepotrzebnej abstrakcji, po prostu wycinasz i nazywasz obowiązki inaczej, co powinno być zawsze pierwszym wyborem przed złożeniem trzech warstw dziedziczenia do tak prostego problemu.
Doc Brown
6

Sama klasa wydaje mi się w porządku, ale masz rację, że LoadFluffies()nie dokładnie to, co reklamuje nazwa. Jednym prostym rozwiązaniem byłaby zmiana nazwy i przeniesienie jawnego przeładowywania z GetFluffies do funkcji z odpowiednim opisem. Coś jak

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

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

wygląda mi na czystą (również dlatego, że jak mówi Patrick: składa się z innych drobnych funkcji posłusznych SRP), a szczególnie wyraźnie, co jest czasami równie ważne.

stijn
źródło
1
Podoba mi się w tym prostota.
kruk
6

Wierzę, że twoja klasa robi jedną rzecz; to pamięć podręczna z limitem czasu. LoadFluffies wydaje się bezużyteczną abstrakcją, chyba że wywołasz ją z wielu miejsc. Myślę, że lepiej byłoby pobrać dwie linie z LoadFluffies i umieścić je w warunku NeedsReload w GetFluffies. Uczyniłoby to implementację GetFluffies o wiele bardziej oczywistą i nadal jest czystym kodem, ponieważ komponujesz pojedyncze podprogramy odpowiedzialności, aby osiągnąć jeden cel, czyli buforowane pobieranie danych z bazy danych. Poniżej znajduje się zaktualizowana metoda zdobywania puchu.

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

    return m_Cache;
}
Patrick Goley
źródło
Chociaż jest to całkiem dobra pierwsza odpowiedź, należy pamiętać, że kod „wynikowy” jest często dobrym dodatkiem.
Pozew funduszu Moniki
4

Twoje instynkty są prawidłowe. Twoja klasa, choć niewielka, robi za dużo. Powinieneś podzielić logikę buforowania odświeżania czasowego na całkowicie ogólną klasę. Następnie utwórz specyficzną instancję tej klasy do zarządzania Fluffies, coś takiego: (nieskompilowany, działający kod pozostanie jako ćwiczenie dla czytelnika):

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

Dodatkową zaletą jest to, że teraz bardzo łatwo można przetestować TimedRefreshCache.

Kevin Cline
źródło
1
Zgadzam się, że jeśli logika odświeżania stanie się bardziej skomplikowana niż w przykładzie, dobrym pomysłem może być przefakturowanie jej do osobnej klasy. Ale nie zgadzam się, że klasa w tym przykładzie robi zbyt wiele.
Doc Brown
@kevin, nie mam doświadczenia w TDD. Czy możesz opracować sposób testowania TimedRefreshCache? Nie uważam tego za „bardzo łatwe”, ale może to być mój brak wiedzy.
kruk
1
Osobiście nie podoba mi się twoja odpowiedź z powodu jej złożoności. Jest bardzo ogólny i bardzo abstrakcyjny i może być najlepszy w bardziej skomplikowanych sytuacjach. Ale w tym prostym przypadku jest to „po prostu za dużo”. Proszę spojrzeć na odpowiedź stijn. Cóż za miła, krótka i czytelna odpowiedź. Wszyscy zrozumieją to natychmiast. Co myślisz?
Dieter Meemken
1
@raven Możesz przetestować TimedRefreshCache przy użyciu krótkich odstępów czasu (np. 100 ms) i bardzo prostego producenta (np. DateTime.Now). Co 100 ms pamięć podręczna będzie generować nową wartość, a pomiędzy nimi zwróci poprzednią wartość.
kevin cline
1
@DocBrown: Problem polega na tym, że jak napisano, jest nie do przetestowania. Logika taktowania (testowalna) jest sprzężona z logiką bazy danych, którą następnie można wyśmiewać. Po utworzeniu szwu, aby wyśmiewać połączenie z bazą danych, jesteś w 95% drogi do ogólnego rozwiązania. Przekonałem się, że budowanie tych małych klas zwykle się opłaca, ponieważ w końcu są wykorzystywane częściej niż oczekiwano.
kevin cline
1

Twoja klasa jest w porządku, SRP dotyczy klasy, a nie funkcji, cała klasa jest odpowiedzialna za dostarczanie „Fluffies” z „Source Data”, więc jesteś wolny w wewnętrznej implementacji.

Jeśli chcesz rozwinąć mechanizm cahing, możesz stworzyć klasę odpowiedzialną za oglądanie źródła danych

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
źródło
Według wujka Boba: FUNKCJE POWINNY BYĆ ZROBIONE. POWINNY ZROBIĆ TO DOBRZE. POWINIEN ZROBIĆ TYLKO. Wyczyść kod s.35.
kruk