Come rifattorizzare il codice che ha 2 problemi

Question

Come rifattorizzare il codice che ha 2 problemi

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

1

Dire che ho questo codice:

        if ($this->ship == "e") {
            $this->price->addLine($this->price->base * $this->price->e, 0, "Export");
            $this->price->total += $this->price->base * $this->price->e;

        } elseif ($this->ship == "d") {
            $this->price->addLine($this->price->base * $this->price->d, 0, "Domestic");
            $this->price->total += $this->price->base * $this->price->d;
        }

Ho più blocchi identici di codice come sopra. Ho anche un altro codice (diverso da $this->ship ) che è simile nella struttura. Fondamentalmente, questo codice aggiunge Line a un PDF, dove la riga dice "Domestico" o "Esporta" insieme ad altre opzioni. Il codice sopra calcola anche il prezzo per quella linea e lo aggiunge al totale.

I miei pensieri su come refactoring questo:

Voglio rimuovere la duplicazione. Voglio anche rispettare i principi SOLID, cioè la separazione delle preoccupazioni. Vedo che il codice sopra fa due cose (calcola il prezzo e aggiunge un elemento pubblicitario al PDF). Posso sicuramente suddividere questo codice in due funzioni separate, ma poi mi ritrovo ancora con codice duplicato - Devo controllare se $this->ship equivale a e o d in entrambe le posizioni. Non va bene. Se devo aggiungere una terza opzione, devo cambiare il codice in due punti.

Quindi posso spostare il mio controllo per e o d nel corpo principale del codice, ma ho ancora la duplicazione perché avrò

switch($this->ship)
{
    case 'd': $cost = $this->getShippingCost($this->price->base, $this->price->d);break;
    case 'e': $cost = $this->getShippingCost($this->price->base, $this->price->e);break;     
}

Ottengo queste due lunghe code di codice praticamente per un singolo cambiamento di carattere. Mentre forse è abbastanza accettabile, non mi piace molto. Se devo cambiare il numero di parametri per esempio, o il parametro di base o ecc, dovrò cambiarlo in più di un posto.

Fondamentalmente, finora non importa in che modo faccio girare questo codice, finisco con qualche duplicazione da qualche parte. Voglio dire se è inevitabile, immagino sia okay. Ma qui vengo per un consiglio su come vedere se c'è un modo per separare le preoccupazioni e rimuovere la duplicazione e avere un numero minimo di posti (idealmente one ) quando devo cambiare qualcosa. può essere fatto?

php solid single-responsibility separation-of-concerns

posta Dennis 06.05.2014 - 23:37

fonte

2 risposte

Leggi altre domande sui tag php solid single-responsibility separation-of-concerns

Come migrare un framework di serializzazione XML in Java in un sistema strettamente accoppiato? È stato aggiunto il supporto per un altro sistema operativo a una versione secondaria oa una patch?

score 2 · Answer 1

Penso che il più semplice e più leggibile sia, estraendo il prezzo e l'etichetta nel caso e mantenendone il resto fuori:

switch($this->ship)
{
    case 'd': 
        $price = $this->price->d;
        $label = 'Domestic';
        break;
    case 'e': 
        $price = $this->price->e;
        $label = 'Export';
        break;
}
$cost = $this->price->base * $price;
$this->price->addLine($cost, 0, $label);
$this->price->total += $cost;

Questo ti consente anche di rompere questo metodo in metodi separati che restituiscono il prezzo e l'etichetta in base al tipo (sì, ci sono ancora due problemi, a meno che tu non lo veda come restituire 'proprietà di tipo' o qualcosa del genere), un altro metodo che calcola il costo, ecc.

Alternativa: nome proprietà variabile

Personalmente non mi piacciono le soluzioni come questa, perché penso che siano fragili, scarsamente leggibili e difficili da debugare, ma hanno i loro usi e forse ti piacerà:

Potresti anche "abusare" del fatto che il personaggio è uguale al nome della proprietà, quindi puoi utilizzarlo in questo modo:

$ship = $this->ship;
$price = $this->price->$ship;

Forse anche questo funzionerà, ma non sono sicuro di questa sintassi. E anche se lo fosse, lo capisci? Io non. :)

$price = $this->price->($this->ship);

Alernative: matrici con le chiavi:

È possibile memorizzare i prezzi in un array per chiave, in modo da poter leggere $this->price->prices['e'] . Quindi il tuo codice potrebbe apparire come questo:

$cost = $this->price->base * $this->price->prices[$this->ship];
$this->price->addLine($cost, 0, $this->labels[$this->ship]);
$this->price->total += $cost;

Devo dire che posso vedere il vantaggio della variante dell'array, anche se forse la prima variante è più leggibile e richiederà meno lavoro di refactoring.

score 0 · Answer 2

È difficile produrre un consiglio di refactoring davvero solido basato su poche righe di codice fuori dal contesto, ma in generale non preoccuparti troppo di alcune linee quasi identiche, non è un problema pratico.

In alcuni casi potrebbe essere un segno di cattiva progettazione, tuttavia la semplice eliminazione della duplicazione servirà solo a nascondere il problema.

Nel tuo caso, non riesco a immaginare come finirai con quel codice, in che modo $this può contenere sia il totale cumulativo sia il prezzo per i singoli articoli, con quegli articoli che apparentemente non sono nemmeno memorizzati in un array? Penso che tu abbia avuto problemi più grandi di poche righe duplicate. Sembrerebbe che tu abbia provato a usare OOP senza essere realmente preparato per questo. Il tipico risultato è il codice spaghetti con gli oggetti.

Se desideri un'analisi più approfondita del tuo codice ti suggerisco di scrivere un post su Revisione del codice .