Kilka pomysłów na kod, który łatwiej zrozumieć

Ostatnio dość sporo siedziałem w starym, odziedziczonym kodzie – nie tylko cudzym, ale i swoim. Sporo czasu zeszło mi na próby zrozumienia co autor miał na myśli… Na szczęście mogłem sobie pozwolić na wykonanie odpowiedniej refaktoryzacji z marszu – ok, część tego czego się domyśliłem umieściłem tylko w komentarzach ale tam gdzie się dało usprawnić kod od ręki – tam to zrobiłem.

Przy tej okazji chciałbym się podzielić kilkoma koncepcjami: zarówno starymi i dobrze znanymi jak i kilkoma, na które wpadłem – niekoniecznie ostatnio – zastanawiając się jak można pewne rzeczy sformułować lepiej… Ogólny zamysł przyświecającym tym ideom jest jeden – łatwiej zrozumieć co się dzieje w kodzie, także przy ewentualnym braku wsparcia ze strony VS (np. podczas łączenia gałęzi w repozytorium kodu…) gdy kod jest samo-dokumentujący. Że banał? Jasne. Tym niemniej zapraszam do lektury.

Magiczne liczby

Wszyscy niby wiemy, że magiczne numerki są złe. Bardzo złe… Dość oczywiste przykłady:

string libName = columnData.Substring(20, 13);
Item itm = collection.GetElementAt(indexRequested - 1);
// ...

Takie rzeczy zwykliśmy załatwiać za pomocą stałych, odpowiednich komentarzy, obiektów z konfiguracją itp.

Ale wszystkie one kręcą się wokół jednej rzeczy – co oznacza dany argument – skąd jego wartość, co ona oznacza dla aplikacji. Czyli np. w drugim przykładzie “1” oznacza (zazwyczaj) “korektę między systemem numeracji kolekcji, z którego korzysta algorytm dostarczający wartość zmiennej “indexRequested” do systemu numeracji wykorzystywanego przez samą kolekcję “collection“. W pierwszym przykładzie mamy jednak już dwa problemy jednocześnie – nie tylko co oznaczają podane argumenty ale jaką rolę pełnią odpowiednie parametry w wywołanej funkcji? (Ok, Susbstring() jest trywialnym przykładem – wszyscy ją znamy, ale chodzi mi o zasadę :P).

Czasami rozwiązując pierwszy problem, możemy jednocześnie rozwiązać i drugi. Np. w ten sposób:

string libName = columnData.Substring(libColumnDefinition.StartIndex, libColumnDefinition.Width);

Jednocześnie pozbywamy się magicznych liczb i umieszczamy je w konfiguracji (nawet jeśli nadal jest ona zakodowana na sztywno, to będąc trzymaną w jednym miejscu niesie o wiele więcej spójnej informacji) ale jednocześnie wskazujemy na znaczenie poszczególnych wartości dla Funkcji Substring().

Są jednak przypadki, gdy usunięcie magicznej liczby nie jest za bardzo możliwe. Np.

Timespan serviceTimeout = configuration.GetServiceTimeout(this.ServiceName) ?? new Timespan(0, 10, 0);

Oczywiście można by umieścić samą wartość Timespan w “stałej” by nadać jej znaczenie (“domyślny czas wygasania usługi”), ale tam dala widniałyby cyferki. A gdyby tak dokonać małego – być może – nadużycia składni?

private static readonly DEFAULT_SERVICE_TIMEOUT = new Timespan(hours: 0, minutes: 10, seconds: 0);

//...

Timespan serviceTimeout = configuration.GetServiceTimeout(this.ServiceName) ?? DEFAULT_SERVICE_TIMEOUT;

Że trywialne? Zgadzam się. Ale jeśli w jednym miejscu konfigurujemy argumenty dla wielu rozmaitych parametrów a do tego niektóre zawierają dodatkowo dni – to nawet tak nieznaczna wskazówka może zaoszczędzić cenne sekundy podczas dalszej pracy z kodem. A wstawienie nazw parametrów nie zajmuje dużo czasu – zwłaszcza, że znajdują się na liście IntelliSense (choć chyba dodaje je tam dopiero R#).

Jeszcze lepiej, gdy pracujemy z biblioteką, która nie ma zamrożonego API – jeśli znaczenie / kolejność parametrów się cichcem zmieni (oj bywało… bolesne…) – nasz kod nadal działa prawidłowo! Hmmm… Tak sobie myślę, że dodatkowo będzie to zabezpieczenie przed zmianą nazw parametrów – a więc ze sporym prawdopodobieństwem także ich znaczenia – dostaniemy błędy podczas kompilacji. Inna sprawa, że jeśli zmiany nazwy dokonamy za pomocą R# to i tak zmieni od razu we wszystkich miejscach. Jednak, jeżeli biblioteka nie jest częścią naszej solucji – będzie dobrze.

Magiczne stringi (nie majty czarodziejki)

Można się kłócić czy wszystkie stosowane napisy powinny się znaleźć w zasobach. Osobiście np. komunikaty wyjątków trzymałbym wyłącznie w postaci łańcuchów verbatim i wyłącznie po angielsku. Jak (nie?) wspomniałem w poprzednim artykule – wyjątki są tylko dla programistów. Użytkownik powinien otrzymać stosownie opracowane komunikaty błędów – ale tylko w określonych przypadkach. Dlatego tłumaczenie wyjątków uważam za zbyteczne. Sam .Net Framework niestety lokalizuje komunikaty wyjątków, przez co czasami dużo trudniej wygooglać o co mu chodzi… Zapewne nigdy nie będzie to spójne – zależnie od bibliotek, których używamy, możemy oczekiwać komunikatów w wielu językach – oczywistym w związku z tym jest, że treść komunikatów nie powinna być przedmiotem analizy w bloku catch… Dalej – tych łańcuchów tekstowych nie umieszczałbym także ani w stałych ani w jakichś klasach magazynowych ale zwyczajnie w miejscu wywołania wyjątku, tak by ewentualne odnalezienie treści komunikatu podczas analizy kodu nie wymagało dodatkowego poszukiwania i klikania – zwłaszcza przy porównywaniu wersji kodu.

Być może warto umieścić komunikaty, które idą do logów w lokalizowanych zasobach. Szczerze mówiąc w tym przypadku jestem zarówno za tym by logi były również zawsze spójnie i tylko po angielsku – bo to ewentualnie znacznie uprości pracę helpdesku czy supportu, ale również w przypadku niektórych aplikacji poddałbym je lokalizacji, zwłaszcza jeśli dla użytkownika końcowego będą czymś więcej niż narzędziem wyłącznie diagnostycznym. Tu trzeba by się zdać na zdrowy rozsądek i podejść do każdej sytuacji indywidualnie – aczkolwiek w przypadku ogólnym chyba bardziej bym się skłaniał ku opcji pierwszej.

Generalnie wszelkie napisy prezentowane użytkownikowi należałoby zebrać w zasobach. Jasne, że w niektórych przypadkach może to nie być trywialne – np. gdy dane kombo napełniamy kolejnymi wartościami z enuma (choć i na to są sprawdzone sposoby – atrybuty + refleksja). Oczywiście, gdy tworzymy małą aplikację, której absolutnie nie planujemy lokalizować, to zapewne można by to zignorować. Ale z drugiej strony może to nie tylko wyrobić dobre nawyki, ale także sprawić, że opracujemy i przećwiczymy odpowiednie standardy i kod wspomagający takie podejście, które będzie można z łatwością wykorzystać w dużym projekcie.

Kolejna sprawa jest już bardziej skomplikowana – mianowicie mam na myśli łańcuchy znaków stosowane w atrybutach (zresztą – liczb to też może dotyczyć). Koniecznym wymogiem stawianym przez składnię jest to, by atrybuty były serializowalne w momencie kompilacji. W związku z czym odpadają wszelkie dynamiczne podejścia typu zasoby, dostawcy czy fabryki. Możemy tylko stosować różnego rodzaju stałe:

[XmlElement(@"userName")]
public string Name { get; set; }
public partial class User
{
    public static readonly string ATTRIBUTE_USER_NAME = @"user_name";
    // ...

    [XmlElement(ATTRIBUTE_USER_NAME)]
    public string Name { get; set; }
    // ...
}
internal static class UserAttributes
{
    public const string USER_NAME = @"user-name";
    // ...
}

public partial class User
{
    [XmlElement(UserAttributes.USER_NAME)]
    public string Name { get; set; }
    // ...
}

Schody zaczynają się, gdy z tak przygotowanej klasy piórkowej chcemy np. skorzystać w zapytaniach xQuery. Oczywiście – można napisać specjalizowane metody dla każdej encji, klasy filtrów itd – ale to ogromna ilość kodu. Spróbujmy z czymś prostszym. Wyobraźmy sobie, że mamy przygotowane uniwersalne repozytorium do tego rodzaju czynności, które przyjmuje jako parametry ścieżkę xPath do elementu / atrybutu w encji XML oraz wartość, której ma szukać:

    public enum XPathOperator
    {
        Eq,
        NEq,
        Gt,
        GtE,
        Lt,
        LtE
        // ...
    }

    public struct Filter
    {
        public Filter(string xPath, XPathOperator filterOperator, string value)
        {
            XPath = xPath;
            Operator = filterOperator;
            Value = value;
            Values = new string[0];
        }

        public Filter(string xPath, XPathOperator filterOperator, params string[] values)
        {
            XPath = xPath;
            Operator = filterOperator;
            Value = null;
            Values = values;
        }

        public string XPath;
        public XPathOperator Operator;
        public string Value;
        public string[] Values;
    }

    public interface ICrudRepository<TEntity>
    {
        IEnumerable<TEntity> FindEntities(params Filter[] filters);
        // ...
    }

Przykładowe wywołanie mogłoby wyglądać np. tak:

ICrudRepository<User> repo = GetRepoFromSomewhere<User>();
var users = repo.FindEntities(
    new Filter("user-name", XPathOperator.Eq, "karol"),
    new Filter("user-age", XPathOperator.GtE, "25"));

Tragedii nie ma. Ale co jeśli zmienią nam się nazwy elementów? W życiu nie ogarniemy wszystkich łańcuchów znaków, które trzeba zmienić tak by żadnego nie przegapić – zwłaszcza w dużym projekcie. Jeśli skorzystamy z wcześniejszych mapowań atrybutów – to jesteśmy dość dobrze kryci:

ICrudRepository<User> repo = GetRepoFromSomewhere<User>();
users = repo.FindEntities(
    new Filter(UserAttributes.USER_NAME, XPathOperator.Eq, "karol"),
    new Filter(UserAttributes.USER_AGE, XPathOperator.GtE, "25"));

D.R.Y.! Czy można inaczej? Czasem nawet trzeba – np. gdy nie możemy zmodyfikować modelu danych, nie jest on nawet opatrzony stosownymi atrybutami a potrzebujemy jednak zdobyć bieżącą nazwę właściwości. Albo atrybutu. Pierwszym co przychodzi do głowy jest refleksja – ale to czasochłonna operacja, w warstwie dostępu do danych nie powinna mieć miejsca.
Jest co prawda inny sposób – często wykorzystywany na potrzeby implementacji interfejsów typu IPropertyChangeNotifier. Związany jest z używaniem wyrażeń lambda, jednak w naszym przypadku bezużyteczny gdyż wymaga instancji obiektu do działania… A jakby do tego jeszcze dodać budowanie ścieżek XPath dla hierarchii zagnieżdżonych obiektów lub co gorsza kolekcji… No ok, teoretycznie można by przetworzyć gdzieś dane z refleksji (raz a dobrze) i się potem do nich w cwany sposób odwoływać – ale tu już za bardzo odbiegam od tematu 🙂

Boolean też może być zły

Spójrzmy na trywialny przykład:

using (var stream = new StreamWriter(filePath, true))
{
    // ...
}

Kto pamięta co robi ten strumień gdy w konstruktorze jako drugi argument przekażemy ‘true’? Zapewne każdy z nas – bo ta klasa jest często używana… No ale czy nie można by od razu dać:

using(var stream = new StreamWriter(filePath, append: true))
{
    // ...
}

Wówczas nie ma potrzeby nawet chwili marnować na przypominanie sobie parametrów konkretnej funkcji! Nawet tak prostej.

Ale to jest prosty przykład: flaga jest tylko jedna, nie są przekazywane kolejne argumenty (co wymusiłoby podawanie nazwy parametru dla wszystkich kolejnych argumentów – to już mogłoby sytuację bardziej zaciemnić niż rozjaśnić…). Zobaczmy kilka trudniejszych przykładów oraz inny proponowany przeze mnie pomysł:

IEnumerable<string> documentPaths = GetDocuments(documentsRoot, "*.pdf", true, false, true);

Konia z rzędem temu kto powie co robi ta funkcja! Kto – po tym jak ją sprawdzi w dokumentacji lub podejrzy w kodzie – będzie nadal pamiętał po tygodniu? Raczej nikt. Zgodnie z wcześniejszym pomysłem można by próbować wprowadzić nazwy parametrów:

IEnumerable<string> documentPaths = GetDocuments(documentsRoot, "*.pdf",
    recursive: true, readOnly: false, modified: true);

Ale jednak trochę mało to poręczne i elastyczne… Można jednak zaprząc wyspecjalizowane typy enum, na dwa sposoby:

IEnumerable<string> documentPaths = GetDocuments(documentsRoot, "*.pdf", 
    GetDocumentOptions.Recursive | GetDocumentOptions.Modified);
IEnumerable<string> protectedDocs = GetDocuments(documentsRoot, "*.pdf", 
    GetDocumentOptions.ReadOnly);
IEnumerable<string> documentPaths = GetDocuments(documentsRoot, "*.pdf", 
    ScanFolders.Recursive, DocumentFlags.ReadOnly, DocumentState.Modified);

Oba podejścia mają swoje wady i zalety – ale jedną wspólną – łatwość rozszerzania implementacji. W sytuacji gdy jednak dojdziemy do wniosku, że potrzebujemy więcej możliwych wartości na parametrze – może się okazać, że zmiana boola na coś innego będzie bolesna. Jednak jeśli od początku był tam enum, to nie ma wielkiego problemu.

Na koniec jeszcze jedna kwestia. Wydawałoby się wielokrotnie wspominana w kontekście samo-dokumentującego się kodu. Przykład:

if ((action & Actions.NoPageView) != Actions.NoPageView)
{
	if (this.Session != null && (action & Actions.NewPageView) != Actions.NewPageView)
	{
		// ...
	}
	else
	{
		// ...
	}
				
	bool newPageView = (action & Actions.NewPageView) == Actions.NewPageView || 
        ((action & Actions.TrackPageView) == Actions.TrackPageView && this.Pageview == null);
	if (newPageView)
	{
        // ...
    }
}
else
{
    // ...
}

Autentyczny kawałek z legacy kodu. Warunków wbrew pozorom nie ma tak wiele ani nie są specjalnie skomplikowane. Główne zamieszanie wprowadza analiza opcji przekazanych za pomocą flag. Gdyby jednak odrobinkę bardziej to rozpisać – zrozumienie intencji kodu staje się o wiele prostsze:

bool allowPageViewsProcessing = (action & Actions.NoPageView) != Actions.NoPageView;
bool shallTrackNewPageView = (action & Actions.NewPageView) == Actions.NewPageView;
bool shallTrackPageViews = (action & Actions.TrackPageView) == Actions.TrackPageView;
bool pageViewAlreadyExists = this.Pageview != null;
bool trackingSessionAlreadyExists = this.Session != null;

if (allowPageViewsProcessing )
{
    if (trackingSessionAlreadyExists  && !shallTrackNewPageView)
    {
        // ...
    }
    else
    {
        // ...
    }

    bool doTrackNewPageView = shallTrackNewPageView || (shallTrackPageViews && !pageViewAlreadyExists);
    if (doTrackNewPageView)
    {
        // ...
    }
}
else
{
    // ...
}

O ile prościej się to czyta! I o ile łatwiej uniknąć błędów 😀

***

Jak zwykle chętnie zapoznam się z opiniami innych programistów – zarówno dotyczących bieżącego tematu, rozwinięcia innych rzeczy czy ogólnego poziomu bloga – może np. nie powinienem powtarzać i przypominać Czytelnikom podstaw tylko od razu skupić się na bardziej zaawansowanych tematach? Osobiście wydaje mi się, że to tło może być interesujące, ale zapewne mogę się mylić 😛

One thought on “Kilka pomysłów na kod, który łatwiej zrozumieć”

Comments are closed.