Dlaczego LOGIKA w KONTROLERACH to zło?
Być może spotkałeś się w internecie ze stwierdzeniem, że nie powinieneś umieszczać logiki w kontrolerach. A jeśli się nie spotkałeś w internecie to spotkasz się przy pierwszej okazji kiedy pokażesz komuś doświadczonemu kod swojej aplikacji webowej.
O co tutaj chodzi? Dlaczego nie powinno się tej logiki pakować do kontrolera? Jakie są tego wady? Tego dowiesz się w dalszej części tekstu.
Logika – co to takiego?
Na początek warto sobie odpowiedzieć na pytanie czym jest ta logika, której nie powinniśmy umieszczać w kontrolerach? Bo każdy tak o niej mówi, a właściwie rzadko się to pojęcie wyjaśnia. Co dla początkującego może nie być najbardziej komfortową sytuacją.
Najbardziej zwięzłą definicją jaką mógłbym tutaj podać jest taka, że logika to wszystko to co operuje na danych. Jeśli w jakiś sposób przetwarzasz dane to jest to logika. Taką logiką będzie więc walidacja danych, pobieranie danych z bazy, filtrowanie wyników, zmiana wartości w obiektach czy tworzenie tych obiektów.
Logiką nie będzie jedynie wywołanie ostatecznej funkcji uruchamiającej cały proces, np. funkcji „pobierz listę zamówień”. Bo mimo wszystko w końcu trzeba jakoś te dane do kontrolera dostarczyć. W tym celu mimo wszystko jakąś funkcję trzeba wywołać. Jednak dopiero ta funkcja powinna wykonywać jakiekolwiek operacje na danych, które być może nam finalnie zwróci.
Trzy argumenty za logiką poza kontrolerem
Pokażę tutaj przykład z faktycznej aplikacji, którą zacząłem jakiś czas temu pisać. Jest to prototyp i w tym momencie akcja kontrolera zawiera właśnie w sobie logikę. Jest to akcja zwracająca pojedynczy wydatek zapisany w bazie danych:
[HttpGet] [Authorize] [Route("{id}")] public EditExpenseDto GetSingle([FromQuery] string id) { var userId = Guid.Parse(User.Claims.First(x => x.Type == "UserId").Value); var expense = _context.Expenses.SingleOrDefault(x => x.Id == new Guid(id)); if (expense == null || expense.UserId != userId) { Response.StatusCode = StatusCodes.Status400BadRequest; return null; } return new EditExpenseDto() { Identifier = expense.Id.ToString(), Store = expense.Store, Category = expense.Category, Date = expense.Date.Value.ToLongDateString() }; }
Do przetestowania czy wszystko ma sens być może takie rozwiązanie by się sprawdziło. Jednak zobaczmy z czym wiąże się pozostawienie tej logiki w tym miejscu.
Zmiany w logice
Kod ma to do siebie, że się zmienia. Nawet w aplikacji, która ma określony cykl tworzenia i życia, a nawet konkretny i niezmienny zestaw funkcjonalności. Do pewnych rozwiązać programista bardzo często dochodzi iteracyjnie. Dopiero któreś z kolei rozwiązanie się sprawdzi i będzie tym ostatecznym. Zdarza się też, że pewne zachowania są wspólne dla kilku obszarów i dopiero końcowy etap przetwarzania danych się zmienia. W takim wypadku zmiana w logice będzie się odbywać równolegle w dwóch miejscach.
Jeżeli zostawimy logikę w kontrolerze, tak jak w przykładzie powyżej, to w przypadku jakiejkolwiek zmiany musimy za każdym razem przeszukiwać wszystkie obszary, w których dany zestaw instrukcji może się znaleźć. Jeśli w powyższym przypadku zmieni mi się sposób tworzenia DTO albo dojdą nowe dane to będę musiał to zmienić w każdym miejscu gdzie pobieram pojedynczy wydatek. I jest spora szansa, że o którymś z tych miejsc zapomnę.
Rozwój aplikacji
Istnieje całkiem niemała grupa projektów gdzie standardowo aplikacja zwraca swoje widoki użytkownikowi ale daje też dostęp do API z tymi samymi danymi. Załóżmy więc, że mamy kontroler z logiką, który zwraca widok, typowa aplikacja w ASP.NET MVC. Wszystko sobie działa spokojnie i w pewnym momencie dochodzi wymaganie „dodajmy API dla zewnętrznych dostawców”. W sytuacji kiedy cała logika znajdowała by się w osobnych serwisach, repozytoriach i innych tego typu tworach to sprawa jest prosta. Dodajemy kontroler dla API i wywołujemy tą samą funkcję. W przypadku logiki w kontrolerze mamy dwie opcje – skopiować cały kod i mieć go w dwóch miejscach, albo podjąć decyzję o refaktoryzacji i jednak przenieść go do osobnych klas. Pierwsze rozwiązanie odczujemy w przyszłości kiedy trzeba będzie coś zmienić, a drugie spowoduje wydłużenie się developmentu w danym momencie. Oba w jakimś momencie powodują stratę czasu, a więc i pieniędzy.
Testowanie
Dobrą praktyką jest żeby jednak do kodu pisać testy. W powyższym przykładzie napisanie testów może i będzie możliwe, ale prawdopodobnie po pierwszym ich napisaniu już nigdy nie będziecie chcieli wracać w to miejsce.
Jeżeli aplikacja zwraca w akcjach widoki to nie przetestujesz po prostu wartości zwróconej przez nią tylko będziesz testować np. ActionResult, który zawiera mnóstwo dodatkowych danych. Jeżeli dodatkowo w kontrolerze korzystasz z jakichś filtrów czy atrybutów, które są zdefiniowane w innych klasach to tak naprawdę przetestowanie „gołych” akcji i tak nie daje Ci informacji jak obiekty się zachowają w trakcie działania. Testowanie kontrolera może też wiązać się z koniecznością mockowania wielu mechanizmów frameworka albo testowaniem obiektów tego frameworka takich jak ViewState czy ViewBag.
W konsekwencji spora część Twoich testów „jednostkowych” może okazać się tak naprawdę testami frameworka, który ktoś już przetestował albo testami integracyjnymi, które nie zawsze są najlepszym albo pożądanym wyborem. Właściwie uważam, że testy akcji kontrolera, w których znajduje się logika zawsze będą testami integracyjnymi bo tak naprawdę nigdy nie oddzielimy się od tej warstwy frameworka.
Co można zrobić?
Krótka odpowiedź – przenieść całą logikę do serwisów albo innych handlerów :) Kod kontrolera w moim przypadku powinien wyglądać mniej więcej tak:
[HttpGet] [Authorize] [Route("{id}")] public EditExpenseDto GetSingle([FromQuery] string id) { var userId = Guid.Parse(User.Claims.First(x => x.Type == "UserId").Value); return _expenseService.GetSingleExpence(userId, id); }
Jedynie pobranie id użytkownika bym zostawił jako element związany z samym frameworkiem.
A odpowiadając w bardziej rozbudowany sposób to powinniśmy wydzielić wszystko co nie zależy bezpośrednio od kontrolera czy requestu do osobnych klas. Zależnie od potrzeb będą to serwisy, repozytoria, handlery czy jak to jeszcze nazwiemy. Chodzi o to, aby wszystko co się tam znajdzie mogło istnieć nie wiedząc w ogóle, że coś takiego jak kontroler istnieje.
Nawet jeżeli to co mam na przykładzie z początku posta przeniósłbym 1:1 do funkcji w osobnej klasie to pozbyłbym się problemów opisanych powyżej. Miałbym funkcję, którą mogę użyć zarówno w MVC jak i API. Mógłbym dużo łatwiej całą logikę przetestować mockując jedynie bazę danych. A w przypadku zmian np. w polach DTO nie musiałbym się zastanawiać czy ten fragment kodu nie jest przypadkiem używany w kilku miejscach, które powinienem zmodyfikować.
Hej
Czy dobrze rozumiem – _expenseService to obiekt, który wstrzykujesz poprzez DI w konstruktorze?
Pozdrawiam
Tak, dokładnie :) zapomniałem o tym wspomnieć
Siema Siema