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

[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ą ?

[skowron-line]

Dam 2 za chęci.
Klasa Saver to pomyłka
1. wstrzykujesz EntityManager a jak bedzie inny manager to sie wywali (design by contract)
2. metoda Save tez przyjmuje tylko klasy Mail, czyli dla klasy Person bedzie inny saver, hmmm słabo


Piszesz klase do wysylki maili w ktorej odwolujesz sie do klasy do wysylki maili, troche słabe.

No i najwazniejsze SRP (SOLID) zapis to zapis a wysylka to wysylka nie wpychaj tego do jednej klasy, zrob sobie tak ze zapisujesz do bazy maile a pozniej odczytujesz i wysylasz.

[ziolo]

@skowron-line Dzięki wielkie za odpowiedź.

Dam 2 za chęci.
Klasa Saver to pomyłka
1. wstrzykujesz EntityManager a jak bedzie inny manager to sie wywali (design by contract)
2. metoda Save tez przyjmuje tylko klasy Mail, czyli dla klasy Person bedzie inny saver, hmmm słabo

1) nie bardzo rozumiem o co Ci chodzi jaki inny EntityManager. Może nie być żadnego EntityManagera, co on ma wspólnego z wysypaniem się.
Mogę sobię stworzyć SaverToCsv implementujący metodę save - zapisujący po prostu maile do pliku csv.
Do klasy Mailer wstrzykuje tylko obiekt typu SaverInterface - mający metodę save. To mnie interesuje w klasie Mailer, a jak to już robi Saver to jego sprawa.

2) Mail - to jest encja własna tego bundla i Mailer tylko ją będzie wykorzystywał, nie będzie żadnych Person. Ok też właściwie powinno być MailInterface -(z getterami i stterami) ale to sobie już darowałem.

Piszesz klase do wysylki maili w ktorej odwolujesz sie do klasy do wysylki maili, troche słabe.

Chodzi o klasę Sender ?
Ok z tym się częściowo zgodzę, ale załóżmy, że np chciałbym wysyłać np wiadomości na facebooka albo sms, tak sobie wstrzykuje różnych senderów. Mailer mi się nie zmienia i o to chodzi.
Inna sprawa, że pasowałoby nazwać klasę zamiast Mailer(np Messager, Notifier)

No i najwazniejsze SRP (SOLID) zapis to zapis a wysylka to wysylka nie wpychaj tego do jednej klasy, zrob sobie tak ze zapisujesz do bazy maile a pozniej odczytujesz i wysylasz.

Idęą tej klasy był łatwy interfejs do wysyłania maili i przy okazji zapisywanie ich gdzieś.
Chcę sobie w kontrolerze mieć prosty zapis:

[PHP] pobierz, plaintext 
$this->get('own_mailer')->send($from,$to,$subject,$body)
[PHP] pobierz, plaintext 

Wysyłaniem zapisywaniem zajmują się: Sender i Saver. Mogę sobię stworzyć wiele instancji klas Mailer z różnymi konfiguracjami teraz.
Klasa Mailer nie łamie zasady SRP. Mailer ma jedną odpowiedzialność skoordynować te operacje udostępniając klientowi prosty interfejs - czytaj wzorzec Facade.
Druga opcja dla mnie odpada, to ma być prosty bundle, wykorzystywany w wielu projektach, nawet mniejszych, wszędzie trzeba by pamiętać np skonfigurować crona do wysyłki.

[irmidjusz]

Spoko rozwiązanie, ale co, jeśli zechcę zapisać maila za pomocą kilku różnych implementacji SaverInterface jednocześnie?

[ziolo]

Spoko rozwiązanie, ale co, jeśli zechcę zapisać maila za pomocą kilku różnych implementacji SaverInterface jednocześnie?

Faktycznie dobry pomysł
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ć

[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

[MateuszS]

Hm akurat w tak prostym mailerze raczej ciężko o jakieś zaawansowane obiektowe rozwiązania.