Typowe błędy programistów.

Pisałem już kiedyś o błędach popełnianych przez młodych programistów. Byłem wtedy na początku swojej programistycznej kariery i myślałem, że już dużo wiem. Teraz wiem ile jeszcze nie wiem 😉 Niemniej jednak z większością z tamtych punktów zgadzam się do dziś. Postanowiłem więc, po kilku latach jeszcze raz zebrać kilka najczęstszych błędów, które zauważyłem już w aplikacjach enterprise. Poniższe punkty nazwałem błędami architektonicznymi, czyli odnoszącymi się bezpośrednio do kodu programu. Jak wiadomo praca programisty składa się jeszcze z wielu innych aktywności, ale o nich napiszę w innym poście.

1. Nadużywanie statycznych metod

Statyczne metody nie powinny być zbyt często używane. Programiści często idą na skróty i dopisują funkcje statyczne, które zawierają logikę biznesową bądź zmieniają stan aplikacji. Logika biznesowa w metodach statycznych to najgorsze, co może być. Dlaczego? Dlatego, że dla takiej metody jest bardzo trudno napisać jakikolwiek test jednostkowy. Metody statyczne powinny być używane dla bardzo prostych helperów, służących tylko do prostych obliczeń, niewymagających testowania. Chodź i tak do tego lepiej nadają się extension methods. Przykład błędnego użycia metody statycznej znajdziecie poniżej.

var user = User.GetUserById(userId);

public class User
{
    public static User GetUserById(int userId)
    {
        //Implementation
    }
}

Zamiast tego powinno się używać serwisów, które mogą być wstrzyknięte poprzez DI i z łatwością zmockowane.

2. Niekorzystanie z interfejsów

Punkt ten często wiąże się z poprzednim, gdyż metod statycznych nie można umieścić w interfejsie. Często myślimy, że jeśli aktualnie do dostępu do bazy używamy ADO.NET, to tak będzie już zawsze i tworzymy metody pobierające/zapisujące dane w klasie, do której odwołujemy się bezpośrednio. Niestety takie podejście ma bardzo wiele wad. Przede wszystkim, tak samo jak w przypadku metod statycznych bardzo ciężko jest testować taki kod. Nie można stworzyć żadnego mocka, który symulowałby nam dostęp do bazy danych. Drugim problemem z tym związanym jest brak możliwości zmiany aktualnie używanego rozwiązania. Jeśli już w całym programie używa się bezpośrednich połączeń do metod z ADO.NET, to nie przepiszemy wszystkiego na Entity Framework w jeden tydzień.

To co powinno się robić, to używać interfejsów zawsze. Nawet jeśli sposób implementacji w danym momencie jest tylko jeden.

3. Wrzucanie funkcji do jednego worka

Jeśli myślimy zbyt obiektowo, nadchodzi czas gdy biznes przerasta nasze obiektowe pojęcie. Wtedy właśnie zdajemy sobie sprawę, że nasza klasa User trochę się rozrosła i każda operacja mająca cokolwiek wspólnego z użytkownikiem, nie powinna znajdować się w tej jednej klasie. Często osoby, które nie stosują żadnych wzorców architektonicznych, popadają w pułapkę klas Bogów. Zamiast myśleć o obiektach jako encjach bazodanowych, należy pomyśleć o obiektach w kontekście biznesu. Bardzo przydatna w zrozumieniu tego zagadnienia jest książka Domain Driven Design. Pomocne w rozdzieleniu funkcji mogą być również wzorce projektowe, takie jak CQRS.

4. Wykorzystywanie tych samych DTO w wielu, niepowiązanych miejscach.

Najtrudniejsze w programowaniu jest wymyślanie nazw. Dlatego często idziemy na łatwiznę i nadajemy nic nie znaczące nazwy. Załóżmy, że mamy klasę do edycji użytkownika, więc nazwiemy ją UserDto. Po kilku miesiącach zapominamy, piszemy inną funkcjonalność i dajmy na to, że musimy wyświetlić wszystkich użytkowników z daną rolą. Jakiej klasy użyjemy? Prawdopodobnie UserDto, pomijając, że klasa ta posiada znacznie więcej pól niż potrzeba przy prostym wyświetleniu użytkowników. Jest to kolejny błąd, który ciężko naprawić, bo im więcej odwołań do danej klasy tym ta klasa jest trudniej edytowalna.

5. Pisanie nieczytelnych funkcji

Temat pisania nieczytelnych funkcji, poruszany jest w wielu wątkach i opisywany w niejednej książce, mimo to myślę, że wciąż są problemy z czytelnym pisaniem funkcji. Przede wszystkim problemy występują w instrukcjach warunkowych, które potrafią być całkiem sporym kawałkiem logiki. Istnieje nawet kampania anty-ifowa, której członkowie namawiają do programowania bez użycia instrukcji if oraz switch. Nie jestem za popadaniem ze skrajności w skrajność i wiem, że czasami użycie if jest czytelniejsze niż przerobienie architektury, niemniej jednak należy zwrócić uwagę na to, czy inni programiści zrozumieją, kiedy kod wchodzi do bloku if.

Poniższy przykład pokazuje świetnie ewolucje oprogramowania. Pierwsza linijka jest akceptowalna, bo to tylko jeden warunek, do tego w miarę czytelny i logiczny- ma pieniądze, więc może kupić. Po dodaniu funkcjonalności ról, umieszczamy kolejny warunek i już zaczyna się robić mało czytelnie. W tym momencie powinniśmy opakować cały warunek w jakąś ładnie brzmiącą metodę. Niestety wiele razy widziałem, że ostatni krok jest pomijany i zostaje taki tasiemiec z instrukcji warunkowych, nad którym trzeba spędzić kilka minut, żeby go zrozumieć.

// this is readable
if (user.AccountBalance >= productCost) { }

// we add Roles to program and code is ugly
if (user.AccountBalance >= productCost && user.Roles.Any(r => r.AllowInAppPurchases)) { }

// after refactor it is better
if (user.CanBuyProduct(productCost)) { }

To tylko kilka uwag, które zamierzałem przedstawić, żeby przestrzec innych. Często w pośpiechu sam zapominam o niektórych zasadach, ale mam nadzieję, że dzięki opisaniu ich tutaj, sam zacznę się bardziej pilnować. 🙂

4 Comments

  1. Ad. 4. A jak mam jedna klase DTO User i podczas edycji wykorzystuje ViewModel UserEdit a przy wyświelteniu listy ViewModel UserGridListItem ? Automapperem tworze ViewModele z obiektów klasy User.

    Odpowiedz

    1. Pytanie, czym jest dla Ciebie to DTO User, czy to klasa encyjna, czy coś pośredniego? Czy potrzebujesz tego UserDto do zmapowania encji na viewmodele? Jeśli pobierasz z bazy całego Usera, a później go mapujesz do prostej klasy posiadającej Id i Email, to jest to moim zdaniem nadmiarowość.

      Odpowiedz

  2. Jest to klasa encyjna. Całego usera nie pobieram, tylko np id nazwe i mail. Czyli stored procedure -> DapperORM -> zwrocony obiekt User z uzupelnionym tylko id, nazwa, mail -> Autommaper.Map() zwraca odpowiedni view model.

    Odpowiedz

    1. Jeśli jest to klasa encyjna to ok, inaczej nie pobierzesz danych z bazy danych, fajnie że używasz AutoMappera i mapujesz to na inny obiekt, a nie zwracasz całe UserDto do Viewmodelu. Problem by się zaczął jeśli dajmy na to klasę UserGridListItem użył w innym ViewModelu i tam, nie korzystałbyś już ze wszystkich właściwości tego obiektu a tylko z części. Na początku wydaje się to niewinne, ale z czasem potrafi zrobić niezły bałagan 😉

      Odpowiedz

Dodaj komentarz

Twój adres email nie zostanie opublikowany. Pola, których wypełnienie jest wymagane, są oznaczone symbolem *