Nie SOLID-nie #01: Single Responsibility Principle

N

Seria zainspirowana bardzo dobrym kursem SOLID od Jarka Stadnickiego, dostępnym na platformie Udemy – SOLID praktyczny kurs

(Nie jest to reklama, ani żadna afiliacja. Wyrażam swoje zdanie 🙂 )

 


Spis postów z serii Nie SOLID-nie:

  1. Nie SOLID-nie #01: Single Responsibility Principle
  2. Nie SOLID-nie #02: Open Close Principle
  3. Nie SOLID-nie #03: Liskov Substitution Principle
  4. Nie SOLID-nie #04: Interface Segregation Principle
  5. Nie SOLID-nie #05: Dependency Inversion Principle

SOLID

Wikipedia podaje, że SOLID to ukuty przez Roberta C. Martina mnemonik. Wystarczy jednak zapamiętać, że jest to zbiór zasad jakimi powinien się kierować programista, podczas pisania kodu. Zwłaszcza w paradygmacie programowania obiektowego.

Dużo się pokazuje jak na prawdę powinien wyglądać kod spełniający kryteria zawarte w poszczególnych regułach SOLIDa, natomiast niewiele się pokazuje przypadków niepoprawnego zastosowania tych reguł, lub nie zastosowania ich w ogóle.

Właśnie to będzie celem tego cyklu. Tylko i wyłącznie przykłady i ich opis, tego jak to NIE powinno wyglądać. Zapraszam na pierwszy post z cyklu „Nie SOLID-nie”.

 

 

Single Responsibility Principle

 

Reguła SRP mówi o tym, że każda klasa powinna „robić jedną rzecz”, powinna mieć jedną odpowiedzialność. Regułę tę można również stosować w ramach metod. W miarę jasne, nie?

 

Przykład

Na prędce napisana klasa MessageProcessor.  Na pierwszy rzut oka widać, że zajmuje się przetwarzaniem wiadomości. Nie ważne skąd, dokąd i czy poprawnie. W tej chwili ważne jest to co się dzieje podczas przetwarzania czyli w metodzie ProcessMessage gdzie ewidentnie naruszana jest zasada SRP poprez zapisywanie do bazy i odczytywanie z bazy, bezpośrednio w tej metodzie, tej klasy. Dla kontrastu, również w tej samej metodzie, pokazuję działania na bazie – jak to się mówi – na data setach, jak również przy pomocy obiektu _dbClient. Przy czym, gdyby to miało być zrobione „po bożemu”, obiekt _dbClient byłby wstrzyknięty w konstruktorze klasy MessageProcessor, a nie w nim zainicjalizowany. Akurat takimi przypadkami zajmiemy się pod koniec cyklu.

Widać, że klasa ta mogłaby pewne działania „delegować” na zewnątrz, właśnie za pomocą jakiegoś obiektu wstrzykniętego poprzez konstruktor.

public class MessageProcessor
{
    private readonly Thread _messageProcessorThread;
    private DatabaseClient _dbClient;
    private readonly DatabaseAccess _dbAccess;

    private readonly var _messagesQueue = new ConcurrentQueue<Message>();

    public void AddMessage(Message msg)
    {
        _messagesQueue.Enqueue(msg);
    }

    public bool StopProcessor()
    {
        try
        {
            _processorThread.Abort();
            while (!_messagesQueue.IsEmpty)
            {
                Message msg;
                _messagesQueue.TryDequeue(out msg);
            }

            return true;
        }
        catch (Exception ex)
        {
            return false;
        }
    }

    public MessageProcessor(string processorId, string DBAddress, string DBUser,
        string DBPassword, string DBName)
    {
        _dbClient = new DbClient(Convert.ToInt32(processorId))
        {
            DatabaseAddress = DBAddress,
            DatabaseUserName = DBUser,
            DatabasePassword = DBPassword,
            DatabaseName = DBName
        };

        _dbClient.ConnectToDatabase();
        
        _messageProcessorThread = new Thread(MessageProcessing)
        {
            Name = "MessageProcessorThreadId: " + processorId
        };
        _messageProcessorThread.Start();
    }

    private void ProcessMessage(Message m)
    {
        switch ((int) m.MessageType)
        {
            case (int) MessageTypes.UNKNOWN:
            case (int) MessageTypes.UNKNOWN2:
                SendACK(m, 0);
                break;
            case (int) MessageTypes.ERROR:
            {
                DataTable dt = databaseClient.GetEntity(m.LocationID);
                foreach (DataRow dr in dt.Rows)
                {
                    foreach (Entity entity in m.entities)
                    {
                        int entityId = Convert.ToInt32(dr["Id"]);
                        string drSubType = Convert.ToString(dr["EntitySubType"]);
                        int entityTypeId = Convert.ToInt32(dr["EntityTypeID"]);
                        string subtype = entity.subtype;
                        int type = entity.type;
                        int result = databaseClient.SaveTranssactions(entityId,
                            entity.ValueBefore, entity.ValueAfter);
                    }
                }

                SendACK(m, 0);
            } 
                break;
            case (int) MessageTypes.EVENT:
            {
                DataTable dt = new DataTable();
                int nresult = _dbClient.EventInsert(m.transactionId, m.locationId,
                    m.EventType, m.TimeStamp, m.EventStatus);
                SendACK(m, 0);
            }
                break;
            default:
                SendACK(m, 0);
                break;
        }
    }
}

 

Inny przykład

Kolejnym przykładem jest klasa kontrolera i jego akcje. Tym razem, naruszenie zasady SRP nie jest tak oczywiste, jak mogłoby się wydawać. Klasa UserController, tak jak w poprzednim przykładnie również „ogarnia” za dużo odpowiedzialności, ale jest jeszcze jedno miejsce warte uwagi. Spójżcie w metodę EditUserAccount, co prawda robi to co ma robić i do czego została przewidziana, ale również wywołuje metodę prywatną CheckAccessRight. Natomiast metoda CheckAccessRight enkapsuluje osobne serwisy, wstrzykiwane w kontstruktorze klasy. Należy się zastanowić, czy nie byłoby dobrym krokiem, wyciągnięcie metody CheckAccessRight na zewnątrz do innej klasy.

public class UserController : ApiController
{
    private IUserService _userService;
    private IAccessRightsService _accessRightsService;
    private ICountersService _countersService;

    public UserController(IUserService userService,
        IAccessRightsService accessRightsService,
        ICountersService countersService)
    {
        _userService = userService;
        _accessRightsService = accessRightsService;
        _countersService = countersService;
    }

    [HttpGet("api/activate/{userId:int}")]
    public Task<HttpActionResult> ActivateUserAccount(int userId)
    {
        var response = _userService.ActivateUserAccount(userId);
        return Task.FromResult(response);
    }

    [HttpPut("api/edit/{userId:int}")]
    public void EditUserAccount(int userId)
    {
        if (CheckAccessRight())
            _userService.EditUserAccount(userId);
    }

    [HttpPut("api/login/{user}/{password}")]
    public void LoginUser(string user, string password)
    {
        _userService.LoginUser(user, password);
    }

    private bool CheckAccessRight()
    {
        _countersService.BumpAccessRightCounter();
        return _accessRightsService.CheckAccessRight();
    }
}

 

Reasumując

Prawda jest taka, że tej zasady nie można traktować dosłownie i nie można jej egzekwować w stu procentach. Zwłaszcza to drugie.

 

Zalecam rozsądek

 

Nie możemy wydzielać każdej możliwej funkcjonalności do osobnej klasy ze względu na to, że im wyższa abstrakcja, tym trudniej nam będzie używać tych klas. Wyobrażacie sobie wstrzykiwanie do jakiegoś konstruktora dziesięć serwisów? Tego się nie da używać, a już na pewno nie jest przyjemnym mockowanie takiej klasy i pisanie testów jednostkowych.

Jak zawsze trzeba znaleźć złoty środek.

Nie tworzyć nadmiarowej ilości klas w procesie „wydzielania odpowiedzialności”.

Starać się, aby klasy i metody nie realizowały zbyt dużej ilości różnych zadań.

Odpowiednio enkapsulować składowe klas.

 

Napisz komentarz! Dodaj coś od siebie!

Czołem!

 



 

About the author

5 komentarzy

  • masz rację, jak poznałem Solida, nie tak dawno (wcześniej znałem z teorii) zaczołem się zastanawiać czy jeżeli chcę żeby aplikacja była solid to muszę stosować wszystkie pięć zasad? Z trzeciej to ja jeszcze nigdy nie skorzystałam, ale czy przez to moje aplikacje są mniej solid? Solid ro zbiór reguł nie wytycznych, a czy korzystamy i jak korzystamy z tego to wszystko zależy od nas (od kontekstu). Problem ze wszystkimi regułami i zasadami jest taki że jeżeli to wszystko zastosujemy to nasz kod będzie mniej czytelny niż wcześniej.
    Masz racje u miar trzeba znać, tylko kiedy mogę powiedzieć sobie dość?

    • Ja zwykłem zawsze mówić, że kod powinien być czytelny jak dobra książka fabularna, beletrystyka. Powinien się czytać jak tekst z fabułą, prawie z bohaterami. To jest, moim zdaniem, najważniejsze. Reguły takie jak SOLID, czy KISS, czy YAGNI, czy DRY są jedynie jak reguły które mówą jak powinien wyglądać artykuł w gazecie, albo jak powinno wyglądać wypracowanie. Czytelnik dzięki temu lepiej rozumie to co czyta, a pamiętajmy, że kod najczęściej jest czytany :).

  • Hej, dzięki za fajny post!
    W czasach juniorskich było mi ciężko wyrobić sobie intuicję co do sformułowania „Robi jedną rzecz”, bo przecież przetwarzanie wiadomości albo edycja profilu użytkownika to w końcu jedna rzecz, prawda? Z pomocą przyszła mi alternatywna definicja, a mianowicie „Robi jedną rzecz” => „Istnieje tylko jeden powód do zmiany w klasie”. Wtedy zmiana schematu DB => zmiana w Procesorze, zmiana sposobu przetwarzania wiadomości => zmiana w Procesorze. Zatem widać, że zmiana w dwóch różnych obszarach aplikacji generuje zmianę w tej samej klasie. Zatem naruszyliśmy tutaj SRP.
    Twój post wpadł mi w oko również z innego powodu: pracuję aktualnie nad serią postów o typach i klasach w .NET/C#, i jeden z tych postów dotyczyć będzie literki L (Liskov Substitution Principle), i również omawiam go przez podanie przykładów łamania dobrych praktyk związanych z typami i projektowaniem zachowań. Podrzucę linka, gdy uda mi się skończyć serię (ach te postanowienia noworoczne;)) Co powiesz na gościnny post w Twojej serii?:)

    Pozdrawiam,
    Tomek

    • Zastanawiam się nad Twoją, alternatywną definicją SRP. I muszę to sobie przyswoić.
      Co do propozycji gościnnego posta – to świetny pomysł! Odezwę się jutro :).

      • To nie jest alternatywna definicja, tylko oryginalna definicja Roberta C. Martina. To że 90% programistów ją rozumie po swojemu to inna sprawa. Tutaj wytłumaczona jeszcze raz przez autora: https://blog.cleancoder.com/uncle-bob/2014/05/08/SingleReponsibilityPrinciple.html

        Another wording for the Single Responsibility Principle is:

        Gather together the things that change for the same reasons. Separate those things that change for different reasons.
        If you think about this you’ll realize that this is just another way to define cohesion and coupling. We want to increase the cohesion between things that change for the same reasons, and we want to decrease the coupling between those things that change for different reasons.

        However, as you think about this principle, remember that the reasons for change are people. It is people who request changes. And you don’t want to confuse those people, or yourself, by mixing together the code that many different people care about for different reasons.

By Patryk

Autor serwisu

Patryk

Społecznościowe

Instagram

Newsletter



Historycznie

Tagi