3
\$\begingroup\$

I would like to know whether I applied the SOLID principle and the repository pattern the right way.

My code should derive exchange rates from a database and do basic currency conversions.

The database table is very easy. I assume I have a base currency and all exchange rates are against the base currency. So the database table "exchangerate" is just like this:

CREATE TABLE exchangerate (currency varchar(3) PRIMARY_KEY,    
exchangerate INT NOT NULL);

I also have set the base currency as a global variable (actually it could be a constant too):

$GLOBALS["basecurrency"] = "EUR";

I have a currency repository class that talks to the database. I have intentionally not used an ORM (after a months of review of different ORMs, I think, I will still code my own SQL queries). And I have intentionally not used an interface, because I cannot think of any reason why I ever would need another implementation of the currency repository than just fetching the rates from the database. Of course both decisions can be criticised, I would be interested in feedback on this. My reason for not using an ORM is because I have highly efficient SQL queries that have 50 lines. I am just not keen to rewrite them into some ORM logic just to find out, that the performance is horrible:

<?php

namespace Bjc\Iis\Core;

use Bjc\Iis\Core\Db;

class currencyrepository {

    public $rows;

    function getAll() {

        $db = new Db();
        $db->query('SELECT currency, exchangerate FROM exchangerate');
        $rows = $db->resultset();
        return $rows;
    }
}

I have a class that will serve the exchange rates. This is my data object. I planned to have all exchange rates in one object, so I set this to a singleton because there is no need to have several instances of this class. I would also like to have feedback if it is a wise decision to hold all the exchange rates in one object and if the use of a singleton is correct here.

The data object also incorporates some business logic, because if the requested target and source currency both are not the base currency, the class will compute an "artificial" exchange rate between those too:

<?php

namespace Bjc\Iis\Core;

class currencyconversionrate {

    private final function __clone() { }
    public final function __sleep() {
        throw new Exception('Serializing of Singletons is not allowed');
    }

    static private $instance = null;

    public function __construct() {

        $currencyrepository = new currencyrepository();
        $rows_get_exchangerate = $currencyrepository->getAll(); 

        foreach ($rows_get_exchangerate as $row) {
            $currency = $row["currency"];
            $conversionratearray["$waehrung"] = $row["kurs"];
        } // ende foreach ($rows as $row)

        $this->conversionratearray = $conversionratearray;

    } // ende function __construct()

public static function getInstance() {
       if (self::$instance === null) self::$instance = new self;
       return self::$instance;
    }

    public function checkCurrency($currency) {

        foreach ($this->conversionratearray as $checkcurrency => $checkconversionrate) {
             if ($checkcurrency == $currency) {
                $checked = 1;
             } // ende if ($checkcurrency == $currency)
        } // ende if (foreach ($this->conversionratearray ...)

        if ($currency == $GLOBALS["basecurrency"]) {
            $checked = 1;
        } // ende if ($currency == $GLOBALS["basecurrency"])

        if ($checked != 1) {
            throw new Exception('No valid currency.');
        } // ende if ($checked != 1)

    } // ende function checkCurrency($currency)

    public function getConversionrate($sourcecurrency,$targetcurrency) {

        try {
            $this->checkCurrency($sourcecurrency);
            $this->checkCurrency($targetcurrency);

            if ($targetcurrency == $sourcecurrency) {
                $conversionrate = 1;
            } elseif ($GLOBALS["basecurrency"] == $targetcurrency) {
                $conversionrate = 100 / $this->conversionratearray["$sourcecurrency"];
            } elseif ($GLOBALS["basecurrency"] == $sourcecurrency) {
                $conversionrate = $this->conversionratearray["$targetcurrency"];
            } else {
                $conversionrate = (100 / $this->conversionratearray["$sourcecurrency"]) * $this->conversionratearray["$targetcurrency"];
            } // ende if ($this->targetcurrency == $this->sourcecurrency)

        } catch (Exception $e) {
            $GLOBALS["htmloutput"] .= $e->getMessage();
            $conversionrate = 0;
        } // ende try/catch

        return $conversionrate;

    } // ende function getConversionrate()

} // ende class currencyconversionrate

And finally I have a class that does the conversion of a certain amount:

<?php

namespace Bjc\Iis\Core;

class currencyconvertamount {

    public function setAmount($amount) {
        $this->amount = $amount;
    }

    public function setSourceCurrency($sourcecurrency) {
        $this->sourcecurrency = $sourcecurrency;
    }

    public function setTargetCurrency($targetcurrency) {
        $this->targetcurrency = $targetcurrency;
    }

    public function convert() {

        $currencyhandler = currencyconversionrate::getInstance();
        $conversionrate = $currencyhandler->getConversionrate($this->sourcecurrency,$this->targetcurrency);

        $convertedamount = round($this->amount * $conversionrate,2);
        return $convertedamount;

    } // ende public function convert()

} // ende class currencyconvertamount

Am I doing correct OOP here?

\$\endgroup\$
  • \$\begingroup\$ Welcome to CodeReview Benedikt. Quite the daunting question here. Mind adding in some bold/italics to focus in on what you want? \$\endgroup\$ – Legato Apr 12 '15 at 22:42
1
\$\begingroup\$

About the problem itself, you might want to have a look at https://github.com/florianv/swap. It does what you are trying to do, if so why reinvent the well.

Regarding your code..

  1. currencyrepository shouldn't create db connection, it should be passed via constructor.
  2. public properties ($rows) are evil, they can be easily overwritten and this breaks your cache. Make it protected/private and add getter public function getExchangeRates(){...}. (ad. just noticed it is not used, but if you plan to, do it this way or probably the better way - forget the getter and return it in the main call if its set. This will simplify things.)
  3. Singleton is almost never the correct answer and often is considered an antipattern. Use Registry or Dependency Injection Container to maintain objects.
  4. Again in currencyconversionrate you create your dependency in constructor, pass it as an argument.
  5. globals in any form are NO, NO, base currency should be a constant (as you mentioned it yourself) of the main class (probably currencyconvertamount, though it needs some improvements)
  6. $conversionratearray["$waehrung"] = $row["kurs"]; this line is broken, neither your table has kurs column nor $waehrung is defined.
  7. currencyconversionrate seems a bit off in general, not sure if it shouldn't be a part of currencyconvertamount as internal method (protected / private)
  8. currencyconvertamount is broken in the meaning that it won't work unless you set amount, source and target currency before calling convert and you do not do any safety checking. Either way it is not the way to go. Those setter should be removed and the convert method should get them as parameters. Better way would be to check Money pattern and /or use one of the php implementations mathiasverraes/money or sebastianbergmann/money.
  9. currencyconvertamount should get currencyconversionrate via constructor
  10. Your naming is not consistent, check PSR-1 and PSR-2 for a commonly used styling guidelines. There are tools to help you with it PHP-CS-FIXER
  11. I'd rename currencyconvertamount to CurrencyConverter bcos thats what it does in general.

It is not exhaustive list, probably only scratches top of the iceberg.

In general for better understanding of oop I recommend reading about few rulesets / principles:

Where the SOLID one is really the main thing, and will enforce others when implemented correctly.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.