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:
class Mailer {
    //...
    
    /**
     * @var MailQueue
     */
    private $mailQueue;
    
    /**
     * @var MailTransport
     */
    private $mailTransport;

    /**
     * @param Mail $mail
     */
    public function send(Mail $mail) {
        $this->mailTransport->send($mail);
    }

    /**
     * @return MailQueue
     */
    public function getMailQueue() {
        return $this->mailQueue;
    }

}

class MailQueue {
    //...
    
    /**
     * @var MailTransport
     */
    private $mailTransport;
    
    /**
     * @param Mail $mail
     */
    public function addMail(Mail $mail) {
        //...
    }
    
    public function sendMails() {
        //...
        foreach($mails as $m) {
            this->mailTransport->send($m);
            //...
        }
    }
    
}
Használni pedig így lehet:
$mailer->send($mail);

// vagy:

$mailer->getMailQueue()->addMail($mail1);
$mailer->getMailQueue()->addMail($mail2);
$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):
$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:
$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ő 
$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:
class A {

    private $serviceFactory;

    function construct(ServiceFactory $serviceFactory) {
        $this->serviceFactory = $serviceFactory;
    }

    public function someMethod() {
        $this->serviceFactory->getInstance()->doSomething();
    }

}
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?
class A {

    private $service;

    function construct(ServiceFactory $serviceFactory) {
        $this->service = $serviceFactory->getInstance();
    }

    public function someMethod() {
        $this->service->doSomething();
    }

}
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

class A {  
  
    private $service;  
  
    function construct(Service $service) {  
        $this->service = $service;  
    }  
  
    public function someMethod() {  
        $this->service->doSomething();  
    }  
  
} 

$service = $serviceFactory->getInstance();
$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.