OOP - Design Patterns - Ocena jakości kodu Opcje

[ziolo]

Witam,

Chciałem was prosić o ocenę jakości kodu OOP na przykładzie klasy.
Ostatnio zacząłem zwracać większą uwagę na jakość swojego kodu.

Jest to prosta klasa Mailer. Ma zadanie wysyłać maila oraz zapisywać go do bazy(w razie czego gdyby np była awaria smtp, to dobrze byłoby mieć tego maila w bazie).
Mail to zwykła encja z polami (from, to, subject, body, created) - (kod będzie wykorzystywany w SF2.0)
Pierwsza wersja klasy poniżej, to jest kod jaki zawsze pisałem. Generalnie z braku czasu mógłbym go zostawić. Ale ponieważ chciałem coś poprawić to zacząłem refaktorować.

[PHP] pobierz, plaintext 
 
class Mailer
{
    /**
     * @var \Swift_Mailer
     */
    private $mailer;
 
    /**
     * @var EntityManager
     */
    private $em;
 
 
    function __construct(EntityManager $em, \Swift_Mailer $mailer)
    {
        $this->em = $em;
        $this->mailer = $mailer;
    }
 
    public function send($from, $to, $subject, $body)
    {
        $this->sendMail($from, $to, $subject, $body);
        $this->addMailToDatabase($from, $to, $subject, $body);
    }
 
    protected function sendMail($from, $to, $subject, $body)
    {
        $message = \Swift_Message::newInstance()
            ->setSubject("=?UTF-8?B?" . base64_encode($subject) . "?=")
            ->setFrom($from)
            ->setTo($to)
            ->setBody($body, 'text/html');
        $this->mailer->send($message);
    }
 
    protected function addMailToDatabase($from, $to, $subject, $body)
    {
        $mail = new Mail();
        $mail->setFrom($from);
        $mail->setBody($body);
        $mail->setSubject($subject);
        $mail->setTo($to);
        $this->em->persist($mail);
        $this->em->flush();
    }
}
[PHP] pobierz, plaintext 

Po pierwsze co mi się nie podoba to jaki pisze Robert C Martin im mniej argumentów mają funkcje tym lepiej.
Chcę mieć publicznie metodę send(from,to,subject,body) dostępną bo tak mi pasuje, ale czy muszę te same argumenty wywoływać w dwóch metodach prywatnych ?
Zastanawiałem się też, czy klasa nie łamię zasadę jednej odpowiedzialności(w końcu ona wysyła maila oraz zapisuje go do bazy) ale uznałem, że nie. Traktuję moją klasę jako Facadę(mamy pierwszy wzorzec projektowy) koordyującą te dwie czynności.

Ok teraz druga wersja kodu:

[PHP] pobierz, plaintext 
 
class Mailer
{
    /**
     * @var \Swift_Mailer
     */
    private $mailer;
 
    /**
     * @var EntityManager
     */
    private $em;
 
 
    function __construct(EntityManager $em, \Swift_Mailer $mailer)
    {
        $this->em = $em;
        $this->mailer = $mailer;
    }
 
    public function send($from, $to, $subject, $body)
    {
        $mail = $this->createMail($from, $to, $subject, $body);
        $this->sendMail($mail);
        $this->save($mail);
    }
 
    /**
     * @param $from
     * @param $to
     * @param $subject
     * @param $body
     * @return Mail
     */
    protected function createMail($from, $to, $subject, $body)
    {
        $mail = new Mail();
        $mail->setFrom($from)
            ->setBody($body)
            ->setSubject($subject)
            ->setTo($to);
        return $mail;
    }
 
    protected function sendMail(Mail $mail)
    {
        $message = \Swift_Message::newInstance()
            ->setSubject("=?UTF-8?B?" . base64_encode($mail->getSubject()) . "?=")
            ->setFrom($mail->getFrom())
            ->setTo($mail->getTo())
            ->setBody($mail->getBody(), 'text/html');
        $this->mailer->send($message);
    }
 
    protected function save(Mail $mail)
    {
        $this->em->persist($mail);
        $this->em->flush();
    }
}
[PHP] pobierz, plaintext 

Co się zmieniło - dopisaliśmy funkcję createMail - która ma za zadanie, utworzyć instancję klasy Mail i przypisać jej attrybuty. Zmieniłem nazwę metody addMailToDatabase na save. Być może w jakieś klasie dziediczącej będziemy chcieli zapisywać np do pliku a nie do bazy danych. Więc nazwa będzie odpowiedniejsza.
Myślę, że już jest lepiej.
Co możemy zrobić dalej. Na pewno createMail - jest do poprawy(nazwa jest zła, ona nie tylko tworzy ale także buduje mail) pasowałoby wykorzystać wzorzec FactoryMethod, gdybyśmy chcieli zmieniać encję,

[PHP] pobierz, plaintext 
class Mailer
{
    /**
     * @var \Swift_Mailer
     */
    private $mailer;
 
    /**
     * @var EntityManager
     */
    private $em;
 
    /**
     * @var Mail
     */
    private $mail;
 
 
    function __construct(EntityManager $em, \Swift_Mailer $mailer)
    {
        $this->em = $em;
        $this->mailer = $mailer;
    }
 
    public function send($from, $to, $subject, $body)
    {
        $this->createMail();
        $this->buildMail($from, $to, $subject, $body);
        $this->sendMail();
        $this->save();
    }
 
    protected function createMail()
    {
        $this->mail = new Mail();
    }
 
    protected function buildMail($from, $to, $subject, $body)
    {
        $this->mail->setFrom($from)
            ->setBody($body)
            ->setSubject($subject)
            ->setTo($to);
    }
 
    protected function sendMail()
    {
        $message = \Swift_Message::newInstance()
            ->setSubject("=?UTF-8?B?" . base64_encode($this->mail->getSubject()) . "?=")
            ->setFrom($this->mail->getFrom())
            ->setTo($this->mail->getTo())
            ->setBody($this->mail->getBody(), 'text/html');
        $this->mailer->send($message);
    }
 
    protected function save()
    {
        $this->em->persist($this->mail);
        $this->em->flush();
    }
}
[PHP] pobierz, plaintext 

Ok co zrobiliśmy - mamy teraz dwie metody createMail oraz buildMail, naturalnym okazało się przeniesienie $mail jako atrybut klasy, dzięki temu w poszeczególnych metodach mamy mniej argumentów - super.
Czy to już koniec ? Moim zdaniem nie, zdecydowanie pasuję zrobić MailerBuilder(kolejny wzorzec) osobną klasę z metodami createMail oraz buildMail.


Ok więc teraz wersja 4 kodu:

[PHP] pobierz, plaintext 
 
 
class MailBuilder
{
    /**
     * @var Mail
     */
    private $mail;
 
    public function createMail()
    {
        $this->mail = new Mail();
        return $this;
    }
 
    public function buildMail($from, $to, $subject, $body)
    {
        $this->mail->setFrom($from)
            ->setBody($body)
            ->setSubject($subject)
            ->setTo($to);
        return $this;
    }
 
    public function getMail()
    {
        return $this->mail;
    }
} 
[PHP] pobierz, plaintext 

[PHP] pobierz, plaintext 
use Doctrine\ORM\EntityManager;
 
class Mailer
{
    /**
     * @var \Swift_Mailer
     */
    private $mailer;
 
    /**
     * @var EntityManager
     */
    private $em;
 
    /**
     * @var Mail
     */
    private $mail;
 
    /**
     * @var MailBuilder
     */
    private $builder;
 
 
    function __construct(EntityManager $em, \Swift_Mailer $mailer, MailBuilder $builder)
    {
        $this->em = $em;
        $this->mailer = $mailer;
        $this->builder = $builder;
    }
 
    public function send($from, $to, $subject, $body)
    {
        $this->mail = $this->builder
            ->createMail()
            ->buildMail($from, $to, $subject, $body)
            ->getMail();
        $this->sendMail();
        $this->save();
    }
 
    protected function sendMail()
    {
        $message = \Swift_Message::newInstance()
            ->setSubject("=?UTF-8?B?" . base64_encode($this->mail->getSubject()) . "?=")
            ->setFrom($this->mail->getFrom())
            ->setTo($this->mail->getTo())
            ->setBody($this->mail->getBody(), 'text/html');
        $this->mailer->send($message);
    }
 
    protected function save()
    {
        $this->em->persist($this->mail);
        $this->em->flush();
    }
}
[PHP] pobierz, plaintext 

Ok mamy w końcu zamiast jednej klasy, dwie klasy. MailerBuilder i Mailer. W Mailerze w fajny sposób wykorzystujemy Buildera.
Wszystko ok, generalnie mógłbym zakończyć refaktoryzacje. Niby wszystko ok, możemy przeciążąć save, sendMail, Buildera.
Ale ciągle coś mi tu nie pasuję, czy to łamanie zasady jednej odpowiedzialności, czy może to, że w Mailer wykorzystujemy EntityManager oraz SwiftMailer a nie powinniśmy.
No właśnie zdecydowanie należy stworzyć kolejne abstrakcje nazwijmy Sender or Saver i wykorzystać wzorzec Strategy.

Ok a więc teraz wersja 5 kodu. Namnożyło się klas i interfejsów (IMG:style_emoticons/default/smile.gif)

[PHP] pobierz, plaintext 
interface SaverInterface
{
    public function save(Mail $mail);
} 
[PHP] pobierz, plaintext 

[PHP] pobierz, plaintext 
class Saver implements SaverInterface
{
    /**
     * @var EntityManager
     */
    private $em;
 
    function __construct(EntityManager $em)
    {
        $this->em = $em;
    }
 
    public function save(Mail $mail)
    {
        $this->em->persist($mail);
        $this->em->flush();
    }
} 
[PHP] pobierz, plaintext 

SaverInterface oraz implementacja Savera zapisująca mail do bazy.

[PHP] pobierz, plaintext 
interface SenderInterface
{
    public function sendMail(Mail $mail);
} 
[PHP] pobierz, plaintext 

[PHP] pobierz, plaintext 
class Sender implements SenderInterface
{
    /**
     * @var \Swift_Mailer
     */
    private $mailer;
 
    function __construct(\Swift_Mailer $mailer)
    {
        $this->mailer = $mailer;
    }
 
    public function sendMail(Mail $mail)
    {
        $message = \Swift_Message::newInstance()
            ->setSubject("=?UTF-8?B?" . base64_encode($mail->getSubject()) . "?=")
            ->setFrom($mail->getFrom())
            ->setTo($mail->getTo())
            ->setBody($mail->getBody(), 'text/html');
        $this->mailer->send($message);
    }
} 
[PHP] pobierz, plaintext 

SenderInteface oraz implementacja Sendera wykorzystująca Swift_Mailer.

[PHP] pobierz, plaintext 
class Mailer
{
    /**
     * @var SenderInterface
     */
    private $sender;
 
    /**
     * @var SaverInterface
     */
    private $saver;
 
    /**
     * @var MailBuilder
     */
    private $builder;
 
 
    function __construct(SenderInterface $sender, SaverInterface $saver, MailBuilder $builder)
    {
        $this->sender = $sender;
        $this->saver = $saver;
        $this->builder = $builder;
    }
 
    public function send($from, $to, $subject, $body)
    {
        $mail = $this->builder
            ->createMail()
            ->buildMail($from, $to, $subject, $body)
            ->getMail();
        $this->sender->sendMail($mail);
        $this->saver->save($mail);
    }
}
[PHP] pobierz, plaintext 

Oraz nasza klasa Mailera, która nie potrzebuje już zależności EntityManager oraz Swift_Mailer. Klasa MailBuilder nie zmieniła się już w stosunku do wersji 4.

Teraz mamy chyba wszystko idealnie, możemy sobie konfigurować Mailera dowolnie z różnych Builderów, Senderów or Saverów. Pozbyłem się też wrażenia, że Mailer łamie zasadę jednej odpowiedzialności, teraz wyraźnie widać, że on koordynuję pracę róznych obiektów. Wykorzystane wzorce projektowe(Facade, Builder, FactoryMethod, Strategy, DI)

I teraz chciałem się zapytać o ocenę, jakbyście ocenieli wersję 5 kodu, w skali 1-10.
Patrząc też na to jak się zmieniłą wersja 1 od wersji5. Kto uważa, że to jest przerost formy nad treścią ?

[irmidjusz]

Spoko rozwiązanie, ale co, jeśli zechcę zapisać maila za pomocą kilku różnych implementacji SaverInterface jednocześnie? (IMG:style_emoticons/default/tongue.gif)

[ziolo]

Spoko rozwiązanie, ale co, jeśli zechcę zapisać maila za pomocą kilku różnych implementacji SaverInterface jednocześnie? (IMG:style_emoticons/default/tongue.gif)

Faktycznie dobry pomysł (IMG:style_emoticons/default/smile.gif)
Teraz jest jeszcze elastyczniej, tak samo można wstrzykiwać tablicę senderów jakbyśmy chcieli.
Wychodzi też zaleta OOP, bo kolejne wymagania/zmiany wymagań dużo łatwiej zakodować (IMG:style_emoticons/default/tongue.gif)

[PHP] pobierz, plaintext 
class Mailer
{
    /**
     * @var SenderInterface[]
     */
    private $senders;
 
    /**
     * @var SaverInterface[]
     */
    private $savers;
 
    /**
     * @var MailBuilder
     */
    private $builder;
 
 
    function __construct(SenderInterface[] $senders, SaverInterface[] $savers, MailBuilder $builder)
    {
        $this->senders = $senders;
        $this->savers = $savers;
        $this->builder = $builder;
    }
 
    public function send($from, $to, $subject, $body)
    {
        $mail = $this->builder
            ->createMail()
            ->buildMail($from, $to, $subject, $body)
            ->getMail();
        foreach($this->senders as $sender){
          $sender->sendMail($mail);
        }
        foreach($this->savers as $saver){
          $saver->save($mail);
        }
    }
}
[PHP] pobierz, plaintext 

Ten post edytował ziolo 5.10.2014, 22:00:52