Kilka przykładów czytelności kodu

Temat czytelności kodu jest bardzo dyskusyjny. Mam na myśli to, że jest tutaj wiele „zależy”. Funkcję, którą dziś opisuję, popełniłem kilka dni temu, nie myśląć specjalnie o tym, że będą ją tutaj omawiał.

Spójrz najpierw na oryginalny kod poniżej krytycznym okiem, a później zapraszam do mojej analizy „dlaczego tak, a nie inaczej”.

public void ProcessRequest(HttpContext context)
{
    var queryCriterion = new QueryCriterion
    {
        Operator = QueryOperator.MatchExact,
        Property = SamAccountNameADName,
        Value = _userName,
    };

    var userService = ServiceLocator.Resolve<IUserService>();
    var person = userService.GetUser(queryCriterion, new[] { ThumbnailPhotoADName });

    if (person == null)
    {
        SetResultNotFound(context);
        return;
    }

    var bytes = person[ThumbnailPhotoADName] as byte[];

    if (bytes == null || bytes.Length == 0)
    {
        SetResultNotFound(context);
        return;
    }

    context.Response.ContentType = ";image/jpeg";
    context.Response.AddHeader("Content-Length", bytes.Length.ToString(CultureInfo.InvariantCulture));
    context.Response.Cache.SetCacheability(HttpCacheability.Public);
    context.Response.Cache.SetExpires(DateTime.Now.AddDays(1));
    context.Response.Cache.SetMaxAge(TimeSpan.FromDays(1));
    context.Response.Cache.SetLastModified(DateTime.Now);
    context.Response.BinaryWrite(bytes);
    context.Response.Flush();
    context.Response.Close();
    context.Response.End();
}

Słowno-muzycznie: Jest to custom HttpHandler. Dostaje adres „/userimage/wojtek.kowalski”, gdzie „wojtek.kowalski” jest przypisywane do _userName. Pobiera obrazek (bajty) z zewnętrzenego serwisu. Ustawia odpowiednie nagłówki, dzięki czemu przeglądarka wyświetli to jako obrazek.

Wiele krótko żyjącyh zmiennych

Tworzę dużo zmiennych. Nawet gdy coś występuje tylko raz, to tworzę tą zmienną. Przykłady to queryCriterion oraz userService. Tak bym nie zrobił:

var person = ServiceLocator.Resolve<IUserService>().GetUser(new QueryCriterion
{
    Operator = QueryOperator.MatchExact,
    Property = SamAccountNameADName,
    Value = _userName,
}, new[] { ThumbnailPhotoADName });

Zrobiłem właśnie tak:

var queryCriterion = new QueryCriterion
{
    Operator = QueryOperator.MatchExact,
    Property = SamAccountNameADName,
    Value = _userName,
};

var userService = ServiceLocator.Resolve<IUserService>();
var person = userService.GetUser(queryCriterion, new[] { ThumbnailPhotoADName });

(Oprócz czytelności skłania mnie do tego również system kontroli wersji. Przeglądam stary, kod a nie tylko dodaje nowy. Historie gdzie w osobnych linijkach tworzymy obiekty a w osobnych z nich korzystamy zdecydowanie się lepiej przegląda.)

(Nie powoduje to dodatkowych cykli procesora – podstawy .NET)

Inicjalizacja tablic i obiektów

new[] { ThumbnailPhotoADName } dopuszczam w jednej linijce. Natomiast wszystko co ma 2 lub więcej elementów rozbijam na wiele linii. Dokładnie tak jak tworzenie queryCriterion. Ta sama zasada obowiązuje dla tworzenia tablic jak i dla object initializers.

Pełne, wszystko mówiące nazwy

Nie ma powodu aby robić skróty zamiast pełnych nazw zmiennych. queryCriterion pochodzi wprost od klasy QueryCriterion. Nie qC, qc, queryCrit, queryC, crit, criterion. Ostatni wymieniony najbardziej by tu jeszcze pasował, ale dla zachowania ogólnej zasady lepiej pozostać przy pełnych nazwach.

Kiedy stała, a kiedy nie

ThumbnailPhotoADName jest stałą:

private const string ThumbnailPhotoADName = "thumbnailphoto";

Jest to jakaś wiedza domenowa i tutaj robię stałą. Pewnie wystąpi też w innej klasie i wtedy się to jakoś uwspólni. Powodem nie jest to że występuje 2 razy w tym kawałku kodu. Gdyby wystąpiło raz też bym chyba zrobił stałą. Można to porównać do kodu niżej:

context.Response.ContentType = "image/jpeg";
context.Response.AddHeader("Content-Length", bytes.Length.ToString(CultureInfo.InvariantCulture));

Tutaj nic się nigdy nie zmieni, literówka się nie zdarzy. Kiedyś myślałem, że tego typu stringi są pierwsze do wyciągania do stałych, ale pragmatyzm i kod, który wtedy powstawał, mówi mi, że lepiej zostawić tu stringi. To tak bardzo subiektywnie.

ToString(CultureInfo.InvariantCulture)

Length.ToString(CultureInfo.InvariantCulture)

Przy zamianie liczb na stringi dodaję CultureInfo.InvariantCulture. Nie chcę mieć warningów z R# i Code Analysis. Dobrze się trzymać tych zasad. Dla typów całkowitoliczbowych nic się złego nie stanie, ale już dla zmiennoprzecinkowych czasem w innych kawałkach kodu zdarzą się małe bugi.

DateTime.Now.AddDays(1)
TimeSpan.FromDays(1)

Odpowiednie fabryki, przeładowania

Korzystać z najbardziej adekwatnych metod. Czyli jeśli kiedyś było TimeSpan.FromHours(1), a teraz przyszedł CR, aby zmienić na 24 godziny to zmieniam na TimeSpan.FromDays(1) a nie TimeSpan.FromHours(24).

Wszędzie używam var

Jedynym przypadkiem kiedy można by nie używać var jest:

Dictionary<string, object> person = userService.GetUser(queryCriterion, new[] { ThumbnailPhotoADName });

Tak się już jednak przyzwyczaiłem, że zupełnie się nad tym nie zastanawiam i używam wszędzie.

Jak wyszło?

Tak ja się staram pisać kod. Jestem bardzo ciekaw, czy nie nagiąłem jakiejś zasady do której czytelnicy się stosują na co dzień. Uwagi?

Reklamy
Ten wpis został opublikowany w kategorii Programowanie i oznaczony tagami , , , . Dodaj zakładkę do bezpośredniego odnośnika.

11 odpowiedzi na „Kilka przykładów czytelności kodu

  1. Pingback: dotnetomaniak.pl

  2. jedmac pisze:

    Co do var, jest ono bardzo problematyczne. Weźmy pod uwagę taki kod (opisany z resztą już gdzieś, kiedyś, przez kogoś, ale świetnie opisujący problem)

    var lemon = new Lemon() (Color)
    var orange = new Orange() (Fruit)

    Jeśli chodzi o bardziej życiowy przykład:

    var configuration = GetConfiguration() -> za nic w świecie nie widać na pierwszy rzut oka jakiego typu jest configuration 🙂

    W swoim kodzie korzystam z var tylko wtedy, gdy od razu wiemy jaki to typ, np.: var listOfCars = new List();

  3. @jedmac, Regułkę „var nie należy stosować gdy od razu nie widać o jaki typ chodzi” znam niby kilka lat. Szczerze to jakoś nie przytrafił mi się kod w którym by to zaszło.

    Np ten wspomniany przykład z ‚var configuration’. Dobry przykład żeby nie stosować var, choć z drugiej strony, czy to ważne jaki to typ? Jeśli configuration.IpAddress się kompiluje to wiemy że jest takie property. Na zasadzie „jeśli kwacze to kaczka”.

  4. hellhax pisze:

    właśnie moim zdaniem przypisanie:

    var person = userService.GetUser(queryCriterion, new[] { ThumbnailPhotoADName });

    jest tu haniebne i woła o pomste do nieba. Wynika ze zwykłego lenistwa. A wiadomo jak się w programowaniu lenistwo konczy. O ile jeszcze coś takiego przejdzie (bo jest całkiem czytelne):

    var queryCriterion = new QueryCriterion { …};
    var userService = ServiceLocator.Resolve();

    to przypisanie z person jest do bani. Człowiek się spodziewa ze dostanie insanicje klasy Person (ew User? w koncu get user?) a dostaje…słownik?…rly? wtf? person jest słownikiem? A wogóle dlaczego zmienna nazywa się person a nie user? Tutaj wszystko jest mylące.

    Moim zdaniem var jest najbardziej naduzywanym słowem kluczowym w C#. 90% programistów używa go tak jak Ty. Powstało w zupełnie innym celu (linq i typy anonimowe). IMO największą przyczyną tego jest resharper ktróry domyślnie sugeruje takie uzycie (podobnie jak sugeruje masę innych idiotycznych rzeczy ale to już temat na inną dyskuję). Najgorsze jest jak ludzie przypisują wyniki wywołania metod do zmiennych typu var.

    Poza tym odwołujesz się do elementu w słowniku bez sprawdzenia czy dany element w nim istnieje:
    var bytes = person[ThumbnailPhotoADName] as byte[];

    co w runtime powduje rzucenie wyjątku. To czy ThumbnailPhotoADName będzie w tm slowniku czy nie zależy od implementacji metody GetUser, która jest w innym miejscu i może zostać zmieniona (przez Ciebie później albo przez kogoś innego) albo jeszcze czegoś innego czego nie ma w tym kodzie (np bazy danych?)

    Oczywiście może się zdażyć, że jak tego nie ma w słowniku to system jest w niepoprawnym stanie i lepiej rzućić wyjątek. Ale lepiej samemu to sprawdzić i rzucić customowy wyjątek z własnym message niż pozwolić zeby poleciało zwykłe key not found exception. Własnym message łatwiej namierzyć w logach i w kodzie.

  5. Adrian pisze:

    Jeśli mówimy o czytelności to ja bym przede wszystkim podzielił tę metodę na mniejsze o odpowiednich nazwach, tak, żeby następna osoba, która będzie patrzeć na kod nie musiała się zastanawiać co tu właściwie się dzieje.
    Jeśli chodzi o new [] { ThumbnailPhotoADName } to ja bym właśnie przypisał to do zmiennej, bo w zasadzie nie wiadomo do czego to służy. Domyślam się, że jest to lista pól, które mają zostać zwrócone, ale nie widać tego na pierwszy rzut oka.
    Ja osobiście staram się var używać jak najrzadziej, nawet w przypadkach kiedy typ jest widoczny po prawej stronie przypisania, ale to już kwestia preferencji.
    Poniżej przykład „refactoringu”:

    public void ProcessRequest(HttpContext context)
    {
    byte[] thumbnail = RetrieveThumbnail();

    if(thumbnail != null && thumbnail.Length >= 0)
    {
        PrepareHttpResponse(thumbnail, context);
    }
    else
    {  
        SetResultNotFound(context);
    }
    

    }

    private void RetrieveThumbnail()
    {
    IUserService userService = ServiceLocator.Resolve();
    string[] propertiesToRetrieve = new[] { ThumbnailPhotoADName };
    Dictionary<string, object> user = userService.GetUser(GetQueryForUserByName(), propertiesToRetrieve);

    if(user == null || !user.Contains(ThumbnailPhotoADName)
    {
        return null;
    }
    
    return user[ThumbnailPhotoADName] as byte[];
    

    }

    private void GetQueryForUserByName()
    {
    return new QueryCriterion
    {
    Operator = QueryOperator.MatchExact,
    Property = SamAccountNameADName,
    Value = _userName,
    };
    }

    private void PrepareHttpResponse(byte[] thumbnail, HttpContext, context)
    {
    context.Response.ContentType = „image/jpeg”;
    context.Response.AddHeader(„Content-Length”, thumbnail.Length.ToString(CultureInfo.InvariantCulture));
    context.Response.Cache.SetCacheability(HttpCacheability.Public);
    context.Response.Cache.SetExpires(DateTime.Now.AddDays(1));
    context.Response.Cache.SetMaxAge(TimeSpan.FromDays(1));
    context.Response.Cache.SetLastModified(DateTime.Now);
    context.Response.BinaryWrite(thumbnail);
    context.Response.Flush();
    context.Response.Close();
    context.Response.End();
    }

  6. @hellhax, @Adrian dzięki za uwagi, wszystko się zgadza. Zapomniałem sprawdzić czy klucz istnieje (chociaż wiem, że instnieje, bo sprawdzam zapewniam to w metodzie GetUser(), jednak to wiedza która tylko przeszkadza, sprawdzenie powinno być). Nazwa person wywalona. Kod rozbiłem na metody takie jak w refactoringu.

    Ciągle nie wiem jak się przekonać do nie nadużywania ‚var’. Widocznie muszę więcej kodu cudzego przeglądać, który też tego nadużywa. Może wtedy się wreszcie przejadę
    .
    Może w następnym projekcie wyłączę podpowiadanie ‚var’ w opcjach R#. Jeśli winić R# za nadużywanie ‚var’ to może to jest metoda.

  7. Łukasz K. pisze:

    A ja nie mam nic przeciwko korzystania z var. Jeśli chcę wiedzieć czym jest var, to po prostu najadę myszką na metodę lub nazwę zmiennej i środowisko mi podpowie. Ewentualnie ‚pójdę’ do definicji i wtedy wszystko staje się jasne jak słońce.
    To prawda, że var zostało stworzone do typów anonimowych, co nie zmienia faktu, że korzystanie z niego znacznie polepsza czytelnośc kodu i skraca zapis.
    Myślę, że doświadczeni programiści nie mają problemu z var.

  8. dream pisze:

    Jeśli chodzi o var, to dokładnie tak jak mówią @jedmac, @hellax, @Adrian: nie powinno się go nadużywać. Słówko var używamy tam gdzie wartość typu jest dla nas oczywista patrząc na sam kod. I tak robimy z innymi rzeczami w kodzie: kod czytamy i wszystkie rzeczy mają być oczywiste – kiedy czytelnik musi wchodzić gdzieś tam dalej, domyślać się, analizować kod, to coś jest nie tak. Nie pozostawiamy miejsca na domysły, kod ma być jednoznaczny, redukujemy niepotrzebny czas poświęcony na dodatkową analizę.

    Reguła: Jeśli linia zawiera typ, to możemy skrócić jego redundancję słówkiem var. Dzięki temu
    – Jestem w stanie spoglądając na kod dużo o nim powiedzieć, od razu.
    – Jestem w statnie od razu zrobić na typie „Go To Definition”, z var muszę wejść gdzieś tam przez coś tam

    Po to C# jest statycznie typowany, żeby z tych typów korzystać. W JavaScripcie jest var, i przez to używa się czasem np. notacji węgierskiej do ogarniania typów, var iNumer = 0, var sName = „cos”. Nie bez powodu ta linika się nie kompiluje w C#:
    var number = 0;

    Dobrze:
    var configuration = new Configuration();
    var configuration = Service.Get(); // Jak zwraca Configuration
    var configuration = argument as Configuration;

    Źle:
    var configuration = Service.GetConfiguration(); // zwraca IConfiguration, Configuration, a może Conf, a może TempConf2, a może Dictionary<Parameter, Tuple<string, int>>?

    Raczej źle:
    Configuration configuration = new Configuration(); // typ dwa razy, można skrócić, redundancja
    Configuration configuration = argument as Configuration;

    • dream pisze:

      // UPDATE2
      Nie bez powodu ta linika się nie kompiluje w C#:
      *var number;

  9. dream pisze:

    eh, bo wycina nawiasy ostre UPDATE:

    Dobrze:
    ….
    var configuration = Service.Get<. Configuration .>(); // Jak zwraca Configuration //UPDATE: DOBRZE
    ….

  10. marek pisze:

    odnosnie tej dyskusji dotyczacej var – to nie zostalo stworzone po to by pisac 3 literki zamiast pelnej nazwy wiec var configuration = new Configuration(); tez jest raczej zle. Var ma sens przy selectach – gdy nie chcemy tworzyc nowej klasy (bo i po co jesli zostanie uzyta tylko w jednym miejscu).

    Odnosnie najazdow robionych myszka 🙂 – ja np jestem na to zbyt leniwy – nie chce tego robic – chce znac od razu typ bez jakiegokolwiek myslenia.

    Zreszta var mozna uzywac – byle na koncu uruchomic jakis addon do vs ktory pozmienia var na konkretne klasy :).

Możliwość komentowania jest wyłączona.