[Code review] Obozy kodu

Kontynuujemy przygodę z publicznym code review. Tym razem do sprawdzenia dostałem kod gry webowej od osoby, która chciała być podpisana jako kmph. Bardzo dziękuję autorowi za podzielenie się swoim projektem. Jest on o tyle ciekawy z punktu widzenia tej serii, że na pierwszy rzut oka wygląda jak przyzwoity kod, jednak zagłębiając się w szczegóły można dostrzec coraz więcej problemów z „czystością”. Dodatkowo można w nim dyskutować nad problemami na poziomie architektury. Z tego powodu zastanawiam się czy ten projekt nie pojawi się tutaj więcej niż raz.

Repozytorium dostępne jest pod adresem https://kmph.visualstudio.com/_git/mon?version=GBdev. Ja oceniam gałąź dev jako, że zawiera aktualny kod.

Kod pokazywany w tym wpisie należy do jego autora. Pokazuję go tutaj dzięki zgłoszeniu się autora drogą mailową. Jeżeli chcesz aby Twój kod został oceniony zapoznaj się z TYM postem.

Co by tu ocenić?

Jak napisałem na wstępie, projekt ten można analizować na dwóch płaszczyznach – pojedynczych plików i architektury. Z racji, że jest on rozbudowany to w tym wpisie pokażę po jednym przykładzie wszystkiego.

Długo. Za długo.

Pierwsza rzecz jaką można dosyć łatwo znaleźć to obecność kilku plików, które zawierają mnóstwo kodu. Zresztą od tych plików powstał tytuł tego wpisu. Są to swego rodzaju obozy kodu, w których kod aplikacji się zgromadził i siedzi. Przykładowo plik z akcjami zawiera ponad 900 linii kodu.

Jednak nawet nie jest to największym problemem. Większy problem z tymi plikami jest taki, że zawierają one mnóstwo klas. Także w tym miejscu sugestia do autora aby nie bał się większej ilości plików. Wbrew pozorom jest to łatwiejsze w czytaniu.

Najprostszy przykład. Chciałem sprawdzić co zawiera jakaś klasa, której obiekt spotkałem. Odruchowo zacząłem szukać pliku z nazwą klasy. Jednak nie znalazłem takiego. Musiałem przebijać się przez jeden duży plik. W dodatku zgadując najpierw o jaki plik chodzi. Wiem, że w Visual Studio mam funkcję „Go to definition…„. Jednak patrząc bardziej ogólnie taka funkcja niekoniecznie musi być dostępna.

Dlatego tutaj porada jest taka, aby dać trochę przestrzeni tym wszystkim klasom. Nawet jeśli zajmują one kilkanaście linijek to niech dostaną swoje pliki. Pozwoli to lepiej uporządkować kod i ekstremalnie ułatwi przeglądanie go. W tym momencie ciężko stwierdzić gdzie kończy się jedna klasa, a zaczyna druga. Jeżeli autor korzysta z Resharpera to jest tam funkcja „Refactor this -> Move to another file„. Dzięki temu można wydzielić klasy do osobnych plików za pomocą dwóch kliknięć.

Dodatkowy plus takiego podejścia do uniknięcie problemu z nazwami plików – będą się one nazywać tak jak klasy. W tym momencie plik OtherParticularEffects.cs nie mówi za wiele.

Przywrócić nawiasy

Jeżeli chodzi o kod to pierwszą rzeczą na jaką zwrócę uwagę jest brak nawiasów przy niektórych instrukcjach warunkowych.

O ile sam C# jak najbardziej obsługuje takie konstrukcje to jednak nie wpływają one pozytywnie na czytelność i utrzymywalność kodu. Przy pierwszym rzucie oka przeszło mi przez głowę, że ten if nie zawiera żadnego kodu, który się wykona w przypadku spełnienia warunku.

Jednak większy problem się robi kiedy w takim kodzie będziemy cokolwiek zmieniać. Stosowanie zapisu bez nawiasów klamrowych jest chyba jedną z częstszych przyczyn spędzania godziny na debugowaniu dlaczego nowo dodany kod nie wykonuje się w ogóle albo wykonuje się zawsze mimo, że nie powinien.

Jeszcze większym potencjalnym źródłem problemów może być instrukcja else zapisana w tej samej linijce co kod, który się w jej ramach wykona. Jeszcze mocniej zaciemnia to kod i mimo, że C# pozwala na takie zabawy to jednak unikałbym tego praktycznie zawsze. Kod przede wszystkim powinien być prosty do czytania dla programisty bo maszyna sobie z nim zawsze poradzi.

Mam wrażenie, że autor obawia się zbyt długiego kodu. Jednak patrząc na punkt poprzedni widzę, że szuka oszczędności nie tam gdzie trzeba. Jestem przekonany, że każdy programista C# zwróciłby uwagę na taki zapis i zapytał „dlaczego?”.

Jednak nawet pomijając kwestie czytelności to zastosowanie w kilku miejscach takiego zapisu łamie dobrą praktykę aby kod w aplikacji był spójny. W tym momencie trzeba się dobrze wczytać żeby znaleźć takie pozbawione nawiasów bloki kodu w przypadku kiedy w innych miejscach nawiasy już spotykamy.

Podobne uwagi dotyczą np. niektórych instrukcji foreach.

Fabryka klas

Na koniec dzisiejszej części ciekawszy przypadek, który znajduje się w pliku ParticularSpecies.cs. Mamy tutaj klasy, które tak naprawdę reprezentują jedną klasę z różnymi parametrami. Fragment pliku wygląda w ten sposób:

Widzicie od razu co się tutaj stało i jakie jest rozwiązanie problemu?

Jedyne co się tutaj zmienia to wartości przypisywane do pól. W pliku jest 16 takich klas. W przypadku gdyby trzeba było dodać kolejny typ autor musiałbym dodać koleją klasę, która miałaby tylko inne parametry.

Rozwiązaniem tutaj jest zastosowanie wzorca typu fabryka i dodanie jedynie enuma z dostępnymi typami (kombinując mocniej nawet jego można by się pewnie pozbyć). Do tego niech klasa Species posiada konstruktor przyjmujący wszystkie potrzebne wartości i już możemy te 340 linii zamienić na coś tego typu:

Taka zmiana daje również dodatkowy potencjalny profit – autor będzie mógł bardzo łatwo dodać tworzenie tych obiektów na podstawie wartości zapisanych poza kodem aplikacji, np. w bazie danych albo pliku. Taka zmiana będzie dużo prostsza niż w pierwotnej wersji i będzie wymagała modyfikacji niewielkiego kawałka kodu.

W kodzie z tego pliku można wykonać jeszcze kilka ulepszeń. Jednak te zostawiam na kolejne części, o których dowiesz się śledząc bloga na bieżąco albo lajkując mój fanpage na Facebooku.

Podsumowanie

Bardzo dziękuję autorowi za zgłoszenie się ze swoim projektem. Mimo, że był on świadomy, że kod nie jest idealny to mimo wszystko miał odwagę zgodzić się na publiczny komentarz. A jeśli chodzi o komentarze to są one do Waszej dyspozycji tak samo jak komentarze na Facebooku pod postem z linkiem do tego tekstu. Zapraszam do dyskusji.

2 thoughts on “[Code review] Obozy kodu”

  1. Bardzo dziękuję za cenne uwagi. Jeśliby mój projekt miał się pojawić w kolejnych częściach, to czekam z niecierpliwością :)

    Kwestia nawiasów wydaje mi się interesująca. Wiem, że to kontrowersyjna praktyka. Już w przeszłości wiele osób radziło mi zawsze stawiać nawiasy. I szczerze mówiąc, już chciałem się dostosować i zacząć tak postępować… Aż trafiłem na to:

    https://softwareengineering.stackexchange.com/questions/320247/has-can-anyone-challenge-uncle-bob-on-his-love-of-removing-useless-braces

    https://softwareengineering.stackexchange.com/questions/16528/single-statement-if-block-braces-or-no

    Wygląda na to, że dla niektórych ludzi (w tym „samego” Uncle Boba) opuszczenie „zbędnych” klamerek czyni kod czytelniejszym. Mało tego, niektórzy ludzie, jako Salomonowe wyjście, promują stawianie wyrażenia na tej samej linii, co instrukcji sterującej. Stąd się wzięło moje else return…

    Po lekturze tych wątków na Stacku zacząłem się zastanawiać, czy może jednak ta praktyka nie jest tak jednoznacznie szkodliwa? Dlatego pisząc kod, pozwoliłem sobie ją czasem stosować.

    Interesująca jest też dla mnie porada, jak zrefaktoryzować klasę Species. Jeśli dobrze rozumiem, to w tym momencie wszystkie dane opisujące konkretny Species traktujemy tu jako, no właśnie – dane, a nie kod. Rzeczywiście muszę przyznać – takie podejście zdaje się narzucać się samo.

    Jeśli mogę dać trochę kontekstu…

    Mój pierwszy pomysł to było potraktować w ten sposób i Species, i Move, i Type. Move miał być po prostu (być może pozagnieżdżaną) listą Actions. (Echa tego podejścia chyba jeszcze widać w Moves.cs.) W ogóle pierwszym moim pomysłem było pobierać te dane z pliku konfiguracyjnego i tak właśnie zacząłem to pisać.

    Ale wówczas przeczytałem to:

    http://thedailywtf.com/articles/Soft_Coding

    I zacząłem sądzić, że wpadłem w tę pułapkę. A więc, zrezygnowałem z tego pomysłu. I uczyniłem każdy osobny Move, każdy osobny Species, każdy osobny Type odrębną klasą.

    No i szybko się okazało, że Types, i przede wszystkim Moves, jakoś nie dają się traktować jako po prostu dane. Kodu tam jest sporo. Species, jak widać, pięknie się jako zbiór danych dadzą traktować. Ale, być może to tylko dlatego, że nie zaimplementowałem passive abilities. Troszkę się boję, że jeśli istotnie uczynię wszystkie Species tylko jedną klasą, to próba dodania passive abilities może ten model rozbić w podobny sposób, jak to się stało z Moves i Types. Być może okazałoby się, że traktowanie passive abilities każdego Species jako listy efektów jest tak samo niewystarczające, co traktowanie dowolnego Move jako listy akcji, czy dowolnego Type jako listy vulnerabilities / resistances.

    Ale z drugiej strony, istotnie w Moves, wydaje mi się, że zrobiło mi się spaghetti. Być może gdyby zacząć czyścić kod Moves, to dałoby się tam także zastosować fabrykę?

    1. Jeżeli chodzi o nawiasy to zwróciłem na nie uwagę w pierwszej kolejności trochę prowokacyjnie ;) Zgadzam się, że jest to element dyskusyjny i każdy ma trochę odmienne zdanie. Ale ponieważ masz argumenty na poparcie swojej tezy to jak najbardziej to akceptuję. Chociaż uważam, że pasuje to bardziej do takich języków jak Java, gdzie standardowo pisze się nawias otwierający w tej samej linii co instrukcję czy funkcję.
      Z resztą taka uwaga dla tych, którzy jeszcze nie zaczęli pracy, a teraz to czytają – nie zawsze chodzi o to żeby robić tak jak wam ktoś mówi, ale ważne żeby mieć poparcie swoich przekonań. Tak jak w tym przypadku. Code review to nie rozkaz tylko komentarz, dyskusja, wymiana wiedzy ;) Z resztą miałem o tym jakiś czas temu tekst na blogu https://zajacmarek.com/2018/05/code-review-konsultacje-i-czystosc-kodu-kluczem-do-sukcesu/
      Jeśli chodzi o pisanie lub nie pisanie klas dla akcji czy efektów i ich organizację to tutaj temat jest bardziej rozbudowany. Jako, że dodałeś naprawdę merytoryczny komentarz i w ogóle podesłałeś ciekawy projekt to postaram się w najbliższym czasie złożyć tekst gdzie pokażę jak ja bym to zrobił po prostu działając na Twoim kodzie i dając gotowe rozwiązanie. Bo tutaj sądzę, że można jednocześnie zastosować kilku sprytnych wzorców czy przeorganizować kod i jednocześnie uniknąć przeinżynierowania całości na wzór korpo-projektów. Myślę, że powstanie dzięki temu kawałek przydatnego tekstu na temat organizacji kodu, który nie tylko dla Ciebie będzie interesujący czy przydatny :)

Dodaj komentarz

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