Szanowni profesjonaliści,

Proszę, w wolnej chwili, o przejrzenie kodu i udzielenie cennych wskazówek jak ten kod można ulepszyć.

Kod na Githubie

Plik READ ME.txt - tu jest opis co miało być zaimplementowane.
Instrukcja implementacji interfejsu

No witaj,

Na szybko, co zauważyłem (wieczorem dokładniej się przyjrzę;-)

1. vendor/ do .gitignore
2. wielki + za testy
3. w wielu miejscach brak trzymania się standardów PSR
4. podkreślenia zmiennych prywatnych/protected już nie są w modzie. nie powinno się ich używać.

Architektura też nie powala. Ja bym do tego podszedł inaczej.
Zyjemy w czasach, gdzie podejście DependencyInjection to norma. Dlaczego z tego nie skorzystać.

Zrobiłbym jeden CartManager, który przyjmowałby np. w konstruktorze UserInterface $user. Za pomocą tej oto klasy, dodawałbym, usuwał rzeczy z koszyka:

[PHP] pobierz, plaintext 
* add(CartItemInterface $cartItem)
* remove(CartItemInterface $cartItem)
[PHP] pobierz, plaintext 

Oczywiście metoda zwracająca elementy koszyka, też byłaby wskazana:

[PHP] pobierz, plaintext 
/** @return \CartItemInterface[] $items */
* getCartItems() 
[PHP] pobierz, plaintext 

Zwróć uwagę na komentarz ;-) Dostajemy tablicę CartItemInterface. Co za tym idzie, mamy klasę, która implementuje ten interfejs, posiadająca atrybuty:

[PHP] pobierz, plaintext 
protected ProductInterface $product;
protected int $quantity = 1;
[PHP] pobierz, plaintext 

Typehinty tylko obrazowo.

Metoda Add w cartManagerze sprawdzałaby czy produkt jest już w tablicy, jeżeli tak, to tylko zwiększa quantity. W przeciwnym przypadku, dodaje nowy element do tablicy.

Oczywiście teraz klasa implementująca CartItemInterface miałaby magiczną metodę getCalculatedPrice(), gdzie miałbyś już zaszytą całą logikę wyliczania ceny. Cała klasa mogłaby się nazywać CartItemModifiedPrice, powiedzmy.

M.

- echo w metodzie
- addItem zmien na add i niech zwraca $this; wtedy bedziesz mogl zrobic ->add()->add()
- nie formatuj ceny w metodzie w widok jest tym miejscem.
- getLengthOfString powinno byc w osobnej klasie S z SOLID
- phpdoc (wogole nie wiadomo co mial autor na mysli) np. priceAndDiscounts

Nie polecam. O ile kodu jest mało i wiemy o co chodzi, to jednak trzeba myśleć przyszłościowo. Nie wyobrażam sobie tak mętnej nazwy metody jak "add" w jakiejś większej aplikacji. Zbyt duża abstrakcja i zamieszanie, a addItem mówi samo za siebie. Na rzecz czytleności zostawiłbym addItem - przeczytałem za dużo książek z tematyki "Clean Code", aby coś takiego poczynić. Nie utrudniajmy życia innym.

PS Tak, wiem, to nic wielkiego, ale wszystko zawsze zaczyna się od małego, niewinnego piwa. ; )

Co wcale nie oznacza że w PHP to dobry pomysł. Wręcz bym powiedział że kopiowanie wszystkiego jak leci choćby z Javy już bokiem temu językowi wychodzi. A spróbuj przejąć po kimś większy projekt, jeszcze w dodatku bez komentarzy. Tu jakieś wstrzykiwanie zależności, po drodze zmiana nazw klas, brak komentarzy - brak podpowiadania składni (albo bardzo słabe podpowiadanie) i musisz się wspomagać wyszukiwaniem po plikach całego projektu, jeszcze po drodze jakieś eventy robiące 1000 innych rzeczy. Jest zabawa. Szkoda że taka bezsensowna.

@viking ale namieszałeś. Odnośnie przejmowania większych projektów to masz na myśli Symfony2, Zend Framework 2, Doctrine 2? Czy jeszcze coś większego?
Poruszasz temat komentarzy, ale to nie żadnego związku z DI. O co Ci chodzi ze zmianą nazw klas to nie mam pojęcia. Zdarzenia to temat już zupełnie na osobną dyskusję.

Czyli uważasz, że taki kod:

[PHP] pobierz, plaintext 
class SomeService
{
  /**
   * @ var LoggerInterface 
   */
  private $logger;
 
  public function setLogger(LoggerInterface $logger)
  {
    $this->logger = $logger;
  }
 
  public function execute()
  {
    $this->logger->debug('hello');
  }
}
[PHP] pobierz, plaintext 

jest gorszy od takiego:

[PHP] pobierz, plaintext 
class SomeService
{
  public function execute()
  {
    Logger::getLogger()->debug('hello');
  }
}
[PHP] pobierz, plaintext 

To w takim razie ja również życzę zabawy z takim kodem bez DI. Testowanie pozostawia niezapomniane wrażenia.

PHP jest językiem popularnym (według W3Techs 81% serwerów działa na PeHaPie) i prostym, także mamy w swoim gronie masę amatorów i wanna-be programistów. Jesteśmy skazani na przeróżne "ciekawe" rozwiązania - wystarczy poczytać sobie to forum.

Ale mogę Cię pocieszyć jednym, nie tylko w PHP może zabraknąć komentarzy (brrr, nie lubię). Kiepski kod i "fractal design" to problem tyczący się każdego języka i wszelkiej maści programistów.

Jednak trzeba przyznać, że w PHP z racji popularności tego języka, częściej można spotkać się ze słabym kodem. Jednak pamiętać należy, że mamy w swoim gronie doświadczonych developerów, którzy wybrali ten język jako jedno ze swoich narzędzi.

Obyśmy z ich kodem obcowali i uczyli się od nich. Ejmen.

PS Chcesz zmienić świat na lepsze? Zacznij od siebie!
PS DI jest okay, stoją za tym mocne walory.

@destroyerr To co pokazałeś to zwykłe rzutowanie typów i jakaś metoda statyczna więc nie wiem co tam jest lepsze gorsze. Ja mam na myśli odnoszenie się do kodu poprzez stringi czyli np

[PHP] pobierz, plaintext 
$di->get('Db\Adapter\Oracle');
[PHP] pobierz, plaintext 

Odpada ci cała zaleta podpowiadania składni bo nazwę musisz znać i jeszcze ją całą ręcznie wklepać. Ktoś teraz załóżmy miał wizję i zrobił coś takiego:

[PHP] pobierz, plaintext 
$im = $di->instanceManager();
$im->addAlias('trolololo', 'Db\Adapter\Oracle');
[PHP] pobierz, plaintext 

I skąd ty do siedmiu piekieł masz wiedzieć że jakieś trolololo używane wszędzie w kodzie to adapter Oracle? Albo znasz super aplikację (która załóżmy była wcześniej rozwijana przez kogoś dwa lata), albo masz mega dokumentację. Jak żadnego to praktycznie leżysz i sobie humor psujesz. Częste używanie słów na K słabo działa na ogólne samopoczucie. Druga sprawa że jeśli ktoś był leniwy i nie chciało mu się dodawać:

[PHP] pobierz, plaintext 
/* @var $db Db\Adapter\Oracle */
[PHP] pobierz, plaintext 

to masz jeszcze bardziej przechlapane.
Kolejną sprawą jest że praktycznie nie ma dobrego sposobu na dumpa takich kontenerów bo dostajesz całą masą rekurencji. DI jest też zazwyczaj potwornie wolne w działaniu.

PHP jako językowi brakuje podstaw do takich zabaw. To nie Java. Nie ma choćby adnotacji i od dłuższego czasu wszyscy się głowią czy je wprowadzić czy dalej udawać że te śmieszne, parsowane dodatkowo komentarze (co daje kolejny narzut i jeszcze wolniejsze wykonywanie) zostawić jak są czy w końcu coś z tym zrobić. IoC w takiej postaci jak teraz to zwyczajne "widzimisię". Inni mają to my też się pobawimy.

To właśnie mój kawałek kodu pokazuje czym jest DI. Kontener zależności z Symfony2 pokazuje, że da się zrobić dumpa bez rekurencji. Czyli masz problem z implementacją.

Pokazałeś w przykładowym kodzie implementacje kontenera zależności w wydaniu Zend Framework 2 (o ile się nie mylę), ale nie pokazałeś Dependency Injection. Akurat kontenery czy też manedżery to już nie jest część wzorca DI. Toteż DI nie jest dużo wolniejsze bo aż tak dużej różnicy w czasie wykonania nie ma, czy wstrzykniesz zależność czy pobierzesz ją wewnątrz obiektu.

Nie musisz korzystać z aliasów, zwłaszcza, że są tak idiotyczne i nic nie mówią.

To czy ktoś dodaje komentarze phpdoc to nie ma związku z DI, a to właśnie ten wzorzec w PHP negujesz.

Pokaż jak testujesz obiekty z zależnościami nie korzystając z DI.

@viking: Trochę Cię zmartwię. To o czym piszesz to nie jest DI. Cały Twój wywód jest nie na temat. W przykładzie podanym przez @destroyerr'a nie ma też żadnych rzutowań, a jeżeli nie znasz powodów dla których pierwszy przykład (z wykorzystaniem DI) jest lepszy od drugiego (z bezpośrednim, "twardym" odwołaniem do innej klasy z wewnątrz innej), to wybacz ale jednak nie powinieneś się chyba tutaj wypowiadać - możesz niepotrzebnie wprowadzić innych w błąd.

To co opisujesz to rejest/service locator/service container i jego bezpośrednie używanie w aplikacji jest raczej złym pomysłem - niweczy wszystkie zalety DI.

To o czym piszę jest dokładnie implementacja DI we wszystkich frameworkach. Co więcej, nawet skopiowalem ten przykład z dokumentacji di zf2. Destroyerr podal akademicki goły przykład. Ja natomiast antywzorzec di który w tym wypadku służy jako service locator ale jest całkowicie akceptowalnym wariantem. Źle użyty narobi tylko problemów w aplikacji. Czyli rozumiem ze to nie jest type hinting, przetwarzany następnie przez reflection do odczytania klasy? Czym to w takim razie jest? I weź pod uwagę jeszcze raz. Mówię o faktycznych implementacjach we fw nie akademickich regułach.

Ale Ty piszesz o implementacji kontenera/menedżera zależności, a to nie jest to samo co DI. DI opisał Fowler i to jest jasno ograniczone. To nie akademickie reguły, tylko to Ty próbujesz do tego wzorca przypisać coś co nie jest jego częścią. Zf2 z tego co się orientuję (może błędnie) korzysta głównie z service locatora, a to nie jest DI, tylko jeden ze sposobów wykonania IoC.

Zdaję sobie sprawę że przydałby mi się urlop i moja zdolność tłumaczenia innym kodu drastycznie ostatnimi czasy spadła, liczyłem jednak że mnie zrozumiecie. Po pierwsze, PHP samo z siebie nie odczyta w Twoim kodzie zależności tylko potrzebujesz dołączyć kod jakiejś biblioteki DI (stąd mówię o implementacjach i akademickich teoriach, to samo tyczy artykułu Fowlera na który się powołujesz). Może jeszcze dodam że wszystkie porównania w pierwszym poście robiłem do Springa. Po drugie, czy widzisz różnicę między swoim kodem:

[PHP] pobierz, plaintext 
 /**
  * @ var LoggerInterface
  */
private $logger;
 
public function setLogger(LoggerInterface $logger)
{
$this->logger = $logger;
}
[PHP] pobierz, plaintext 

a

[PHP] pobierz, plaintext 
/**
   * @Inject
   * @var LoggerInterface
   */
protected $logger;
 
/**
   * @Inject
   */ 
public function setLogger(LoggerInterface $logger)
{
}
[PHP] pobierz, plaintext 

Czy teraz rozumiesz co miałem na myśli pisząc o "zwykłym rzutowaniu typów"? Po co był cały mój wywód o komentarzach i braku adnotacji w PHP jako języku (cały czas mając na uwadze IoC Springa, konfigurację beansów, @Autowired)? I nie próbujcie mi wmawiać że mieszam tutaj DI z SM/SL bo nie robię tego tylko pokazałem użycie implementacji DI jako antywzorca. Jeśli nie rozumiecie różnicy to nawet udało się znaleźć linka: http://php-di.org/doc/getting-started.html (sekcja Get objects from the container). I tutaj Dejmien_85 bardzo dobrze zauważył że jak programista jest słaby to stosowanie IoC narobi bardzo dużo szkody. I na koniec. Kto używa ZF2 i przypomni w ramach dyskusji dlaczego powstaje ZF3 i co było źle? ;-) (w szczególności to, że programiści "źle zrozumieli koncepcję")

@viking: Jeszcze raz, wyjaśnijmy sobie proszę bardzo fundamentalne deklaracje. DI to po prostu nazwa dla pewnego wzorca/schematu kodu - nic więcej. Niejednokrotnie wykorzystuje się go nawet nieświadomie. To co cały czas tutaj opisujesz to różnego rodzaju kontenery/menadżery zależności - to już zupełnie inna kwestia (fakt, że w nazwie mają dependency injection (np. CDI) nie oznacza, że tym są). Te już występują w przeróżnych wariantach, z różnymi opcjami konf. bazującymi na różnych podejściach do samej konf. zależności. Często można je spotkać pod nazwą dependency injection container, ale DI ≠ DIC - i to jest najważniejsze w moim i @destroyerr'a postach.

Te kontenery w swoim wnętrzu bardzo często faktycznie będą obsługiwały coś przy pomocy takiej właśnie tablicy (string => obiekt), ale to jest na ich wewnętrzny użytek. Jako użytkownik takiego kontenera nie powinieneś w ogóle z tego korzystać. Tutaj niestety pojawia się taki problem, że wiele FW wręcz zachęca w swoich dokumentacjach do korzystania z tego, stąd też ich użytkownicy to robią. Co do Java'owych @Inject (z CDI) czy @Autowired (ze Springa)
1. To już są narzędzia konkretnych realizacji kontenerów zależności i należą do ich wewnętrznej konfiguracji. Same w sobie nie mają bezpośredniego związku z tematem wstrzykiwania zależności. To dopiero sam kontener realizuje to zadanie.
2. Te adnotacje są wynikiem wprowadzania do Javy (w sumie do PHP też) coraz szerszego podejścia convention over configuration oraz chęci skrócenia kodu/plików konfiguracyjnych. Przy czym warto tutaj zaznaczyć, że dla np. takiego Springa nadal jest to tylko jeden ze sposobów na określenie "co wstrzyknąć", a nie jego podstawowy/jedyny sposób.
3. W PHP istnieją podobne narzędzia, np. w Symfony mamy dostęp do @Inject (JMSDiExtraBundle) przy czym ich działanie jest jednak nadal trochę odmienne - nie potrafi pracować na interfejsach i zawsze musi mieć podany konkretny serwis do wstrzyknięcia.

Na koniec chciałem tylko podkreślić jedną rzecz. Zarówno ja jak i @destroyerr (myślę, że mogę go tutaj wymieć ;] ) również uważamy że zapis typu:

[PHP] pobierz, plaintext 
$mailer = $this->someContainer->get('my.mailer.service');
$mailer->doSth();
[PHP] pobierz, plaintext 

Jest zły, niweczy większość, jak nie wszystkie z zalet DI, utrudnia testowanie aplikacji oraz może sprawiać jakieś problemy przy przejmowaniu po kimś kodu. Chcieliśmy tylko zwrócić Ci uwagę na to że pow. przykład kodu nie jest przykładem realizacji dependency injection.

W porządku. Jeśli potraktować sprawę w ten sposób czyli przedstawienie samej idei wzorca to ok. Jednak idei nie wprowadzimy w aplikacji dlatego zacząłem pisać o implementacji i zarazem kontenerach. Po czym podałem często spotykany problem nadużywania DI jako SL i napisałem że przejęcie po kimś takiego kodu to stracone godziny na przeszukiwanie projektu i generalnie makabra. A biorąc pod uwagę jak osobiście często widzę takie kwiatki, możemy chyba wspólnie przyjąć że stosowanie IoC wymaga wiedzy i uwagi.

Właśnie cały ten nasz wywód był w celu odróżnienia implementacji DI od DIC-ów. Przykład @destroyerra:

[PHP] pobierz, plaintext 
class SomeService
{
  /**
   * @ var LoggerInterface 
   */
  private $logger;
 
  public function setLogger(LoggerInterface $logger)
  {
    $this->logger = $logger;
  }
 
  public function execute()
  {
    $this->logger->debug('hello');
  }
}
[PHP] pobierz, plaintext 

Jest już kompletną*, nieakademicką implementacją IoC w formie DI (konkretnie zrealizowaną przez setter injection). I tego typu konstrukcje spotkamy w większości bibliotek i aplikacji - tych napisanych z zachowaniem minimum standardów jakości. Nie potrzebujemy tutaj żadnych kontenerów zależności by z tego kodu korzystać z pełnym wykorzystaniem zalet obiektówki i DI.

* tak tylko w ramach formalności, należałoby tutaj zauważyć, że logger powinien zostać przekazany w konstruktorze, albo w metodzie execute musielibyśmy sprawdzić czy aby na pewno pod $this->logger jest jakiś obiekt, a nie NULL. Ale to już w tym przykładzie jest nieistotne.

Corzin, piszesz, że zapis typu:

[PHP] pobierz, plaintext 
$mailer = $this->someContainer->get('my.mailer.service');
$mailer->doSth();
[PHP] pobierz, plaintext 

jest zły. Mogłbyś jakoś rozwinąć ten temat? W podobny sposób używa się większości kontenerów IoC... przynajmniej w framewrokach PHP. W jaki sposób taki zapis "niweczy zalety DI" i jakie lepsze rozwiązanie byś proponował?
Pytam serio, to nie jest żadne ironiczne pytanie

Trochę większy przykład:

[PHP] pobierz, plaintext 
class MyService {
  protected $container;
 
  public function __construct($container) {
    $htis->container = $container;
  }
 
  public function doSth() {
    ...
    $mailer = $this->container->get('my.mailer.service');
    $mailer->doSth();
  }
 
  public function doSthElse() {
    ...
    $repo = $this->container->get('my.repo');
    $obj = $repo->create(..);
 
    ...
 
    $repo->save($obj);
  }
}
[PHP] pobierz, plaintext 

1. Tutaj na dobrą sprawę w ogóle nie mamy właściwie DI. Co prawda sam kontener zależności jest przekazywany w sposób typowy dla DI, ale nie on nas tutaj interesuje.
2. Jakie zależności ma obiekt typu MyService? Nie wiadomo.
3. Czy da się napisać testy dla czegoś takiego? Da. Ale w strasznie niewygodny sposób.
4. Czy utworzę dwa obiekty klasy MyService korzystające z dwóch różnych mailerów? Jak się odpowiednio wygimnastykuję to może tak. Zamiast prostego:

[PHP] pobierz, plaintext 
$realService = new MyService($mailer, $repo);
$dummyService = new MyService(new DummyMailer(), $repo);
[PHP] pobierz, plaintext 

Muszę robić jakieś cuda w postaci klonowania całego kontenera, ustawiania mu jednego innego serwisu i przekazywania dopiero tego do MyService. A i tak cholera wie czy to w ogóle dalej będzie działało.

To tak na szybko...

Raczej zawsze odradza się bezpośrednie korzystanie z kontenera. Jedynie wewnątrz kontrolerów jest to nagminnie "promowane". Ale jest to jeszcze raczej akceptowane ze względu na to, że kontrolery są pod tym względem na tyle specyficzne, że często jest to zwyczajnie w świecie bardziej opłacalne. Pamiętajmy, że ostatecznie te techniki mają nam pomóc, a nie być sztuką dla sztuki.

Właśnie, takie użycie kontenera rozpatrywałem w kontekście kontrolera, stąd się zastanawiałem dlaczego to jest źle. Rzeczywiście, w przypadku jakiegoś modułu wpychanie całego kontenera do klasy nie ma sensu, dobrze, że na to zwróciłeś uwagę. Dzięki

To jest oczywiste dla każdego, kto myśli
To tak jakby powiedzieć ,,do działania klasy X potrzebuję kontenera DI". Guzik z pętęlką, wcale nie, Twoja klasa X potrzebuje innej klasy Y.

Co do oceny, dodałbym jeszcze: nie używaj dziedziczenia, zapoznaj się z kompozycją ponad dziedziczenie.