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/JarekDylew[...]ster/src/Data/PrepareData.php - wrzuć te dane do jakiegoś JSON
  3. https://github.com/JarekDylew[...]utorialsArticleController.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/Refact[...]n-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).



 @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


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ę.



function deleteObjDuplicateFromArray($array, $keep_key_assoc = false){

Cały ten kod można zastąpić:

array_unique(
    array_map(function($element) {
        return is_object($element) ? (array) $element : $element;
    }, $input),
    SORT_REGULAR
);


public function getAmmoData():array
{
    $ammoArray = array(

To powinno być w bazie/pliku json/xml gdziekolwiek poza plikiem *.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.


    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.



    $numberPagination = count($allTutorialsArticleID)/5;

    Do tego jest specjalny bundle. Możesz użyć tego, 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ć.



    public function approvalTutorialArticle()

    Dlaczego tutaj się tyle dzieje... Walidacja powinna wylądować w osobnym pliku. Poza tym masz do tego gotowe komponenty.



    $author = $_POST['tutorial_author'];

    GLOBALEEEEEEEE :( Możesz zrobić tak:

    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

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