ugrás a tartalomhoz

Clean code: trainwreck

Lencse · 2014. Szep. 9. (K), 16.51
Sziasztok! Szerintem nem szóltam még itt hozzá. :)

A clean code elvek közül sokáig nehezen tudtam a megérteni a metódusláncok elkerülésére vonatkozót, nem volt világos, miért kell követni, és az sem, hogy pontosan hogyan kerüljük el.
Most azt hiszem, találtam egy aránylag egyszerű példát, ami kulcsot jelenthet a megértéshez.

Van egy Mailer osztály, ennek kell szolgáltatásokat nyújtani az emailek kezelésére. Alapvetően egy emaillel kétféle dolgot tehetünk: kiküldhetjük azonnal, vagy a várakozási sorhoz adhatjuk, ezeket aztán egyszerre küldjük ki.
Itt az implementációm:
  1. class Mailer {  
  2.     //...  
  3.       
  4.     /** 
  5.      * @var MailQueue 
  6.      */  
  7.     private $mailQueue;  
  8.       
  9.     /** 
  10.      * @var MailTransport 
  11.      */  
  12.     private $mailTransport;  
  13.   
  14.     /** 
  15.      * @param Mail $mail 
  16.      */  
  17.     public function send(Mail $mail) {  
  18.         $this->mailTransport->send($mail);  
  19.     }  
  20.   
  21.     /** 
  22.      * @return MailQueue 
  23.      */  
  24.     public function getMailQueue() {  
  25.         return $this->mailQueue;  
  26.     }  
  27.   
  28. }  
  29.   
  30. class MailQueue {  
  31.     //...  
  32.       
  33.     /** 
  34.      * @var MailTransport 
  35.      */  
  36.     private $mailTransport;  
  37.       
  38.     /** 
  39.      * @param Mail $mail 
  40.      */  
  41.     public function addMail(Mail $mail) {  
  42.         //...  
  43.     }  
  44.       
  45.     public function sendMails() {  
  46.         //...  
  47.         foreach($mails as $m) {  
  48.             this->mailTransport->send($m);  
  49.             //...  
  50.         }  
  51.     }  
  52.       
  53. }  
Használni pedig így lehet:
  1. $mailer->send($mail);  
  2.   
  3. // vagy:  
  4.   
  5. $mailer->getMailQueue()->addMail($mail1);  
  6. $mailer->getMailQueue()->addMail($mail2);  
  7. $mailer->getMailQueue()->sendMails();  
És igen, valahol itt lent, a mail queue metódusoknál jelentkezik a vonatroncs. Ha jól értem, ennek elkerülésére burkoló metódusokat kéne írnom a Mailer osztályba (addMailToQueue és sendMailsFromQueue), de egyelőre nem teljesen világos, hogy ennek mi a konkrét haszna, és miért kell elkerülni a method chaininget.

Jól gondolkodom? Nektek mi a véleményetek erről?

Válaszokat előre is köszi!
‹ Felhasználó által beállítható automatizmusok könyvtár/file átnevezés speciális karakter esetén ›
 ■
1

Hali, Method

Práger Ádám · 2014. Szep. 9. (K), 18.17
Hali,

Method chaining

Nagyon sok helyen olvasni hogy bad practice, de ez egyáltalán nem igaz. Nagyon is valid, de ennek is megvan mikorja.

A probléma innen ered (gondolom erről olvastál te is):
  1. $a->getB()->getC();  
Na most ha getB() nem a megfelelő objektummal tér vissza (ha minden oké a kóddal akkor leginkább nullról van szó), akkor baj van.

Ilyen eseteken kerülendő a chaining, az alábbi viszont teljesen rendben van:
  1. $a->setName('name')->setSomething('sth');  
Természetesen a setterek mindig $this-t adnak vissza, nem lehet olyan probléma mint az első példánál.

Általános jó tanács az, hogy ahol más osztályt ad vissza, ott nem ajánlott a chain.

@see Doctrine2 QueryBuilder, Symfony2 DI Configuration

Law of Demeter

Ez inkább a konkrét kérdésedre vonatkozna, úgy érzem itt picit kevered a dolgokat, itt úgy tűnhet, hogy a chain a probléma, de egészen másról van szó, a chain itt csak egy tünet, nem a betegség... ebbe itt most nem mennék bele, mivel ennél jobb magyarázat nem kell:

The Paperboy, The Wallet, and The Law Of Demeter Ez kötelező kellene, hogy legyen minden OOP-t tanulónak.

Egyéb

Szerintem nem a legjobb példát sikerült kiválasztani, a linkelt sokkal érthetőbb. Úgy érzem ez a maileres elvitt egy rossz irányba. A Mailernek nem kell tudnia a Queue-ról. Ha gondolod nézd meg a Swiftmailer implementációját, az nagyon közel van az igazsághoz (még ha a CS ma már elavault is).
2

Köszi! Természetesen a

Lencse · 2014. Szep. 11. (Cs), 13.02
Köszi!

Természetesen a setterek mindig $this-t adnak vissza, nem lehet olyan probléma mint az első példánál.

Általános jó tanács az, hogy ahol más osztályt ad vissza, ott nem ajánlott a chain.

Igen, ezzel tisztában vagyok, hogy a setterek visszatérhetnek az objektummal, tényleg praktikus, a method chaininget én is úgy értettem, hogy különböző típusú objektumokat láncolunk végig.

Szerintem nem a legjobb példát sikerült kiválasztani, a linkelt sokkal érthetőbb.

A Mailer azért jó példa, mert én írtam, tényleg működő kód, egy igazi példán keresztül nézhetem meg, hogy érdemes továbbfejleszteni.
Szerintem a cikkben említett példa eléggé hasonlít az enyémre, itt a Customer~Mailer és Wallet~MailQueue megfeleltetések nagyjából stimmelnek.
Nekem első körben az jött le a cikkből, hogy az első feltételezésem helyes volt, és érdemes a mailQueue mezőt elrejteni a Mailer ügyfelei elől és megírni a két burkolófüggvényt.

Úgy érzem ez a maileres elvitt egy rossz irányba. A Mailernek nem kell tudnia a Queue-ról.

Természetesen nem kell tudnia, az ügyfelek használhatnák közvetlenül a MailQueue osztályt is. A fejlesztőtársam kérte, hogy minden emaillel kapcsolatos dolgot egy osztály (Mailer) intézzen, ha úgy tetszik ez egy facade két másik osztályhoz (MailQueue és MailTransport)

Law of Demeter
A method of an object should invoke only the methods of the following kinds of
objects:
1. itself
2. its parameters
3. any objects it creates/instantiates
4. its direct component objects

Ebből viszont következik (ezen már korábban is gondolkodtam), hogy pl. a rengeteg symfonys tutorialban szereplő
  1. $this->getDoctrine()->getManager()->getRepository()  
jellegű láncok sértik a törvényt, tény, hogy már ránézésre sem egészségesek. Ezt hogy érdemes elkerülni?
3

$this->getDoctrine()->getManager()->getRepository()

Práger Ádám · 2014. Szep. 11. (Cs), 17.07
Symfony Controllerekben azért van így, hogy egyszerű legyen a fejlesztés, de ezt egy serviceben soha nem szabad így használni.

Ha valaminek csak pl a repositoryról kell tudni, akkor azt kell injektálni, és nem a managert / doctrine registryt és végképp nem az egész service containert. (Circular reference exceptionnek nézz utána ha gondolod, érdemes)

A Controller azért kivétel, mert az egész containerről tud... lehetne definiálni a repót egy külön serviceként, és csak simán $this->get('my_repo'), de ez ugyanúgy a containerből szedné a servicet, ami már violation.

Tehát ennek a "hibának" oka a kényelem, és semmi más.

A javasolt módszer egyébként az, hogy ne extendeld a base Controller osztály, hanem serviceként készítsd a saját controllerjeidet, és csak azt injektáld ami kell. A közösen használt serviceknek pedig csinálhatsz egy saját base classt.

Tipikus RAD probléma egyébként, tradeoff mindig van.
4

Értem.

Lencse · 2014. Szep. 12. (P), 15.28
Más, rokon probléma:

Egy osztály használ valamilyen szolgáltatást, amit mondjuk egy gyárból kapunk. Tegyük fel, hogy maga a gyár már ott van a service containerben, ezért könnyen át tudjuk passzolni a konstruktornak.

Ekkor a következő kód szintén sérti Demeter törvényét:
  1. class A {  
  2.   
  3.     private $serviceFactory;  
  4.   
  5.     function construct(ServiceFactory $serviceFactory) {  
  6.         $this->serviceFactory = $serviceFactory;  
  7.     }  
  8.   
  9.     public function someMethod() {  
  10.         $this->serviceFactory->getInstance()->doSomething();  
  11.     }  
  12.   
  13. }  
Kérdés: helyesen járok-e el, ha a szolgáltatást a konstruktorban veszem el, és utána már csak a konstruktorban inicializált objektum metódusait hívogatom?
  1. class A {  
  2.   
  3.     private $service;  
  4.   
  5.     function construct(ServiceFactory $serviceFactory) {  
  6.         $this->service = $serviceFactory->getInstance();  
  7.     }  
  8.   
  9.     public function someMethod() {  
  10.         $this->service->doSomething();  
  11.     }  
  12.   
  13. }  
Pontokba szedve sehol nem sérti a törvényt, de ha úgy nézzük, tűnhet hacknek is. Erről mit gondolsz?
5

Szerintem ez is sérti a

Práger Ádám · 2014. Szep. 12. (P), 17.07
Szerintem ez is sérti a törvényt. A factoryról nem kell tudnia az osztálynak ebben az esetben. (Feltételezve, hogy végig 1 legyártott objektum kell csak)

Symfonyban erre is van megoldás, néhány esetben a prototype scope elég a servicenek, ha viszont rendes factory kell, akkor http://symfony.com/doc/current/components/dependency_injection/factories.html
6

Request

Lencse · 2014. Szep. 15. (H), 09.49
A factoryval kicsit félrevittem, a konkrét dolog, ami eszembe juttatta a problémát, nem is egy factoryval előállított objektum, hanem a Request Stackből előhívató currentRequest.
9

Nincs ezzel semmi baj, ha a

inf · 2014. Szep. 16. (K), 04.57
Nincs ezzel semmi baj, ha a stack egy adatstruktúra, ami csak request tárolásra van. Kb ugyanazt csinálja, mintha egy $request = $array[0]-t hívnál.

A doctrines példád is hasonló lehet, bár annak a belső felépítésébe nem igazán látok bele. Már régen néztem, de úgy emlékszem, hogy a symfony2 és a doctrine2 jól szervezettek.
8

class A { private

inf · 2014. Szep. 16. (K), 04.43
  1. class A {    
  2.     
  3.     private $service;    
  4.     
  5.     function construct(Service $service) {    
  6.         $this->service = $service;    
  7.     }    
  8.     
  9.     public function someMethod() {    
  10.         $this->service->doSomething();    
  11.     }    
  12.     
  13. }   
  14.   
  15. $service = $serviceFactory->getInstance();  
  16. $a = new A($service);  
Az a-nak semmi köze nincs ahhoz, hogy honnan jön a service. Elég gáz lenne úgy kimockolni a service-t unit test-eknél, ahogy te csinálod. Valami mockfactory-t kéne csinálnod hozzá. Onnantól meg inkább integrációs teszt szaga lenne a dolognak, meg nyilván train wreck lenne az is.
10

Igen, ezért is írtam hogy

Lencse · 2014. Szep. 16. (K), 14.58
Igen, ezért is írtam hogy rossz példa volt a service factory. Nagyából értem, mit akartok kihozni, tényleg jó meglátás, hogy szét kell választani az objektumokat és az adatstruktúrákat. Köszi!
7

Ez attól függ, hogy mi a cél.

inf · 2014. Szep. 16. (K), 04.33
Ez attól függ, hogy mi a cél. Ha objektumokat akarsz létrehozni, akkor adat- és implementáció rejtés kell. Ez itt a queue esetében sérül. Ha adatstruktúrákat akarsz létrehozni, akkor meg pont, hogy nem szabad elrejteni az adatokat, mint ahogy a transport esetében teszed. Szóval a mailer-ed objektum és adatstruktúra keverék. Ez nem szerencsés, mert nehezen karbantartható. Minden esetben választanod kell a kettő között.

Jelen esetben az objektumot kell választanod, mert egy facade-et szeretnél létrehozni a mailer osztállyal, ami kitakarja a levélküldés és a várakozási sorba rakás mikéntjét a külvilág számára, tehát a get-queue-nek el kell tűnnie és helyette a mailer-hez kell adnod valami enqueue-t vagy ilyesmit.

Tiszta adatstruktúrák esetében ez a fajta lekérés teljesen jól működő dolog lehet: $(x).get(y).get(z), illetve az adatstruktúra építésénél is teljesen működőképes: d3.select(x).append(y).append(z). Mindkettőnél DOM műveletekről van szó, tehát a DOM fa, mint adatstruktúra építéséről illetve bejárásáról...

Objektumoknál esetleg valamilyen építő mintánál fel lehet használni az objektum gráf, mint adatstruktúra építésére. A setter chain nem igazán jellemző, talán DI container-ben előfordulhat, de pl egy java fejlesztő elég csúnyán nézne rád, ha ilyesmit látna. Egyébként is a legtöbb esetben jobb valamilyen config adatstruktúrát megadni, ami minden paramétert egyszerre tartalmaz, mint egyesével beállítgatni. Így lehetőség van elleőrizni, hogy minden paraméter megvan e, amire az objektumnak szüksége van a működéséhez.