Code review

1

Witam,
Mam tutaj kod do oceny.
Najpierw może kilka słów na temat projektu. Jest to projekt który zacząłem pisać w ramach nauki, początkowo projekt miał być użyteczny, grałem kiedyś w taką gierke gdzie gracz nie otrzymywał w grze prawie żadnych informacji o broni, tak się złożyło, że developerzy gry udostępnili część danych więc to wykorzystałem i napisałem taki o to projekt (developerzy nie mają chyba zamiaru udostępniać reszty danych więc projekt nie będzie użyteczny). Przed projektem coś tam skrobałem ale to było takie bazgranie żeby sobie posprawdzać jak co działa.

Podczas pisania projektu miałem po raz pierwszy styczność z Twigiem, jquery, symfony, doctrine, bootstrapem tak więc jestem początkujący (bez wyzwisk za kod proszę :D)
Piszę ten post z tego powodu, że na ten moment chcę przestać dodawać nowe funkcjonalności i popracować nad jego "czystością" już sam sobie nawet zrobiłem listę w której zapisałem rzeczy które według mnie są nie ok i zobaczymy jak to się pokryje z waszymi opiniami.

W README macie obrazki do sprawdzenia jak stronka wygląda.

A no i mój angielski też nie jest najlepszy, jego nawet nie komentujcie ;)

https://github.com/JarekDylewski/HandGExpert

1
  1. Usuń wszystko co ma być usunięte zanim oddajesz kod do Code Review. (zakomentowane metody, klasy z komentarzem do usunięcia itp.)
  2. https://github.com/JarekDylewski/HandGExpert/blob/master/src/Data/PrepareData.php - wrzuć te dane do jakiegoś JSON
  3. https://github.com/JarekDylewski/HandGExpert/blob/master/src/Controller/tutorials/TutorialsArticleController.php - dużo brzydkiego kodu, postaraj się wydzilić walidację do osobnego obiektu (symfony to ogarnia), nazywaj zmienne z małej litery, pobieranie danych przenieś do repozytorium
4

bez wyzwisk za kod proszę

Code Review ma pomóc Tobie (żebyś mógł pisać lepszy kod i się rozwijał) i ludziom z Twojego zespołu (żeby nie musieli pływać w szambie). Nikt Cie nie powinien wyzywać. Jak ktoś tak robi podczas CR'ki to możesz mu podziękować i znaleźć innego recenzenta. Idąc na code review powinieneś się cieszyć, a nie bać, że ktoś Cie zmiesza z błotem.

Nie bierz też tych uwag do siebie osobiście, bo to że ktoś ma uwagi do Twojego kodu to normalna sprawa. Wszyscy piszemy dupny kod, ale za to jesteśmy zarąbiści w krytykowaniu i znajdywaniu błędów w cudzym kodzie.

Ta książka Ci bardzo pomoże: https://www.amazon.com/Refactoring-Improving-Design-Existing-Code/dp/0201485672



Wszędzie zakomentowany kod i gdzieniegdzie źle sformatowany (np. brakuje spacji przy przecinkach/nawiasach, nazwy zmiennych raz z wielkiej, raz z małej litery).

Czasami używasz type hintów, a czasami nie. Staraj się ich używać wszędzie (łącznie z wartościami skalarnymi, jak int, string, bool).

```php @param $what {string} ``` Staraj się nie komentować bez sensu. Zamiast tego użyj typów i dobierz dobrą nazwę. Wtedy komentarz jest zbędny. Nazewnictwo, nazewnictwo, nazewnictwo...

Przy okazji zamiast stringa lepiej by było, żeby tu była tablica, albo coś... wtedy ktoś nie musi wiedzieć jaki format tych kolumn wymagasz.
W PHPStorm masz skrót (cmd + P lub alt + P) który przy wywołaniu metody pokazuje Ci jakie parametry musisz podać. Widząc tam $what programista dokończy na głos: WHAT THE FUCK i nie będzie miał zielonego pojęcia co ma tam wpisać, żeby Twoja metoda była zadowolona i zwróciła sensowny wynik.
W tym momencie musisz dać albo komentarz, że to kolumny oddzielone przecinkiem, albo zmienić nazwę parametru na commaSeparatedColumns. Z resztą ja w ogóle nie jestem fanem takich super ogólnych funkcji, które sobie wszystko zwracają. Poza tym ->select('w.'.$what) może być wrażliwe na sql injection.

/**
 * @return TutorialArticle[] - to może zostać, bo niestety PHP tego nie ogarnia jeszcze :(
 */
public function select(string $commaSeparatedColumns, $order = 'DESC'): array


```php class TopicValidator ``` Z tego co widzę, to odpowiedzialnością tej klasy nie jest walidacja. Ona coś tam sobie mapuje i usuwa duplikaty, więc warto jej zmienić nazwę.

```php function deleteObjDuplicateFromArray($array, $keep_key_assoc = false){ ``` Cały ten kod można zastąpić: ```php array_unique( array_map(function($element) { return is_object($element) ? (array) $element : $element; }, $input), SORT_REGULAR ); ```

```php public function getAmmoData():array { $ammoArray = array( ``` To powinno być w bazie/pliku json/xml gdziekolwiek poza plikiem `*.php`.

```php /** * @param string|int $IDOrAllGunsData set 'all' for all arrays, int with gun ID * @return array|mixed|string * @throws PrepareData "there is no ID smaller than 0" if set number less then 0; * @throws PrepareData "you can set only string "all" (or "not defined" but then ID is set from the constructor)" * @throws PrepareData "in "getGunsData" method there is no array with ID:[set ID]" * @throws PrepareData "in "getGunsData" method there is no array with ID:[set ID]" * @return array array with guns data */ public function getGunsData($IDOrAllGunsData = 'not defined'):array { ``` 1. Masz return type array, a w dokumentacji `array|mixed|string`, to jak to jest? (prawidłowo powinien być jeden return type) 2. Nie potrzebujesz komentarzy w stylu `@param` lub `@return` jeżeli dobrze nazwiesz metodę i użyjesz dobrodziejstw PHP 7 (czyli scalar type hints). 3. `@throws` powinno zawierać exception, które może zostać wyrzucone, np. `@throws \Exception`. Lepiej jednak wyrzucać exception, którego nazwa już coś nam mówi `@throws InvalidArgumentException` `@throws GunDoesNotExistException`, wtedy nie będziesz musiał ich dodatkowo opisywać. 4. `$IDOrAllGunsData`. Jeżeli nazwa twojej zmiennej lub metody ma w sobie `or` albo `and` to wiedz, że coś się dzieje (czyt. prawie na pewno ukrywa ona wiele odpowiedzialności). 5. Wszelkie magiczne zmienne typu `'not defined'` powinieneś wydzielić do jakiejś stałej i nią się posługiwać. Jeszcze lepiej [gdyby to był enum](https://github.com/myclabs/php-enum).

```php class ModsDataPrepare ``` Znowu nazewnictwo. Nazwa klas/zmiennych nie powinny używać czasowników. Lepszą nazwą może być `CośTamProvider` z metodą `getData/prepare/provide`, czy coś w tym stylu.

```php $numberPagination = count($allTutorialsArticleID)/5; ``` Do tego jest specjalny bundle. Możesz użyć [tego](https://github.com/KnpLabs/KnpPaginatorBundle), albo jakiegoś innego. Jeżeli chcesz to zrobić sam, to ta logika nie powinna tu być, powinieneś ją wydzielić do osobnej klasy, chociaż by po to, żeby móc jej ponownie użyć.

```php public function approvalTutorialArticle() ``` Dlaczego tutaj się tyle dzieje... Walidacja powinna wylądować w osobnym pliku. Poza tym [masz do tego gotowe komponenty](https://symfony.com/doc/current/validation.html).

```php $author = $_POST['tutorial_author']; ``` GLOBALEEEEEEEE :( Możesz zrobić tak: ```php public function approvalTutorialArticle(Request $request) { // ... $request->request->get('tutorial_author'); ```

Ogólnie odnośnie kodu w kontrolerze, to dzieje się tam mnóstwo rzeczy. **W kontrolerze powinny być tylko rzeczy specyficzne dla danego IO**.

Jeżeli Twoim IO jest przeglądarka web, to wtedy jakieś pobieranie parametrów z requesta, renderowanie widoku i dodawanie jakiś komunikatow o błędzie do sesji.

Jeżeli Twoim IO jest Symfony Command, to powinno tam być tylko wypychanie komuniaktów do OutputInterface i pobieranie argumentów z InputInterface. Ewentualnie renderowanie jakiegoś paska postępu. Cała reszta powinna być wydzielona do osobnego serwisu.

Jeżeli będziesz chciał dodać inny kontroler dla apki mobilnej, albo właśnie jakiś command to co wtedy? Będziesz musiał zduplikować logikę, która tworzy coś, co tam sobie tworzysz, bo nie będziesz mógł użyć kontrolera dedykowanego dla web'a dla apki mobilnej, a tym bardziej dla commanda.

0

Dzięki za odpowiedzi, postaram się wprowadzić poprawki do tego co napisaliście :)
Wyjaśnię tutaj kilka rzeczy:
Tych komentarzy tam nie powinno być, wczoraj specjalnie skopiowałem cały projekt i kasowałem wszystkie niepotrzebne komentarze, coś musiałem w gicie chyba namieszać.
Nazwy zmiennych raz z małej raz z dużej- tutaj zawsze zmiennej która ma przypisany obiekt dawałem na początku dużą literkę (dla mnie jest to naprawdę wygodne) ale to też postaram się zmienić.

@param string|int $IDOrAllGunsData set 'all' for all arrays, int with gun ID
"Masz return type array, a w dokumentacji array|mixed|string, to jak to jest? (prawidłowo powinien być jeden return type)"
Zawsze zwraca tablicę, to chyba akurat było wygenerowane przez PHPStorma, pewnie nawet prawidłowo - możliwe, że wcześniej zwracany był też string.

Jeszcze co do przechowywania danych w tablicach, początkowo to miał być JSON, ale miałem z tym problemy, juz nie pamiętam nawet o co w tym dokładnie chodziło ale chyba przy pobieraniu zwracał mi NULL, zapisem do bazy nie chciałem się nawet zajmować (miałem wtedy sporo rzeczy do ogarnięcia) więc zostało to w tablicach. Ogólnie to też się pytałem innych o radę i mówili, że jak dane nie będą się rozrastać to może być w tablicach.

1

Jeżeli już chcesz mieć to w tablicy, to chociaż w innym pliku, bo tutaj kod jest zaciemniony przez ogromną ilość danych.
@Meybea Możesz ewentualnie użyć czegoś takiego (dzięki interfejsowi możesz to później łatwo podmienić na inne repo, np. używające Doctrine'a)

<?php

class Gun
{
	private $name;

	public function __construct(string $name)
	{
		$this->name = $name;
	}

	public function getName(): string
	{
		return $this->name;
	}
}

interface GunRepositoryInterface
{
	public function findAll(): array;

	public function save(Gun $gun);

	public function delete(string $name);
}

class FileGunRepository implements GunRepositoryInterface
{
	private $dbPath;

	private $guns;

	public function __construct($dbPath)
	{
		$this->dbPath = $dbPath;
		$this->guns   = unserialize(file_get_contents($dbPath));

		if (true === empty($this->guns))
		{
			$this->guns = [];
		}
	}

	/**
	 * @return Gun[]
	 */
	public function findAll(): array
	{
		return $this->guns;
	}

	public function save(Gun $gun)
	{
		$this->guns[] = $gun;
	}

	public function delete(string $name)
	{
		$this->guns = array_filter($this->guns, function (Gun $gun) use ($name) {
			return $gun->getName() !== $name;
		});
	}

	public function __destruct()
	{
		file_put_contents($this->dbPath, serialize($this->guns));
	}
}

$dbPath         = __DIR__ . DIRECTORY_SEPARATOR . 'db.ser';
$gunsRepository = new GunRepository($dbPath);

$gunsRepository->save(new Gun('AK47'));
$gunsRepository->save(new Gun('Glock'));
$gunsRepository->findAll();
$gunsRepository->delete('Glock');
0

Że ci się chciało to pisać :D Chyba zacznę doceniać code review, na początku bałem się wrzucać swój kod a tu proszę, wcale tak źle nie jest, jeszcze można się sporo dowiedzieć! I to pewnie działa w obie strony bo wy też macie swoją gumową kaczuszkę :)

0

Ogólnie mam też takie pytanie.
Mam zamiar wkrótce szukać pracy na jakieś stanowisko juniorskie, czy taki projekt warto podlinkować w cv, czy może jest za słaby żeby go pokazywać?
A może w ogóle wstrzymać się z szukaniem pracy z takimi umiejętnościami?

1

W pierwszej pracy bardziej liczy się Twój wiek i attitude niż umiejętności, czy kodzik. Ja osobiście szukałem pracy po 2 tych nauki HTML/CSS i ją dostałem. Trochę było w tym szczęścia, a trochę umiejętności autoprezentacji. Później oczywiście miałem pół roku wyjęte z życia, bo nic innego nie robiłem tylko siedziałem i programowałem, żeby się w tym JanuszSofcie utrzymać.

Na forum i w internecie jest mnóstwo tematów jak się sprzedać, jak napisać dobre CV gdy się nic nie umie. Są też ogólne porady jak dostać pracę i jak przejść rozmowę. Jeżeli jesteś gotowy na naukę i Cię to kręci, to nie widzę powodu, żeby czekać :)

Pamiętaj też, że większość Juniorów nie ma nic na githubie, a jeżeli już coś mają to jakieś oklepane, powtarzalne g**no z bootcampu, którego (w większości) nie napisali sami. Ty nie dość, że napisałeś to sam, to jeszcze sam zauważyłeś problem i postanowiłeś stworzyć aplikację, która go rozwiąże, a to bardzo obiecujące myślenie.

1

fajny projekcik, na pewno leszy od tych ostatnich co były wrzucane, ja bym dodał w readme jakiś opis przed screenami co tam sie w ogóle odpierdziela :)

0

fajny projekcik, na pewno leszy od tych ostatnich co były wrzucane, ja bym dodał w readme jakiś opis przed screenami co tam sie w ogóle odpierdziela :)

Opis dodany

0

Pytanie dotyczące konwencji nazewnictwa - w plikach js stosowałem jakiś_tam_plik.js dla plików które obsługują daną stronę np range_panel.js (gdzie były wyświetlane statystyki dotyczące zasięgu broni) za to dla obiektów js stosowałem juz nazewnictwo JakisTamObiekt.js w HTML jeśli dawałem jakąś klasę czy ID tylko po to żeby obsłużyć to JS'em to też dawałem jakaś_tam_nazwa.js w nazwach tylko po to żeby coś sformatować CSS'em dawałem jakas-tam-nazwa. Nie wiem czy to dobra praktyka i czy tak wgle powinno się robić. Sam muszę się przyznać, że pogubiłem się troszkę w tej konwencji nazewnictwa, czytałem, że zasada ogólnie jest taka żeby trzymać się jednej lub stosować do standardów kodowania a u siebie do różnych rzeczy zastosowałem różne i wyszło na to, że nazwy plików JS w katalogu dotyczącym artykułów odbiegają od mojej konwencji - b, robiłem to w sporym odstępie czasowym i zapomniałem.
Jak to jest, powinienem się trzymać sztywno i wszystko nazywać w taki sam sposób?

1

@Meybea: Tak. Taka klasa może mieć w nazwie Mapper/Transformer/(Data)Provider/Presenter albo coś takiego.

Jak masz wątpliwość, to możesz przeprowadzić taki proces myślowy:

  1. Otwierasz kontroler. 2. Patrzysz na interesującą Cię metodę i zadajesz sobie pytanie: czy jeżeli będę chciał mieć dostęp do tej funkcjonalności z poziomu Symfony Command, to czy będę zmuszony zduplikować jakiś kod, który się tu znajduje?
  2. Jeżeli odpowiedź brzmi tak, to znaczy, że dobrze by było ten kod wydzielić do jakiegoś serwisu. Jeżeli odpowiedź brzmi nie, to znaczy, że w Twoim kontrolerze (poza wywołaniem serwisu) znajdują się tylko rzeczy specyficzne dla danego IO (web/api/cli).

Na konkretnym przykładzie:
Załóżmy, że mamy przypadek tworzenia/rejestracji użytkownika.

  1. Przypadek pierwszy - nie mamy dedykowanego serwisu:
class UserController
{
    public function registerAction(Request $request): Response
    {
        $user = $this->userRepository->findByEmail($request->get('email'));

        if (true === $user instanceof User) {
            // jakieś błędy, bo użytkownik istnieje
        }

        $user = new User();
        $user->firstName = $request->get('first_name');
        $user->lastName = $request->get('last_name');
        $user->password = $this->passwordEncore->encode($request->get('password'));

        $this->entityManager->persist($user);
        $this->entityManager->flush();

        $event = new UserRegistered($user);
        $this->eventDispatcher->dispatch($event);
        
        // renderowanie widoku, czy jakies tam przekierowanie czy tam cos
    }
}

Teraz chcemy móc utworzyć użytkownika z cli:

class CreateUserCommand
{
    public function execute(InputInterface $input, OutputInterface $output)
    {
        // dużo kodu, który pobiera te parametry od usera
        
        $user = $this->userRepository->findByEmail($input->get('email'));

        if (true === $user instanceof User) {
            // jakieś błędy, bo użytkownik istnieje
        }

        $user = new User();
        $user->firstName = $input->get('first_name');
        $user->lastName = $input->get('last_name');
        $user->password = $this->passwordEncore->encode($input->get('password'));

        $this->entityManager->persist($user);
        $this->entityManager->flush();

        $event = new UserRegistered($user);
        $this->eventDispatcher->dispatch($event);
        
        // dużo kodu, że się udało itp.
    }
}

Bleeeeh! Duplikacja!

  1. Serwis do tworzenia użytkownika:
class UserManager
{
    public function register($firstName, $lastName, $email, $password)
    {
        $user = $this->userRepository->findByEmail($email);

        if (true === $user instanceof User) {
             throw new UserAlreadyExistsException();
        }

        $user = new User();
        $user->firstName = $firstName;
        $user->lastName = $lastName;
        $user->password = $this->passwordEncore->encode($password);

        $this->entityManager->persist($user);
        $this->entityManager->flush();

        $event = new UserRegistered($user);
        $this->eventDispatcher->dispatch($event);
    }
}
class UserController
{
    public function registerAction(Request $request): Response
    {
        try {
            // te parametry można do jakiegoś DTO'sa czy coś tam
            $this->userManager->register(
                $request->get('first_name'),
                $request->get('last_name'),
                $request->get('email'),
                $request->get('password')
            );
        } catch (UserAlreadyExistsException $exception) {
            $this->flashMessage('error', 'email istniee') // ląduje do sesji, bo w kontekście webowym mamy sesję
        }
        // renderowanie widoku, czy jakies tam przekierowanie czy tam cos
    }
}
class CreateUserCommand
{
    public function execute(InputInterface $input, OutputInterface $output)
    {
        // dużo kodu, który pobiera te parametry od usera

        try {
            $this->userManager->register(
                $input->get('first_name'),
                $input->get('last_name'),
                $input->get('email'),
                $input->get('password')
           );
        } catch () {
            $output->error('..') // ląduje jako output do konsoli, bo nie mamy tutaj sesji!
        }

        // dużo kodu, że się udało itp.
    }
}

Teraz mamy w controllerze/comandzie kod odpowiedni dla danego IO i nic poza tym. Nie możesz specjalne już nic wydzielić, bo to jest właśnie odpowiedzialność controllera przetworzenie żądania, pobranie parametrów wejściowych i wygenerowanie odpowiedzi. Całe mięcho controller przekierowuje dalej.

Analogia, którą się często posługuję tłumacząc to nowym programistom jest analogia z klientem w restauracji, kelnerem i kucharzem. Klient (użytkownik przeglądarki) składa zamówienie (wykonuje żądanie). Kelner je przyjmuje (po polsku, po angielsku, albo telefonicznie) i tyle. On nie wie jak przyrządzić stejka, a ponieważ nie wie to delegują tą odpowiedzialność do kucharza. Kucharz nie wie jak klient złożył zamówienie. Nie wie czy on mówi po polsku, angielsku, czy zadzwonił. Kucharz ma to gdzieś, on tylko zajmuje się gotowaniem. Jeżeli wystąpi jakiś błąd, bo na przykład skończyła się wołowina (exception!), to mówi to kelnerowi, a kelner idzie poinformować klienta (generuje response w taki sposób jaki jest odpowiedni dla danego klienta, albo idzie na sale, albo dzwoni do niego). Widziałes, kiedyś żeby szef kuchni wszedł na sale powiedzieć klientowi, że czegoś nie ma? :) Jak nie było błędów i już zrobi, co ma zrobić to przekazuje potrawę kelnerowi. Kelner ją bierze i zanosi klientowi w takiej formie (buduje response), jaka jest odpowiedni dla danego klienta (klient, mógł chcieć na wynos, więc trzeba zapakować). I to wszystko :)

Sam nie raz byłem świadkiem tego, że na początku było zapytanie w controlerze i warunki też byłī tam nakładane. Później trzba było też wyszukać to samo ale z poziomu commanda i jeszcze crmu (inny controller). W rezultacie w 3 miejscach był kod, który robi to samo ale różnił się jakimiś tam szczegółami, bo przy rozwijaniu wyszukiwarki programista zapominał dopisać kod w pozostałych 2 miejscach. Przy okazji jak już coś jest w 3 miejscach skopiowane to rozrasta się jak rak. Kolejny programista ma do wyboru przekopiować, albo zrobić refacktoring... często jak ma w nosie prace, albo jest mało doświadczony to weźmie i skopiuje 4... pozniej 5 i 6 raz i koło się zamyka.

PS. Wybacz, że tak niechlujnie napisane, ale nie mam za dużo czasu ;)

1 użytkowników online, w tym zalogowanych: 0, gości: 1