Jaki najlepszy wzorzec projektowy uzyc pod w rozwiazaniu zadanie testowego?

Witam,

Niedawno dostalem taki test do rozwiazania: https://github.com/CMProductions/backend-test

Nie ma co ukrywac, ze dosc prosty test. Jednak jak to w programowaniu mozna go napisac na 99999 roznych sposobow.

Napiszcie jak Wy byscie to rozwiazali? Chodzi mi o samo uzycie wzorcow, rozlozenie klas oraz folderow. Interesuje mnie wylacznie sama architektura.

Tutaj moj kod: https://bitbucket.org/poniat/videos_downloader/

Chciałbym, zeby ktoś ocenił go :)

Muszę Ci pogratulować - moje wrażenia:

• na pierwszy rzut oka: matko boska, ale przeinżynierowana aplikacja.
• na drugi rzut oka: matko boska, ale przeinżynierowana aplikacja :-)

Idąc po kolei:

1. W jakim celu stworzyłeś te wszystkie interfejsy do serwisów oraz modeli? Co one wnoszą?

2. Wszystkie Twoje testy są zbędne:
   2a. https://bitbucket.org/poniat/videos_downloader/src/e282fe06d3684eefdeef4c5b6b33617d3f9d7c80/tests/JsonTest.php?at=master&fileviewer=file-view-default - skoro metoda parse() ma type-hint na tablicę, ten test zawsze będzie prawdziwy.
   2b. https://bitbucket.org/poniat/videos_downloader/src/e282fe06d3684eefdeef4c5b6b33617d3f9d7c80/tests/VideoDownloaderTest.php?at=master&fileviewer=file-view-default - ten test jest pusty.
   2c. https://bitbucket.org/poniat/videos_downloader/src/e282fe06d3684eefdeef4c5b6b33617d3f9d7c80/tests/VideoParserTest.php?at=master&fileviewer=file-view-default - podobnie jak 2a. Dodatkowo testFailure() jedyne co sprawdza, to najwyraźniej czy PHP rozumie dziedziczenie klas - nie testujesz w nim żadnej Twojej logiki, żadnego Twojego zachowania.
   2d. https://bitbucket.org/poniat/videos_downloader/src/e282fe06d3684eefdeef4c5b6b33617d3f9d7c80/tests/YamlTest.php?at=master&fileviewer=file-view-default - identycznie jak 2a.

3. https://bitbucket.org/poniat/videos_downloader/src/e282fe06d3684eefdeef4c5b6b33617d3f9d7c80/src/VideoParser.php?at=master&fileviewer=file-view-default
   3a. An extension is null giving! - nie wiem w jakim języku napisałeś ten komunikat, lecz na pewno nie po angielsku.
   3b. Skoro konstruktor nie może przyjmować nulla (bo od razu rzuca wyjątek), dlaczego w ogóle mu na to pozwalasz? Gdybyś napisał public function __construct(string $extension) to wcale nie musiałbyś niczego sprawdzać na własną rękę, ponieważ zająłby się tym interpreter. Powtarza się to również w wielu innych miejscach.
   3c. Po throw / return nie musisz dorzucać break.

4. https://bitbucket.org/poniat/videos_downloader/src/e282fe06d3684eefdeef4c5b6b33617d3f9d7c80/src/VideoDownloader.php?at=master&fileviewer=file-view-default
   4a. Przeinżynierowałeś z tym CliHelper::isCli() - gdybyś podszedł do tego w taki sposób, każdy mógłby dostosować sposób logowania do swoich potrzeb.
   4b. Dlaczego masz continue; zamiast rzucania wyjątku?

5. https://bitbucket.org/poniat/videos_downloader/src/e282fe06d3684eefdeef4c5b6b33617d3f9d7c80/src/Service/Model/VideoCassandra.php?at=master&fileviewer=file-view-default
   5a. Dlaczego wszystkie pola są publiczne?
   5b. Dlaczego wszystkie pola mają domyślną wartość ''?
   5c. Dlaczego cała ta klasa jest kopiuj-wklejką klasy Video, która jest kopiuj-wklejką klasy VideoModel?

6. https://bitbucket.org/poniat/videos_downloader/src/e282fe06d3684eefdeef4c5b6b33617d3f9d7c80/src/Service/Reader/Reader.php?at=master&fileviewer=file-view-default
   6a. Dlaczego return [];?
   6b. W jakim celu ta klasa w ogóle istnieje? Co jest nie tak z samym file_get_contents(), że musiałeś go przykryć tą fasadą? A jeśli już tak bardzo chciałeś mieć abstrakcję, dlaczego wymyśliłeś własną, a nie wykorzystałeś np. Filesystem z Symfony?

7. https://bitbucket.org/poniat/videos_downloader/src/e282fe06d3684eefdeef4c5b6b33617d3f9d7c80/src/Service/Traits/CliHelper.php?at=master&fileviewer=file-view-default#CliHelper.php - dlaczego to jest trait?

8. https://bitbucket.org/poniat/videos_downloader/src/e282fe06d3684eefdeef4c5b6b33617d3f9d7c80/src/Service/Downloader/VideoDownloader.php?at=master&fileviewer=file-view-default#VideoDownloader.php-18 - ten if jest zbędny.

9. https://bitbucket.org/poniat/videos_downloader/src/e282fe06d3684eefdeef4c5b6b33617d3f9d7c80/src/Service/Parser/Parser.php?at=master&fileviewer=file-view-default#Parser.php-8 - ayyy dlaczego ta ścieżka jest na sztywno?

Jak sam zauważyłeś Nie ma co ukrywac, ze dosc prosty test. - szkoda, że Twoja aplikacja tego w żaden sposób nie odwzorowuje i zawiera fasadę na fasadzie, interfejs na interfejsie oraz abstrakcję na abstrakcji.

@Patryk27:

1. Najwazniejszy dla mnie interfejs jest przy modelach. Ponieważ w teście jest napisane, że chca przełączyć MySQL do Casandra. Wiec zdecydowałem się na Dependency Injections.
2. Testy usunąłem, one nie były skończone.
3. Poprawione.
4. Zdecydowałem się na trait, ponieważ mogę użyć tej metody wszedzie, gdzie chcę.
5. Nie mogłem się zdecydować czy dać pola public i używać po prostu $video->title = 'Example'. Czy dać wszystkie pola prywatne i używać metod set, get $video->setTitle('Example'). Pierwsza opcja jest szybsza i mniej kodu.
6. Nie rozumiem co to fasada i abstrakcja?? To była odpowiedz od firmy: Missing the separation between parser and reader. Na początku miałem parser i reader w jednym pliku, ale ze wzgledu na ten komentarz oddzielilem.

Dostosowałem się do Twoich komentarzy za ktore dziekuje i zaktualizaowałem kod. Czy możesz mi podpowiedzić jak mam napisać ten projekty ze względu na architekture? Czy użyłbyś jakiegoś konkretnego wzorca projektowego?

poniatowski napisał(a):

1. Najwazniejszy dla mnie interfejs jest przy modelach. Ponieważ w teście jest napisane, że chca przełączyć MySQL do Casandra. Wiec zdecydowałem się na Dependency Injections.

Prawidłowo napisany kod powinien mieć oddzielony driver od modeli więc przełączasz sam driver a modele zostają nietknięte, tak masz np. w każdym normalnym frameworku zrobione.

@mr_jaro: Zgadzam się. Tylko jak zinterpretujesz to:
Do not implement any data persistence code, just provide some dummy classes that echo what they are doing. Keep in mind that the company is planning to switch from MySQL to Cassandra in 4 months.?

ciężko, ale ja bym po prostu wtedy zostawił puste modele.

@mr_jaro: Rozwiń myśl? Nie kumam co bys zrobił?

Chyba, że źle rozumiem data persistence code

1. Skoro masz katalog Interface i Trait, dlaczego nie masz katalogu Class? To ironia, te 2 katalogi są błędne moim zdaniem.
2. Nie używaj słowa helper, jak masz helper to znaczy, że coś robisz źle i z czasem helper będzie sterował całym systemem (bo helperem może być wszystko).
3. Twórz obiekty przez wstrzykiwanie danych w konstruktorach a nie przez setery, potem samo tworzenie z tym clone jest dziwne (https://bitbucket.org/poniat/videos_downloader/src/af3a45aa8e99dc5d4335c81f3984e61cf0a7bb6f/src/Service/Parser/YamlParser.php?at=master&fileviewer=file-view-default#YamlParser.php-23), wystarczyło by new Video($url, $name, $tags).
4. Konstruktor w tej klasie https://bitbucket.org/poniat/videos_downloader/src/af3a45aa8e99dc5d4335c81f3984e61cf0a7bb6f/src/Service/Parser/VideoParser.php?at=master&fileviewer=file-view-default mówi mi, że jak ktoś będzie chciał dodać nowy parser to nie wystarczy wstrzyknać dodatkowej klasy, muszę jeszcze przerabiać właściwą klasę. Zamiast wstrzykiwać tutaj string z nazwą wstrzyknij po prostu odpowiednią klasę.
5. Dlaczego wszystko jest w katalogu Service?
6. Gdzie są testy?
7. Po co ta klasa? https://bitbucket.org/poniat/videos_downloader/src/af3a45aa8e99dc5d4335c81f3984e61cf0a7bb6f/src/Service/Reader/Reader.php?at=master&fileviewer=file-view-default
8. Czym się różni klasa VideoCassandra od Video?

@Markuz:

1. To jak miałem nazwać te dwa foldery?
2. Zgadzam się.
3. Przecież model implementuje interface i jest wstrzykiwany w konstruktorze. Niedokonca rozumiem Twojej mysli.
4. Dosc słuszna uwaga. Za bardzo się skupilem na wzorcu projektowym strategy.
5. Katalog jest do usuniecia.
6. Testy napisze jak skoncze projekt.
7. Klasa jest do tego, zeby oddzielic logike reader od parser.
8. W tescie jest napisane, ze beda przełączas Mysql do Casandra.

Dzieki za komentarz.

Najwazniejszy dla mnie interfejs jest przy modelach. Ponieważ w teście jest napisane, że chca przełączyć MySQL do Casandra. Wiec zdecydowałem się na Dependency Injections.

Encje nie powinny być w ogóle zależne od bazy danych - mylisz dziedziny; powinieneś mieć jedną (wspólną) encję, ale wiele data persisterów (wiele klas potrafiących dany model zapisać) - na przykład w taki sposób.

Rule of thumb: interfejs powinien opisywać zachowanie. Encje zachowania nie powinny mieć (są tylko workiem na dane), ergo: nie ma sensu tworzyć dla nich interfejsów.

Zdecydowałem się na trait, ponieważ mogę użyć tej metody wszedzie, gdzie chcę.

Jaką przewagę ma trait nad statyczną metodą w zamkniętej klasie / funkcją?

Pierwsza opcja jest szybsza i mniej kodu.

I przy okazji łamie hermetyzację; dzięki temu nikt nie broni mi napisać $video->title = ['ciekawe', 'co', 'się', 'stanie', 'teraz']; albo $video->title = $video;, i happy debugging later.

Nie rozumiem co to fasada i abstrakcja?

Stworzyłeś masę klas (takich jak Reader) oraz interfejsów (takich jak VideoModelInterface), których istnienie nie jest w żaden sposób uzasadnione; do zasadniczo prostych założeń zaprojektowałeś aplikację, która jest zwyczajnie ogromna (biorąc pod uwagę oczekiwaną skalę).

Jaki najlepszy wzorzec projektowy uzyc pod w rozwiazaniu zadanie testowego?

Ja bym w ogóle nie dała Twojego kodu do sprawdzenia przez kogoś z programistów, bo osoby, które bardzo słabo posługują się językiem mówionym/pisanym zazwyczaj piszą też bardzo słabe programy i to się zawsze sprawdza.
Z ciekawości poprosiłam jednak kolegę by poświęcił 5-10 minut i powiedział, że na pierwszy rzut oka nie ma czego tu poprawiać i to się nadaje niestety do kosza, bo wszędzie widać brak podstaw.
Nawet od najmniej doświadczonych kandydatów nie oczekuje się działającej aplikacji czy dobrego kodu, ale predyspozycji i odpowiedniego sposobu myślenia - u Ciebie tego nie widać niestety.
Ja, jako HR bym radziła coś innego, programista PHP zarabia średnio 7-8k brutto na UoP i jest to najsłabiej opłacana technologia. Mam oferty na magazyn, praca od zaraz, prawie 5k brutto UoP plus pakiet socjalny, posiłki za złotówkę czy dojazd do pracy - oczywiście mowa o 3mieście. Po co uczyć się na PHP, gdzie konkurencja jest duża a płacą tyle, co doświadczonemu elektrykowi czy glazurnikowi? W dodatku na firmy szukające do PHP mam najwięcej skarg.

Patryk27 napisał(a):

I przy okazji łamie hermetyzację; dzięki temu nikt nie broni mi napisać $video->title = ['ciekawe', 'co', 'się', 'stanie', 'teraz']; albo $video->title = $video;, i happy debugging later.

Czyli setter bez type hint'a jest tak samo useless jak publiczne pole? ;D bo też można zrobić $video->setTitle($video);?

TomRiddle napisał(a):

Patryk27 napisał(a):

I przy okazji łamie hermetyzację; dzięki temu nikt nie broni mi napisać $video->title = ['ciekawe', 'co', 'się', 'stanie', 'teraz']; albo $video->title = $video;, i happy debugging later.

Czyli setter bez type hint'a jest tak samo useless jak publiczne pole? ;D bo też można zrobić $video->setTitle($video);?

Nie, nie jest. Setter jest lepszy, bo masz typowanie dla argumentu i wyjścia, a dodatkowo wewnątrz możesz dodać np. jakąś mini-walidację. Póki co, bo jak wyjdzie php 7.4, to będzie typowanie dla pól, to wtedy używanie bezpośrednio publicznych pól będzie bezpieczniejsze niż jest teraz. Ale settery i tak nadal będą mieć więcej możliwości.

serek napisał(a):

TomRiddle napisał(a):

Patryk27 napisał(a):

I przy okazji łamie hermetyzację; dzięki temu nikt nie broni mi napisać $video->title = ['ciekawe', 'co', 'się', 'stanie', 'teraz']; albo $video->title = $video;, i happy debugging later.

Czyli setter bez type hint'a jest tak samo useless jak publiczne pole? ;D bo też można zrobić $video->setTitle($video);?

Nie, nie jest. Setter jest lepszy, bo masz typowanie dla argumentu i wyjścia,

Czyli... type hint o którym pisałem w poście który zacytowałeś? :D

a dodatkowo wewnątrz możesz dodać np. jakąś mini-walidację.

A ile widziałeś setterów które mają min-iwalidację? Bo ja 0 (zwłaszcza że w innych językach są wręcz generatory takich setterów, np lombok w javie).

Póki co, bo jak wyjdzie php 7.4, to będzie typowanie dla pól, to wtedy używanie bezpośrednio publicznych pól będzie bezpieczniejsze niż jest teraz. Ale settery i tak nadal będą mieć więcej możliwości.

A jakie?

Bo ja widzę tylko taką zaletę, jeśli taki setter/getter jest częścią jakiegoś interfejsu który klasa implementuje, wtedy implementacja może się zmieniać - ale jeśli nie? To w sumie jaka jest różnica między setterem/getterem? Oprócz tego że nie można użyć isSet() albo ++ na setterze?

poniatowski napisał(a):

Tutaj moj kod: https://bitbucket.org/poniat/videos_downloader/

Coś Javowy ten kod. Dobrze, że w porę się ewakuowałem z PHP, bo teraz to czasem ciężko odróżnić kod PHP od kodu jaki piszą Javowcy czy C#owcy (chodzi mi o zwyczaje/specyficzny, przeinżynierowany styl pisania i organizacji kodu, a nie o składnię)

Jak dla mnie to klasa VideoDownloader to jest zupełną kompromitacją (https://bitbucket.org/poniat/videos_downloader/src/af3a45aa8e99dc5d4335c81f3984e61cf0a7bb6f/src/Service/Downloader/VideoDownloader.php?at=master&fileviewer=file-view-default)

• Folder z jedną klasą
• Jakaś zmienna nl - niby domyślam się sie że to znaczy "new line"? Chyba? Ale i tak biedna nazwa
• Ale największy fackup to to że ta klasa po pierwsze:
  • umie wypisać coś na standard output
  • wie czy jest uruchomiona w cli lub nie
  • sama decyduje o formacie wyświetlanych danych
  • i jeszcze wie o html'u???!!!
  • i nawet jak wie, to i tak to biednie rozwiązuje bo sam <br> to jest średnie rozwiązanie, i lepiej byłoby np użyć white-space w CSS'sie

poniatowski napisał(a):

@Markuz:

1. To jak miałem nazwać te dwa foldery?
2. Zgadzam się.

ParserInterface przenosisz do katalogu Parser, VideoModelInterface do katalogu Model, a CliHelper usuwasz.

3. Przecież model implementuje interface i jest wstrzykiwany w konstruktorze. Niedokonca rozumiem Twojej mysli.

Porównaj proszę te 2 fragmenty kodu i daj znać który wydaje Ci się mniej skomplikowany:

            $videoObj        = clone $video;
            $videoObj->url   = $row['url'];
            $videoObj->title = $row['name'];

            if(array_key_exists('labels', $row) && $row['labels'] !== null) {
                $videoObj->tags = $row['labels'];
            }

            $results[] = $videoObj;

$tags = [];
if(array_key_exists('labels', $row) && $row['labels'] !== null) {
    $tags = $row['labels'];
}
$results[] = new Video($row['url'], $row['name'], $tags);

4. Dosc słuszna uwaga. Za bardzo się skupilem na wzorcu projektowym strategy.
5. Katalog jest do usuniecia.
6. Testy napisze jak skoncze projekt.
7. Klasa jest do tego, zeby oddzielic logike reader od parser.
8. W tescie jest napisane, ze beda przełączas Mysql do Casandra.

Dzieki za komentarz.

7. Ale ta klasa nie ma żadnej logiki poza stworzeniem aliasu dla funkcji file_get_contents.
8. Ale będziesz przelączał samą nazwę klasy? Przecież jej zachowanie jest dokładnie takie samo. Teraz wyobraź sobie, że chcemy aby nasz system wspierał MySQL, Casandra i jeszcze Redisa - co musisz zrobić? Ctrl+C Ctrl+V + zmiana nazwy klasy na VideoRedis, a teraz wyobraź sobie, że masz 10 takich modeli, co robisz? Ctrl+C Ctrl+V x 10 + zmiana nazwy klasy x 10. A teraz wyobraź sobie, że w każdym z tych modeli dojdzie nowe pole np. createdAt - co robisz? Otwierasz 30 plików, w każdym wstawiasz dokładnie ten sam fragment kodu tj. setCreatedAt, getCreatedAt itd.

TomRiddle napisał(a):

Patryk27 napisał(a):

I przy okazji łamie hermetyzację; dzięki temu nikt nie broni mi napisać $video->title = ['ciekawe', 'co', 'się', 'stanie', 'teraz']; albo $video->title = $video;, i happy debugging later.

Czyli setter bez type hint'a jest tak samo useless jak publiczne pole? ;D bo też można zrobić $video->setTitle($video);?

Tak - mutator niesprawdzający w żaden tego, co przyjmuje, jest tak samo bezużyteczny jak publiczne pole.

Z drugiej strony tęczy, z dala od garńca złota, mamy drugą skrajność, która polega na wrzucaniu pełnej walidacji do setterów (np. setEmail() sprawdzająca format maila, rekordy DNS oraz łącząca się z serwerem w celu potwierdzenia istnienia serwera pocztowego po drugiej stronie).

Wydaje mi się, że w przypadku mutatorów najlepiej jest ograniczyć walidację do sprawdzania poprawności typów oraz wartości enumów - powinno to ogarnąć większość błędów logicznych w kodzie (tzn. tych niezwiązanych bezpośrednio z danymi wprowadzonymi przez użytkownika). Tym samym w językach o nieco bogatszym systemie typów takie checki są całkowicie nadmiarowe (stąd Lombok).