[Code review] Ah ten tekst…

Kontynuuję omawianie projektu Pana kmph, który pojawił się już w poprzednim wpisie z serii „Code review”. Tam też znajdziecie link do repozytorium.

Dzisiaj krótko bo szykuję się do poruszenia tematu związanego z zastosowaniem wzorców i SOLIDa. W dodatku tekst powinien być w niedzielę, a mamy wtorek, także trzeba się śpieszyć :)

Co tym razem?

W tym wpisie zajmę się tematem nazw i tekstów w kodzie. Ponieważ większość pracy programisty to czytanie cudzego kodu dlatego dobrze, żeby nasz kod dobrze się czytało innym.

Pierwszą sprawą jest konwencja stosowana przy nazwach wszelakich. Drugi temat dotyczy tekstów znajdujących się w kodzie.

Nazwa własna

Większość, jeśli nie wszystkie powszechnie stosowane języki programowania posiadają coś takiego jak ogólnie przyjęta konwencja zapisu elementów wszelakich. Od stawiania nawiasów czy spacji, aż po styl wpisywania nazw metod czy zmiennych. I tym ostatnim się dzisiaj pokrótce zajmę. Ponieważ język C# również posiada coś takiego jak konwencję nazewnictwa, przygotowaną przez Microsoft.

Jedną z zasad tam podanych jest rozpoczynanie nazw publicznych pól, właściwości czy metod z dużej litery. Jest to powszechnie stosowany zapis i każdy programista C# to zauważa. Również wszystkie biblioteki czy frameworki pisane są w zgodzie z tą regułą. Jednak autor kodu nie zawsze się do tego stosuje w przypadku metod. A w przypadku właściwości w większości przypadków używa małej litery.

Przykładowo w tym fragmencie kodu:

Mamy metody, które pochodzą z samego .NETa czyli GetHashCode()Equals(). I one są pisane dużą literą. Zaś poniżej metoda autora iterateEffects() zaczyna się z małej litery i wręcz ciężko mi było odnaleźć gdzie jest dokładnie jej nazwa.

To samo dotyczy publicznych pól na początku tej klasy.

Z resztą korzystając z narzędzia Resharper od razu widać gdzie te nazwy nie trzymają się konwencji bo dostajemy podkreślenie i po najechaniu stosowny komunikat:

Jak wspominałem w poprzednim wpisie – spójność jest ważna. Można oczywiście powiedzieć, że przyjęło się inną konwencję nazewniczą. Jednak w takim wypadku należałoby się do niej stosować w całym naszym kodzie i odpowiednio skonfigurować narzędzia do analizy tak, żeby wiedziały co robimy. Tylko zmieniając tę konwencję tracimy spójność z wbudowanymi elementami frameworka i powstają mieszanki jak ta na powyższym przykładzie.

Jeśli ktoś się zastanawia jaki jest polecany sposób zapisu różnych fragmentów naszej aplikacji to może się zapoznać z całym przewodnikiem dotyczącym wszystkich konwencji proponowanych przez Microsoft. Praktycznie każdy twórca zewnętrznych bibliotek i narzędzi się do nich stosuje dzięki temu czytany kod zawsze wygląda w podobny, znajomy sposób.

Tekst tu, tekst tam

W kodzie znajduje się dużo tekstu. Głównie opisów i wiadomości z parametrami.

W przypadku czystych opisów jedyna rada jaką mogę dać to zastanowienie się nad wyciągnięciem ich do jakichś zewnętrznych zasobów. Zwłaszcza jeśli chciałoby się w przyszłości mieć aplikację wielojęzyczną. Jednak jest to bardziej luźna sugestia. Same opisy są chyba we wszystkich miejscach przypisane do jakichś zmiennych więc nie ma walających się w kodzie magic-stringów.

Ale w przypadku tekstów z parametrami wyglądają one tak:

O ile sama konkatenacja stringów może być ok to jednak łączenie ich w ten sposób z parametrami wprowadza nieporządek. Zwłaszcza kiedy parametry są co najmniej dwa. Wtedy linijka robi się oczywiście długa i można przeoczyć jakiś parametr. W takim wypadku wg mnie czytelniejszym rozwiązaniem jest zastosowanie metody String.Format() i zapisanie tego w taki sposób:

Nie mówiąc o tym, że łączenie tekstu w wywołaniu metody to niezbyt korzystny pomysł. Taki zapis zmniejsza czytelność parametrów przekazanych do funkcji i utrudnia debugowanie. Na pierwszy rzut oka nie widziałem, że tam jest drugi parametr gdzieś na końcu. Lepiej by było tekst przypisać do pomocniczej, dobrze nazwanej zmiennej i dopiero ją przekazać do funkcji.

Jeśli chodzi o formatowanie tekstów to C# w wersji 6 wprowadził dodatkowy operator string interpolation, którego używa się podobnie jak String.Format().

Dodatkowo łącząc wartości z tekstem metoda ToString() jest wywoływana automatycznie dlatego nie ma potrzeby jej jawnego używania, co ponownie pokazuje m.in. Resharper:

Dodatkowo zastanawia mnie stosowanie w pewnych miejscach tablic stringów:

Próbowałem znaleźć gdzie mają one sens jednak wszędzie widzę przekazywanie jedynie tablicy jednoelementowej – czyli po prostu pojedynczego tekstu. Więc tutaj pytanie do autora, co za tym stało?

Zakończenie

Tak jak wspomniałem na wstępie dzisiaj tekst krótszy. Ale mimo to mam nadzieję, że wartościowy.

Jeżeli chcecie być na bieżąco z tekstami i newsami, nie tylko dotyczącymi wpisów z serii „Code review” ale ogólnie bloga to zapraszam na fanpage na Facebooku.

Zaś chętnych do podesłania mi kodu do oceny odsyłam do wpisu, w którym podaję więcej informacji na ten temat.

3 thoughts on “[Code review] Ah ten tekst…”

  1. Co do tablic stringów: Jednak nie wszędzie tablice te są jednoelementowe. Chodzi o to, że przekazywany do klienta BattleUpdate ma: samą komendę i parametry. DisplayNotice ma 1 do 2 parametrów, pierwszy parametr to sama notka, drugi parametr to opcjonalna klasa tej notki (klasa w sensie CSS – chodzi o to, że jeśli wynosi ona „minor”, to znaczy, że kod JSscriptowy do notki dołoży CSSową klasę „minor”, która wyświetli ją mniejszą czcionką).

    W Species i Field otoczyłem to lukrem synktatycznym, np. podany przez Ciebie przykład

    woła Species.AddNoticeUpdate, której definicja jest:

    Tymczasem Species.AddUpdate ma definicję:

    Czyli innymi słowy, ta linijka sprowadza się do czegoś takiego, gdzie użycie tablicy stringów jest już jawne:

    No nie mogłem wykorzystać tych funkcyjek w Moves. Bo addNoticeUpdate to nie jest IAction, tylko metoda na Species. A więc zamiast tego dałem IAction ClientCommand, której metoda execute też sprowadza się do wstawienia odpowiedniej BattleUpdate. Ażeby zachować ogólność, konstruktor ClientCommand przyjmuje wszystko: zarówno komendę, jak i listę parametrów.  No ale w praktyce rzeczywiście, 90% jak nie wszystkie takie komendy w Moves to albo: „Sleep”, która jest jednoparametrowa, albo teoretycznie dwuparametrowa „DisplayNotice”, gdzie jednak „major” jest opuszczana jako wartość domyślna.

    Teraz myślę, że to faktycznie bez sensu. ClientCommand jest takim Moves-owym odpowiednikiem Species.addUpdate. Tymczasem powinny być jeszcze: ClientNoticeCommand, ClientSleepCommand, tak samo jak Species ma addNoticeUpdate i addSleepUpdate, które abstrahują składnię tych komend i nie wymagają przekazania im tablicy stringów.

    Szczerze mówiąc dziwię się, że nie zostałem skrytykowany za używanie w tych stringach słów takich jak „monname” i „playername” :) Myślałem, że to zostanie uznane za spaghetti. Są to przecież takie niejawne parametry: tj. dopiero klient zamienia je na nazwę gracza lub mona:

    Miałem spore zawachanie tutaj, bo rozwiązanie takie wydało mi się brzydkie. Jednak z drugiej strony wydało mi się, że jest to najprostsze rozwązanie – choć oczywiście kod C# mógłby te stringi już zbudować całkowicie po stronie serwera, to jednak wymagało by to większej ilości kodu, niż powyższe trzy Javascriptowe linijki. I chyba rozsyłania aż trzech wersji tego stringu: osobnej wersji dla gracza, osobnej dla jego przeciwnika, osobnej dla obserwatorów.

    Na koniec jeszcze raz dziękuję za kolejną porcję wartościowych uwag :) Zamierzam poprawić kwestię nazewnictwa, tak samo zresztą jak i powydzielać klasy do osobnych plików, tylko odkładam to ciągle na później, bo to żmudna robota. Dziękuję też za zwrócenie mi uwagi na String.Format – zdecydowanie jest to czytelniejsze.

    1. Dlatego przy tych tablicach zapytałem co się tam podziało bo ciężko mi było prześledzić całość. Dzięki za wyjaśnienie :)
      Na to podmienianie stringów jeszcze nie trafiłem ;) Projekt jest duży dlatego na pewno dużo rzeczy pominąłem.
      Następnym razem postaram się coś podpowiedzieć jeszcze w sprawach architektury żeby nie skupiać się tylko na takich drobnostkach.

  2. Najgorzej jeśli dostanie się kogoś skrypt a w nim nie ma nawet podstawowych wcięć.. Niektórzy naprawdę bardzo przesadzają. Bardzo dobry wpis, wielu powinno go zobaczyć.

Dodaj komentarz

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