Code review

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

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

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

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.

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.

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');

Ż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ę :)

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?

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.

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 :)

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

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?

@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 ;)