Esercizio con Dependency injection

[salvatore99]

Salve forum,
mi e' stato chiesto di svolgere un piccolo esercizio utilizzando la dependency injection ed essendo un pattern a me sconosciuto sto avendo delle difficolta' nel farlo. Vorrei che qualcuno riesca a mettermi almeno sulla giusta strada.

Questo e' il testo dell'esercizio:
"Creare un semplice report che mostri le transazioni per un customer id specificato come argomento da linea di comando.
Il file data.csv contiene dati di esempio in varie valute [customer,date,value] (il primo carattere di value e' il simbolo della valuta), il report dovra' mostrare i valori in EUR.
Non e' necessario che l'applicazione utilizzi un webservice di cambio valuta reale, un client finto che ritorna valori random o fissi e' sufficiente.
Puoi utilizzare framework o librerie gia' pronte se lo ritieni opportuno. Se utilizzi codice di terze parti ti consigliamo di utilizzare composer.
Il task sara' valutato sulla base del tuo utilizzo della programmazione OO, della dependency injection (ove opportuno) e della leggibilita' e manutenibilita'."

Il codice fornito e' il seguente:

• report.php

foreach ($customer->getTransactions() as $transaction) {}

• customer.php

class Customer {
    public function getTransactions() {}
}

• CurrencyWebservice.php

class CurrencyWebservice {
    public function getExchangeRate($currency) {}
}

• CurrencyConverter.php

 class CurrencyConverter {
    public function convert($amount) {}
}

Io l'unica dipendenza che vedo e' fra CurrencyConverter e CurrencyWebservice in quanto CurrencyConverter ha bisogno di una valore per la conversione che e' gestito da CurrencyWebservice (per questo pensavo di usare una soluzione con un array del genere array('£'=>1.2,'$'=>0.95,'€'=>1). Se mi dite che il ragionamento fila inizio a buttare giu' il codice.

Grazie in anticipo

[M4V1]

Si il tuo ragionamento è corretto, CorrencyConverter dipende da CurrencyWebservice, usando la dependency injections fai si che a fronte di una interfaccia condivisa tu possa modificare CurrencyWebservice senza modificare il resto del codice.
Butta giù qualcosa e vediamo.

[salvatore99]

Ciao m4V1, grazie per avermi risposto. Sono riuscito a far funzionare il tutto solo che non so se ho rispettato il pattern e alla fine ho utilizzato una doppia dipendenza; oltre a quella fra CurrencyConverter e CurrencyWebservice ho creato anche quella fra Customer e CurrencyConverter. Di seguito il codice:

require_once("./models/Customer.php");

$id = $argv[1];


if(!isset($id) || !is_numeric($id)) {
    echo "Argomento non valido. Esecuzione interrotta\n";
    echo "Si prega di inserire un intero\n";
    exit;
}




$customer = new Customer($id, new CurrencyConverter( new CurrencyWebservice() ));


foreach ($customer->getTransactions() as $transaction) {
    echo $transaction."\n";

}

require_once("./models/CurrencyConverter.php");

class Customer
{
    private $id;
    private $currencyConverter;
    
    public function __construct($id, CurrencyConverter $currencyConverter) {
        $this->id = $id;
        $this->currencyConverter = $currencyConverter;
    }


    public function getTransactions()
    {
        $i = 1;
        $result = array();
        if (($handle = fopen("data.csv", "r")) !== FALSE) {
            while (($data = fgetcsv($handle)) !== FALSE) {


                if($i>1) {
                    $line = explode(";",$data[0]);
                    
                    if($line[0] == $this->id) {
                        $new_value = $this->currencyConverter->convert($line[2]);
                        $row = "Transazione del $line[1]: $line[2] ==> €$new_value";
                        array_push($result, $row);
                    }
                }
                $i++;
            }
            fclose($handle);
        }
        return $result;
    }

}

require_once("./models/CurrencyWebservice.php");



class CurrencyConverter
{
    
    private $webservice;
    
    public function __construct(CurrencyWebservice $currencyWebservice) {
        $this->webservice = $currencyWebservice;
    }
    
    public function convert($amount)
    {
        $amount = str_replace('"', "", $amount);
        $currency = substr($amount,0,1);
        $value = substr($amount,1);
        
        $rate =    $this->webservice->getExchangeRate($currency);
        
    return $rate*$value;
    }

}

class CurrencyWebservice{


    private $currencies = array("s"=>1.20,"$"=>0.95,"e"=>1);


    public function getExchangeRate($currency)
    {
    if (array_key_exists($currency, $this->currencies)) {
        return $this->currencies[$currency];
    } else {
            return 0;
        }
    }

}

Spero sia corretto

[M4V1]

Si direi che va quasi bene, io però non creerei la dipendenza tra Customer e CurrencyConverter, sono due cose che concettualmente non sono legate quindi meglio dividere i compiti, io farei così:

L'oggetto Customer restituisce le transazioni
L'oggetto CurrencyConverter prende una transazione e la converte

Questo rende il codice più modificabile ed espandibile.

Ti faccio un esempio, se io volessi in un secondo momento creare un report con due valute cosa dovrei fare?
Con il tuo codice dovrei creare un nuovo CurrencyConverter per la nuova valuta ma anche riscrivere la classe Customer per lavorare con due CurrencyConverter diversi, e rimodificarla di nuovo se un giorno ne volessi 3 o 4.

Togliendo la dipendenza volendo aggiungere un'altra conversione basterebbe soltanto creare un CurrencyConverter per la nuova valuta e aggiungerlo una volta nel foreach che genera il report.

E' sempre bene pensare a cosa potrebbe essere aggiunto o modificato, e strutturare il nostro codice in modo tale che questo richieda il minor numero possibile di modifiche agli oggetti già creati.

p.s. per essere più "moderno" potresti mettere tutto dentro un namespace e usare un autoloader, visto che è un esercizio credo che ti farebbe guadagnare qualche punto. Controlla anche che tutto il codice sia psr-2 perché al momento non è così.

[salvatore99]

Quindi è più corretto così?

class Customer{
    private $id;
    
    public function __construct($id) 
    {
        $this->id = $id;
    }
    
    public function getTransactions()
    {
        $i = 1;
        $result = array();
        $currencyConverter = new currencyConverter(new CurrencyWebservice());

        if (($handle = fopen("data.csv", "r")) !== false) {
            while (($data = fgetcsv($handle)) !== false) {


                if($i>1) { 
                    $line = explode(";",$data[0]);
                    
                    if($line[0] == $this->id) {
                        $new_value = $currencyConverter->convert($line[2]);
                        $row = "Transazione del $line[1]: $line[2] ==> €$new_value";
                        array_push($result, $row);
                    }
                }
                $i++;
            }
            fclose($handle);
        }
        return $result;
    }
}

[M4V1]

No così è come prima, anzi peggio perché non fai neanche la injection.

CurrencyConverter non lo devi usare dentro alla classe Customer, Customer deve restituire un elenco di transazioni non convertite che saranno poi passate al CurrencyConverter, ma fuori dall'oggetto Customer.

Idealmente la conversione avverrà durante la generazione del report, qualcosa di questo tipo:

require_once("./models/Customer.php");

$id = $argv[1];


if(!isset($id) || !is_numeric($id)) {
    echo "Argomento non valido. Esecuzione interrotta\n";
    echo "Si prega di inserire un intero\n";
    exit;
}

$customer = new Customer($id);
$CurrencyConverter = new CurrencyConverter( new CurrencyWebservice() );

foreach ($customer->getTransactions() as $transaction) {
    $converted = $CurrencyConverter->convert($transaction);
    echo $converted . "\n";
} 

[salvatore99]

Che disastro... Adesso l'ho corretto come mi hai suggerito. Ti ringrazio davvero tanto