Drukowana wersja tematu

Napisany przez: smk 17.08.2018, 16:34:55

Witam.
Prosiłbym Was o code review aplikacji do skracania adresów Czym więcej feedbacku, tym lepiej!
https://github.com/selfmadeking/URL-shortener-Symfony-4

Napisany przez: athabus 23.08.2018, 21:25:44

Kurcze, aż szkoda żeby taki fajny temat się zmarnował, więc choć się nie znam, to pierwszy rzucę kamieniem - może bardziej doświadczeni koledzy się przyłączą ;-)

Po pierwsze bardzo nie podoba mi się brak typowania zmiennych i pól w klasie.

[PHP] pobierz, plaintext 
private $urlGeneratorService;
    private $em;
 
    public function __construct(UrlGeneratorService $urlGeneratorService, EntityManagerInterface $em)
    {
        $this->urlGeneratorService = $urlGeneratorService;
        $this->em = $em;
    }
[PHP] pobierz, plaintext 

[PHP] pobierz, plaintext 
public function userIsAuthorOfList($list)
    {
        $urls = $list->getListOfUrls();
        $userId = $this->security->getUser();
        if (http://www.php.net/isset($userId)) {
            $userId = $userId->getId();
        }
        $userIdOfUrl = $urls->first()->getUserId();
        if (http://www.php.net/isset($userIdOfUrl)) {
            $userIdOfUrl = $userIdOfUrl->getId();
        }
 
        return (bool) $userIdOfUrl == $userId;
 
    }
[PHP] pobierz, plaintext 

[PHP] pobierz, plaintext 
$userId = $this->security->getUser();
        if (http://www.php.net/isset($userId)) {
            $userId = $userId->getId();
        }
 
[PHP] pobierz, plaintext 

[PHP] pobierz, plaintext 
$userIdOfUrl = $urls->first()->getUserId();
[PHP] pobierz, plaintext 

[PHP] pobierz, plaintext 
return (bool) $userIdOfUrl == $userId;
[PHP] pobierz, plaintext 

Napisany przez: Pyton_000 24.08.2018, 08:07:15

Jedna uwaga do Form->isValid. Owszem sprawdza ale rzuca exception. Nawet sama treść wskazuje aby używać wpierw `isSubmitted()`
https://github.com/symfony/form/blob/master/Form.php#L736

Napisany przez: athabus 26.08.2018, 17:15:12

No proszę, człowiek uczy się całe życie. Zawsze dla czytelności robiłem if (isPost) {} i dlatego ładnie działało samo isValid(), a tutaj faktycznie kolega tego nie robi i u niego wywali wyjątek.

Napisany przez: Pilsener 1.09.2018, 22:14:03

Jak powyżej, np. taka metoda:

[PHP] pobierz, plaintext 
    public function validate($value, Constraint $constraint)
    {
        if (null === $value || '' === $value) {
            return;
        }
        if (!http://www.php.net/is_string($value)) {
            throw new UnexpectedValueException($value, 'url');
        }
        $pattern = '/^(https?:\/\/)?([\da-z\.-]+)\.([a-z\.]{2,6})([\/\w \.-]*)*\/?/';
        if (!http://www.php.net/preg_match( $pattern, $value, $matches)) {
            $this->context->buildViolation($constraint->message)
                ->setParameter('{{ url }}', $value)
                ->addViolation();
        }
    }
[PHP] pobierz, plaintext 

Napisany przez: Pyton_000 2.09.2018, 09:49:50

Odniosę się do pkt.6 z którym się nie zgodzę. Nie widzę sensu nie przerywanie programu tak szybko jak to tylko możliwe. Redukuje to ilość ifów które potencjalnie mogą wystąpić. Dużo łatwiej się czyta że np. jak jest teraz zamiast przenosić tego returna na koniec i w całym kodzie szukać gdzie jaki warunek jest spełniony i co to zwraca.

Napisany przez: viking 2.09.2018, 09:54:35

Swoją drogą tam wzorzec na url jest niepoprawny. Nie uwzględnia domen np z polskimi znakami ani żadnej współczesnej

Napisany przez: athabus 2.09.2018, 15:22:32

Z if'ami każdy ma swoją teorię. Ja akurat też jestem zwolennikiem przerywania jak najwcześniej. W "Czystym kodzie" był opisywany ten aspekt i tam Marti pisał, że jedno wyjście z funkcji to zaszłość z programowania proceduralnego z długimi i skomplikowanymi funkcjami. Tam faktycznie jeden return miał sens, ale w dobie tanich funkcji, gdzie piszemy (powinniśmy pisać) funkcje, które mają po kilka - kilkanaście liniejek, wg niego kilka wyjść z funkcji poprawia czytelność. Ja osobiście w swoim kodzie staram się jak mogę unikać "else" bo wg mnie psuje czytelność, więc tym bardziej stosuje wczesne returny.

Z drugiej strony rozumiem podejście, że funkcja powinna mieć jedno wyjście - po prostu nie zgadzam się z nim w przypadku krótkich funkcji.

Takie porównanie dla przykładu - funkcja przekształcająca daną wartość na tablicę (tak wiem, że bez sensu)

[PHP] pobierz, plaintext 
public function transformToArray($val): http://www.php.net/array
    {
        if (http://www.php.net/is_array($val)) {
            return $val;
        }
 
        if (http://www.php.net/is_object($var)) {
            return $this->transformObjectToArray($val);
        }
 
        if (http://www.php.net/is_scalar($var)) {
            return $this->transformScalarToArray($val);
        }
    }
 
    public function transformToArray2($val): http://www.php.net/array
    {
        $result = null;
        if (http://www.php.net/is_array($val)) {
            $result = $val;
        } else if (http://www.php.net/is_object($var)) {
            $result = $this->transformObjectToArray($val);
        } else {
            $result = $this->transformScalarToArray($val);
        }
 
        return $result
    }
[PHP] pobierz, plaintext 

Napisany przez: Pilsener 2.09.2018, 20:07:19

[PHP] pobierz, plaintext 
dump($result);
[PHP] pobierz, plaintext 

[PHP] pobierz, plaintext 
return ($a + $b);
[PHP] pobierz, plaintext 

[PHP] pobierz, plaintext 
public function doSth($x = null){
    if($x){
    $y = 0;
}else{
    $y = $x;
}
}
[PHP] pobierz, plaintext 

[PHP] pobierz, plaintext 
public function doSth($x = null){
    $y = 0;
    if($x){
    $y = $x;
}
}
[PHP] pobierz, plaintext 

Napisany przez: nospor 3.09.2018, 10:53:23

[PHP] pobierz, plaintext 
    public function transformToArray2($val): http://www.php.net/array
    {
        $result = null;
        if (http://www.php.net/is_array($val)) {
            $result = $val;
        }
        if ($result === null && http://www.php.net/is_object($var)) {
            $result = $this->transformObjectToArray($val);
        } 
       if ($result === null) {
            $result = $this->transformScalarToArray($val);
        }
 
        return $result
    }
 
[PHP] pobierz, plaintext 

[PHP] pobierz, plaintext 
return ($a+$b);
[PHP] pobierz, plaintext 

[PHP] pobierz, plaintext 
$c = $a+$b;
return $c;
[PHP] pobierz, plaintext 

Napisany przez: athabus 3.09.2018, 15:07:55

Kurcze Pilsener dla mnie 2 wersja jest bardziej czytelna. Może dlatego, że w pierwszej zgubiłeś ! w pierwszym warunku i się zastanawiałem co ta funkcja robi. Ale nawet bez tego jakoś przyzwyczaiłem się do drugiego zapisu i tak bym to zrobił... Widać wszystko kwestią gustu i coś dla jednych jest bardziej czytelne niż dla innych.

BTW poruszyłeś ciekawy temat, który uciekł w toku dyskusji, czyli CQS - command-guery separation. Ostatnio staram się stosować (ostatnio w sumie całe swoje kodowe nawyki zmieniam) i zauważyłem, że czasami powoduje puchnięcie kodu (zwłaszcza jak funkcja na wyższym poziomie grupuje działania wykonywane przez funkcje na niższym poziomie, gdzie część jest typu query, a inne typu command). Stosujecie tą regułę bez odstępstw, czy Wam się jednak zdarza. Ja staram się bez odstępstw, bo uznałem, że czytelny kod czasami może być dłuższy niż ten nieczytelny.

Napisany przez: Pilsener 4.09.2018, 09:45:20

[PHP] pobierz, plaintext 
if(!$sth)
[PHP] pobierz, plaintext 

[PHP] pobierz, plaintext 
if($sth == false)
[PHP] pobierz, plaintext 

[PHP] pobierz, plaintext 
if(!!$sth)
[PHP] pobierz, plaintext 

Napisany przez: athabus 4.09.2018, 19:29:26

Zgadzam się, że nie można na zasady patrzeć ślepo (zwłaszcza, że w praktyce często zasady okazują się wewnętrznie sprzeczne), ale akurat CQS jest taką zasadą, że albo się ją stosuje albo nie - tak mi się przynajmniej wydaje. Jeśli stosuje się ją tam, gdzie wygodnie to traci sens bo wracamy do punktu wyjścia i nadal nie wiemy czy metoda jest zapytaniem czy komendą bez zajrzenia w jej kod. Chyba nawet stosowanie jej w 95% jest gorsze niż nie stosowanie, bo wtedy ma się wrażenie że coś jest np. komendą, a nagle okazuje się mutować obiekt. Gdy jej nie stosujemy to przynajmniej wiemy, że w tym zakresie mamy w kodzie burdel i wszystko trzeba dokładnie sprawdzić... Takie jest moje zdanie, choć praktyki z CQS mam mało - nowy nabytek w arsenale ;-).

Napisany przez: Lion 23.09.2018, 09:30:48

Cytat(athabus @ 23.08.2018, 22:25:44 )

[PHP] pobierz, plaintext 
private $urlGeneratorService;
    private $em;
 
    public function __construct(UrlGeneratorService $urlGeneratorService, EntityManagerInterface $em)
    {
        $this->urlGeneratorService = $urlGeneratorService;
        $this->em = $em;
    }
[PHP] pobierz, plaintext 

Napisany przez: athabus 24.09.2018, 14:45:59

Ciekawe, że każdy ma swoje standardy w kwestii kodu.

Ja bym osobiście jednak komentarzy przy polach klasy nie uznał za nadmiarowe, bo to że ide rozpozna to jedna sprawa, a czytelność kodu to druga. Wchodząc do takiej klasy po miesiącu skąd mam wiedzieć jaki jest typ pola - musiałbym sprawdzić w konstruktorze / w innym miejscu, gdzie jest ustawiana wartość pola. Tak więc moim zdaniem jednak warto docka dać. Nadmiarowy to wg mnie taki, który powiela informacje z sygnatury funkcji bezpośrednio nad nią np:

Napisany przez: nospor 24.09.2018, 14:49:29

Na szczescie w nowym wydaniu php bedzie mozna typowac rowniez i wlasciwosci i nie bedzie juz takich rozterek

Napisany przez: athabus 24.09.2018, 14:54:18

Chyba czytasz w moich myślach, bo właśnie miałem napisać "ciekawe czy kiedyś wprowadzą opcjonalne typowanie pól" ;-)
No to jeszcze poproszę return type mixed przy funkcji i będzie super.

Napisany przez: nospor 24.09.2018, 15:03:38

Napisany przez: athabus 24.09.2018, 16:29:57

Czasami sa sytuacje, ze nie da sie w php przewidziec zwracanego typu. Np. dzisiaj budowalem konfiguracje do kontenera DI i odczyt wartosci parametru mogl zwracac wszystko od inta po inny obiekt. Nie da sie takiego czegos opisac bez doca, a brak zwracanego typu moze oznaczac, ze albo ktos nie oznaczyl typu, albo funkcja nic nie zwraca. Ja w takich sutuacjach zawsze daje doca return mixed aby uniknac dwuznacznosci i wolalbym to jawnie zdeklarowac dokladnie tak samo jak :void, gdy nic nie zwracamy.
Idealnie by bylo gdyby doc byl uzywany tylko, gdy trzeba dac cos wiecej niz to co mozna zawrzec w sygnaturze.

Napisany przez: nospor 24.09.2018, 18:41:44

Jesli nie wiesz, co bedzie zwracac twoja funkcja, znaczy ze masz zle zaprojektowany system

W nowym php tez maja wprowadzic typ OBJECT ktory bedzie ogolnie obiektem nie wazne jakim byle obiektem. Moze to ci pomoze troche ogarnac to co tam masz u siebie

Napisany przez: athabus 24.09.2018, 19:22:12

Czy ja wiem czy zle zaprojektowany. Php ma w sobie dynamike i czasem po prostu nie da sie twardo typowac (albo nie ma to sensu). Tak jak pisalem przyklad z dzisiaj - na wejsciu do kontenera DI mialem konfiguracje w postaci tablicy. Tablica zawierala parametry, ktore wykorzyatywalo reflection api do inicjalizacji klas. Co by nie zrobic to getter do takiej tablicy bedzie zwracal praktycznie dowolny typ danych. Object tu tematu nie rozwiaze, bo w takiej tablicy moga byc skalary, tablice, obiekty/kolejcje obiektow - praktycznie wszystko.
Od czasu jak narzucilem sobie typowanie udalo mi sie 90% sytuacji tak zorganizowac aby funkcja zwracala 1 i tylko 1 typ danych, co z perspektywy uwazam za super rozwiazanie, bo api metod i czytelnosc kodu na tym zyskaly. Ale w swiecie php zawsze znajdzie sie taki przypadek, ktorego nie da sie przerobic albo nie ma to sensu. Getter do rozbudowanej konfiguracji jest IMO takim przypadkiem.
Skoro jest void to i mixed moim zdaniem tez powinno byc aby zachowac jednolity interfejs. W php mixed to tez typ danych w pewnyn sensie

Napisany przez: nospor 25.09.2018, 10:36:37

Napisany przez: Pyton_000 25.09.2018, 16:58:11

Nie ma takiej sytuacji że nie da się tego opisać jednolitym typem danych. IMO kwestia znalezienia rozwiązania.

Napisany przez: borabora 27.09.2018, 07:21:27

Cytat(Pyton_000 @ 25.09.2018, 17:58:11 )

[PHP] pobierz, plaintext 
public function nazwa(?JakiśTypObiektu $obiekt): ?string
{}
[PHP] pobierz, plaintext 

Napisany przez: Pyton_000 27.09.2018, 07:34:34

Null jest dość specyficzny. Ale taki prototyp funkcji nic w sumie nie mówi. Wszystko zależy od tego jak zaprojektujesz. Często tak, Exception załatwia sprawę, a czasami nie. Więc kontekst jest ważny

Taki prototyp często jest używany w Entity gdzie początkowy stan obiektu zwraca null na wszystkim niemal. Więc o ile coś + null jest w miaaarę powiedzmy ok, o tyle int+string+object+.... już nie...

Napisany przez: athabus 7.10.2018, 14:55:56

Cytat(Pyton_000 @ 25.09.2018, 17:58:11 )