Perché l'ingiustizia impigliata con la classe che la usa è una cattiva pratica?

Question

Perché l'ingiustizia impigliata con la classe che la usa è una cattiva pratica?

#1 da (1 voti)
#2 da (1 voti)

1

Ho avuto questo argomento per un po ', perché ho notato che alcune persone preferiscono il codice "leggibile" su uno correttamente strutturato. Quindi nell'esempio che sto mostrando, in pratica ho questa classe Mapper in cui diverse istanze Mappable sono iniettate e il Mapper usa le istanze per sapere come mappare alcuni dati. La domanda qui è, perché è una cattiva pratica farlo. Ho il sospetto che i problemi siano: il metodo è statico, la factory è nella classe sbagliata, rendendo il Mapper strettamente accoppiato alle classi Map .

interface Mappable {
    public function getMetaData();
    public function setMetaData(array $metaData);
}

class BaseMap implements Mappable {
    protected $metaData = [];

    public function getMetaData() {
        return $this->metaData;
    }   

    public function setMetaData(array $metaData) {
        $this->metaData = $metaData;
    }

    /**
     * Possible Code Smell
     */
    public static function getNewMapper() {
        return new Mapper(new static);
    }
}

class MapA extends BaseMap {
    protected $metaData = [
        'backField1' => 'frontField1',
        'backField2' => 'frontField2'
    ];
}

class MapB extends BaseMap {
    protected $metaData = [
        'backFieldA' => 'frontFieldA',
        'backFieldB' => 'frontFieldB'
    ];
}

class Mapper {
    private $map;

    public function __construct(Mappable $map) {
        $this->setMap($map);
    }

    public function setMap(Mappable $map) {
        $this->map = $map;
    }

    public function parseData($dataRow) {
        $metaData = $this->map->getMetaData();
        $parsedDataRow = [];

        foreach($dataRow as $columnName => $columnValue) {
            if(isset($metaData[$columnName]))
                $parsedDataRow[$metaData[$columnName]] = $columnValue;
        }

        return $parsedDataRow;
    }
}

// Probably correct
$mapper = new Mapper(new MapA());
print_r($mapper->parseData(['backField1' => 'foo1', 'backField2' => 'bar1']));

$mapper->setMap(new MapB());
print_r($mapper->parseData(['backFieldA' => 'fooA', 'backFieldB' => 'barB']));

// Possible Code Smell
print_r(MapA::getNewMapper()->parseData(['backField1' => 'foo1', 'backField2' => 'bar1']));
print_r(MapB::getNewMapper()->parseData(['backFieldA' => 'fooA', 'backFieldB' => 'barB']));

Schema di classe UML:

php static factory-method

posta ziGi 07.11.2014 - 14:16

fonte

2 risposte

Leggi altre domande sui tag php static factory-method

Utilizzo del codice Primo concetto di Entity Framework in produzione Archiviazione efficiente e ricerca multidirezionale di dati gerarchici

score 1 · Answer 1

L'altro problema che vedo con il tuo codice è che l'ereditarietà sta facendo contratti con le classi di cui non dovrebbe aver bisogno e sta davvero infrangendo tutte le pratiche di oop. Questo approccio consente una minore quantità di codice e un codice più pulito, ma segue anche la convenzione delle funzionalità raggruppate e dell'ereditarietà multipla.

interface Mappable {
    public function getMetaData();
    public function setMetaData(array $metaData);
}

abstract class BaseMap implements Mappable {
    protected $metaData = [];

    public function getMetaData() {
        return $this->metaData;
    }   

    public function setMetaData(array $metaData) {
        $this->metaData = $metaData;
    }
}

abstract class Mapper extends BaseMap{
    private $map;

    public function setMap($map) {
        $this->map = $map;
    }

    public function parseData($dataRow) {
        $metaData = $this->getMetaData();
        $parsedDataRow = [];

        foreach($dataRow as $columnName => $columnValue) {
            if(isset($metaData[$columnName]))
                $parsedDataRow[$metaData[$columnName]] = $columnValue;
        }

        return $parsedDataRow;
    }
}

class MapA extends Mapper {
    protected $metaData = [
        'backField1' => 'frontField1',
        'backField2' => 'frontField2'
    ];
}

class MapB extends Mapper {
    protected $metaData = [
        'backFieldA' => 'frontFieldA',
        'backFieldB' => 'frontFieldB'
    ];
}


/* i think the benifits speak for them selfs */
// less code
// less objects 
// clean and readable
// easy to use
$mapA  = new MapA();
$mapA->parseData(['backField1' => 'foo1', 'backField2' => 'bar1']);

$mapB  = new MapB();
$mapB->parseData(['backField1' => 'foo1', 'backField2' => 'bar1']);

score 1 · Answer 2

Non sono un esperto di PHP, ma posso leggere il codice.

Quando vedo questo codice

// Probably correct
$mapper = new Mapper(new MapA());
print_r($mapper->parseData(['backField1' => 'foo1', 'backField2' => 'bar1']));

Quindi penso tra me e me, perché devo istanziare un Mapper con MapA . Vorrei avere 1 classe che sia detiene i dati e sa come mappare. Vorrei class Mapper estendere BaseMap e MapA estendere Mapper

Quindi, posso semplicemente

$mapper = new MapA();
print_r($mapper->parseData(['backField1' => 'foo1', 'backField2' => 'bar1']));

Certo, MapA potrebbe non essere il miglior nome, potresti aver usato il nome del vero metodo di mappatura.

Modifica:

L'argomento contatore per questo è che questo lascia il design attuale flessibile, si potrebbero usare diverse classi Mapper . Il mio contro-argomento; questa funzione non è intuitiva perché l'unico metodo factory restituisce solo Mapper .

Ciò significa che il tuo approccio non è e non è intuitivo. Se si desidera mantenere flessibile la progettazione, è necessario rimuovere il metodo di produzione statico. Puoi sostituirlo con un commento che indica che può essere utilizzato da diversi mapper.

Infine, dovresti progettare con YAGNI in mente, il refactoring per i nuovi requisiti è ok.