Code review

Odpowiedz Nowy wątek
2018-08-21 11:04

Rejestracja: 1 rok temu

Ostatnio: 4 tygodnie temu

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

Pozostało 580 znaków

2018-08-21 11:31

Rejestracja: 11 lat temu

Ostatnio: 3 minuty temu

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

Pozostało 580 znaków

2018-08-21 11:56

Rejestracja: 4 lata temu

Ostatnio: 40 minut temu

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.

edytowany 7x, ostatnio: Desu, 2018-08-21 12:11
dobra, zabrałem się za to co tutaj wypisałeś - przeniosłem paginację do osobnej klasy, użyłem gotowej walidacji i mam teraz pytanie odnośnie tego co napisałeś na samym dole, czyli że w kontrolerach nie powinno się tyle dziać. Czyli co, takie rzeczy jak przygotowanie danych do szablonu też powinno być wydzielone do osobnych klas? Przykładowo takie coś jest pokazywane w dokumentacji symfony - ale teraz rozumiem, że nie powinienem wszystkiego brać za wzór. - Meybea 2018-08-25 17:47

Pozostało 580 znaków

2018-08-21 12:47

Rejestracja: 1 rok temu

Ostatnio: 4 tygodnie temu

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.

Pozostało 580 znaków

2018-08-21 13:05

Rejestracja: 4 lata temu

Ostatnio: 40 minut temu

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');
edytowany 5x, ostatnio: Desu, 2018-08-21 13:28

Pozostało 580 znaków

2018-08-21 14:30

Rejestracja: 1 rok temu

Ostatnio: 4 tygodnie temu

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

Pozostało 580 znaków

2018-08-21 15:33

Rejestracja: 1 rok temu

Ostatnio: 4 tygodnie temu

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?

Szukaj i się ucz cały czas, przy czym bądź gotowy na to, że dużo pracodawców może Cię odrzucić - ale 1 na N się znajdzie i Cię przyjmie :) - Markuz 2018-08-21 15:43
Samo pisanie CV to dla mnie też skomplikowana sprawa, głównie z tego powodu, że w ofertach pracy wszyscy posługują się ogólnikami, przykładowo "stanowisko junior php developer wymagania: Bardzo dobra znajomość PHP 7 OOP (albo znajomość php i obok 5 gwiazdek) (...)" - Meybea 2018-08-21 15:59
Kiedyś oglądałem jedną prelekcję gdzie rekruter wypowiadał się na ten temat, mówił że na 5 gwiazdek to ci co rozwijają ten język może potrafią, a na 4 to dobry senior może potrafi. Więc wnioskuję, że określenie umiejętności to kwestia subiektywna a 5 gwiazdek czy "bardzo dobrych" umiejętności bym sobie nie dał - Meybea 2018-08-21 16:00

Pozostało 580 znaków

2018-08-21 16:19

Rejestracja: 4 lata temu

Ostatnio: 40 minut temu

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.

edytowany 1x, ostatnio: Desu, 2018-08-21 16:22
Co miałeś na myśli pisząc, że liczy się wiek? - Meybea 2018-08-21 21:12
Nie ma co ukrywać, że 20-30 letni junior ma łatwiej niż 40-50 letni junior. Większość osób od kogoś w tym wieku podświadomie oczekuje dużej wiedzy z danej dziedziny, a nie postawy zapalonego, ledwo co opierzonego, zielonego studenta. - Desu 2018-08-21 22:37

Pozostało 580 znaków

2018-08-21 16:38

Rejestracja: 5 lat temu

Ostatnio: 4 godziny temu

Lokalizacja: Piwnica

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


Pozostało 580 znaków

2018-08-21 17:46

Rejestracja: 1 rok temu

Ostatnio: 4 tygodnie temu

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

edytowany 2x, ostatnio: Meybea, 2018-08-22 14:17

Pozostało 580 znaków

2018-08-23 18:14

Rejestracja: 1 rok temu

Ostatnio: 4 tygodnie temu

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?

Nie ten dzial. Załóż nowy temat w dziale js, jeżeli chodzi tylko o ja, lub w dziale webmastering jeżeli interesuje Cię również css - Desu 2018-08-24 06:34

Pozostało 580 znaków

Odpowiedz

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