Jeśli nie DRY, to co?

Jakiś czas temu przeglądając kod aplikacji natrafiłem na kawałek kodu, który po znacznym uproszczeniu mógłby wyglądać mniej więcej tak:

class GenericInvoiceMapper {

    protected $invoice;

    protected function getTemplateCode() {
        return $this->invoice->getRegistryCode();
    }
}

class NormalInvoiceMapper extends GenericInvoiceMapper {

    public function map(Invoice $invoice) {
        $this->invoice = $invoice;

        $invoiceDTO = new NormalInvoiceDTO();
        $invoiceDTO->TemplateCode = $this->getTemplateCode();
        $invoiceDTO->NormalInvoiceItem = $this->addDocumentItem();

        return $invoiceDTO;
    }

    private function addDocumentItem() {
        $itemMapper = new NormalInvoiceItemMapper();
        return $itemMapper->map($this->invoice);
    }
}

class ProformInvoiceMapper extends GenericInvoiceMapper {

    const TEMPLATE_CODE_SUFFIX = '-K';
    
    public function map(Invoice $invoice) {
        $this->invoice = $invoice;

        $proformInvoiceDTO = new ProformInvoiceDTO();
        $proformInvoiceDTO->TemplateCode = $this->getTemplateCode() . self::TEMPLATE_CODE_SUFFIX;
        $proformInvoiceDTO->ProformInvoiceItem = $this->addDocumentItem();

        return $proformInvoiceDTO;
    }

    private function addDocumentItem() {
        $itemMapper = new ProformInvoiceItemMapper();
        return $itemMapper->map($this->invoice);
    }
}

Kilka słów o tym co tu się dzieje. Klasy służą do mapowania obiektu faktury na obiekt DTO.
Mamy klasę rodzica GenericInvoiceMapper, po której dziedziczą klasy NormalInvoiceMapper i ProformInvoiceMapper mapujące na konkretne obiekty DTO w zależności od potrzeby.

Co z tym kodem jest nie tak?

Na pierwszy rzut oka widać, że instancją klasy GenericInvoiceMapper nie wiele jesteśmy w stanie zrobić – tworzenie jej obiektu nie ma sensu. Powinna być klasą abstrakcyjną. Brakuje też abstrakcyjnej metody map.

DRY

Jednak to co jest tu najbardziej interesujące możemy znaleźć w klasach potomnych: NormalInvoiceMapper i ProformInvoiceMapper. Widać, że klasy są do siebie podobne… a za razem na tyle się różnią, że ciężko wydzielić część wspólną. Algorytm mapowania jest taki sam, natomiast wykonywany jest na różnych obiektach, różniącymi się po między sobą nazwami atrybutów. Dodatkowo metody addDocumentItem korzystają z różnych maperów.

Tak naprawdę mamy tutaj problem z DRY. Nie widać tego od razu bo to nie zwykły „copy-paste”. Programista zwyczajnie tego nie zauważył albo nie wiedział co z tym zrobić.

Więc co z tym zrobić?

Najprościej jest pójść tropem klasy i metod abstrakcyjnych. Wyciągamy różnice w algorytmie do metod i określamy te metody jako abstrakcyjne w klasie GenericInvoiceMapper. Klasa rodzica jako jedyna powinna mieć metodę ::map() zawierającą uwspólniony algorytm i pobierać odpowiednie nazwy atrybutów czy instancje klas z abstrakcyjnych metod nadpisanych w klasach potomnych. Po refactoringu kod mógłby wyglądać w następujący sposób:

abstract class GenericInvoiceMapper {

    const TEMPLATE_CODE_SUFFIX = '';

    protected $invoice;

    abstract protected function getDTO();
    abstract protected function getItemMapper();
    abstract protected function getItemFieldName();    
    
    public function map(Invoice $invoice) {
        $this->invoice = $invoice;
        $itemPropertyName = $this->getItemFieldName();

        $dto = $this->getDTO();
        $dto->TemplateCode = $this->getTemplateCode();
        $dto->{$itemPropertyName} = $this->addDocumentItem();

        return $dto;
    }    

    protected function getTemplateCode() {
        return $this->invoice->getRegistryCode() . static::TEMPLATE_CODE_SUFFIX;
    }
    
    protected function addDocumentItem() {
        $itemMapper = $this->getItemMapper();
        
        return $itemMapper->map($this->invoice);
    }    
}

class NormalInvoiceMapper extends GenericInvoiceMapper {

    protected function getDTO() {
        return new NormalInvoiceDTO();
    }

    protected function getItemFieldName() {
        return 'NormalInvoiceItem';
    }

    protected function getItemMapper() {
        return new NormalInvoiceItemMapper();
    }    
}

class ProformInvoiceMapper extends GenericInvoiceMapper {

    const TEMPLATE_CODE_SUFFIX = '-K';

    protected function getDTO() {
        return new ProformInvoiceDTO();
    }

    protected function getItemFieldName() {
        return 'ProformInvoiceItem';
    }

    protected function getItemMapper() {
        return new ProformInvoiceItemMapper();
    }

}

To co widać powyżej nie jest niczym innym jak wykorzystaniem wzorca Template Method. Podsumowując: pozbyliśmy się zaśmiecającego kod DRY i wykorzystaliśmy do tego stary jak świat wzorzec projektowy. Wygląda na to, że wszystko jest OK… ale czy na pewno?

Co tu znowu jest nie tak?

Problem pojawia się w momencie gdy chcielibyśmy takie rozwiązanie przetestować jednostkowo. Testowanie klas za pomocą unit testów powinno odbywać się w jak największej izolacji. I nagle okazuje się, że nie jesteśmy w stanie samodzielnie przetestować klasy abstrakcyjnej ani klasy potomnej. Z Template Method jest taki problem, że używa dziedziczenia, a dziedziczenie wprowadza bardzo silną zależność po między klasami. I znowu jest problem…

I co teraz z tym zrobić?

Skoro problemem okazuje się dziedziczenie to trzeba się go pozbyć. I jak się okazuje wcale to nie jest trudne. Idziemy w dobrym kierunku i mamy już częściowo zrefaktoryzowany kod, który łatwo można przerobić na wzorzec strategi.

Klasa GenericInvoiceMapper na na powrót przestaje być abstrakcyjna. Jej abstrakcyjne metody wyciągamy do interfejsu. Klasy mapperów: NormalInvoiceMapper i ProformInvoiceMapper zamieniamy na strategie implementujące interface. One zaś będą wstrzykiwane do głównej klasy GenericInvoiceMapper, znającej algorytm mapujący.

class GenericInvoiceMapper {
    private $invoice;
    
    public function __construct(InvoiceMapperStrategy $strategy) {
        $this->strategy = $strategy;
    }

    public function map(Invoice $invoice) {
        $this->invoice = $invoice;

        $dto = $this->strategy->getDTO();
        $dto->TemplateCode = $this->strategy->getTemplateCode($this->invoice);
        $dto->{$this->strategy->getItemFieldName()} = $this->addDocumentItem();

        return $dto;
    }

    private function addDocumentItem() {
        $lineDTO = $this->strategy->getLineDTO();

        return $this->lineBuilder->build($lineDTO, $this->invoice);
    }
}

interface InvoiceMapperStrategy {

    public function getDTO();

    public function getTemplateCode(Invoice $invoice);
    
    public function getItemFieldName();
    
    
}

class ProformInvoiceMapperStrategy implements InvoiceMapperStrategy {

    const TEMPLATE_CODE_SUFFIX = '-K';

    public function getDTO() {
        return new ProformInvoiceDTO();
    }

    public function getTemplateCode(Invoice $invoice) {
        return $invoice->getRegistryCode() . self::TEMPLATE_CODE_SUFFIX;
    }    

    public function getItemFieldName() {
        return 'ProformInvoiceItem';
    }
    
    public function getItemMapper() {
        return new ProformInvoiceItemMapper();
    }    
}

class NormalInvoiceMapperStrategy implements InvoiceMapperStrategy {

    public function getDTO() {
        return new NormalInvoiceDTO();
    }
    
    public function getTemplateCode(Invoice $invoice) {
        return $invoice->getRegistryCode();
    }      

    public function getItemFieldName() {
        return 'NormalInvoiceItem';
    }
    
    public function getItemMapper() {
        return new NormalInvoiceItemMapper();
    } 
}

Jak widać powyższy kod diametralnie różni się od tego co zastaliśmy na początku. Nie ma DRY, nie ma Template Method i dziedziczenia, nie ma problemu z testami bo każdą z klas jesteśmy w stanie przetestować osobno.