4
\$\begingroup\$

Recently, I've been trying to transform my code to be more OO and adhere to SOLID principles. I also have been trying to get into the habit of making my code more testable and have been trying to implement a TDD mindset. Here is a snippet of my code from my class ChargebackReport:

/**
 * Populates the properties of this class (ChargebackReport) using CamsModel
 * which is a database model responsible for executing queries and returning
 * the required information. GeoIPUtils is a class used to populate the
 * loginIpInformation attribute by making a GET request to an API. 
 */
public function generateReport() {
    $this->transactions = $this->camsModel->getLastIntTransactionsByUser($this->userId, 20);
    $this->loginHistory = $this->camsModel->getLastIntLoginsByUser($this->userId, 100000);
    $this->chargebacks = $this->camsModel->getChargebacksByUser($this->userId);
    $this->chargebackTotal = $this->camsModel->getChargebackValueByUser($this->userId);

    $geoUtils = new GeoIPUtils();
    $loginIps = $this->camsModel->getLoginIpsFromLoginHistory($this->loginHistory);
    $this->loginIpInformation = $geoUtils->getIPAddressInfo($loginIps);

    $resultsToMap = [
        "userId" => $this->camsModel->getIDUrl($this->userId),
        "user"   => $this->camsModel->getUserById($this->userId),
        "cc"     => $this->camsModel->getCCInfoByUser($this->userId, $this->chargebacks)
    ];

    foreach($resultsToMap as $result) { $this->mapResultsToProperties($result); }
}

The responsibility of this function is to populate the various properties of this class. Some properties can be directly populated from the query. Other queries return arrays of information, which needs to be parsed through and assigned via the mapResultsToProperties() method, which maps variable keys to their respective property within the class.

Now, it's apparent that this function has a lot of stuff going on, but its overall goal is to just populate the fields in the class using the CamsModel and another class called GeoIPUtils which returns information on a given IP. The function is not responsible for querying and making GET request to an API, so I delegated those responsibilities to other classes.

Am I applying the SOLID principles properly in this example?

And for my second question, how would I go about making this class unit testable? The function as is doesn't return anything, nor are any exceptions thrown which could potentially halt the execution of it. It seems like it wouldn't be beneficial to have the function return true and just making an assert akin to the following:

$this->assertTrue($chargebackReport->generateReport());

I don't know how beneficial it would be to mock every single method in the CamsModel class (I call 8 different functions from CamsModel), have it return random input, and just checking if it assigned properly.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! Please write a title that describes what your code does in a few words rather than what you would like to improve about it. We all want to make our code better here so it is implied in the question. \$\endgroup\$ – Phrancis Jan 7 '16 at 16:17
1
\$\begingroup\$

GeoIPUtils being a utility of methods, with the CamsModel being more of an object? camsModel could then accept an instance of GeoIPUtils rather than assigning in the separate procedural logic below:

$geoUtils = new GeoIPUtils();
$loginIps = $this->camsModel->getLoginIpsFromLoginHistory($this->loginHistory);
$this->loginIpInformation = $geoUtils->getIPAddressInfo($loginIps);

CamsModel always benefit from a GeoIPUtils instance? If so could be in the constructor. If wish to keep CamsModel, GeoIPUtils responsibilities better separated, add more indirection with a separate object which uses instances of CamsModel and GeoIPUtils as needed. Refactor generateReport into a Report object which requires a CamsModel instance, a GeoIPUtils instance, and your current this state. Creating a new Report object would replace:

$this->camsModel->getLastIntTransactionsByUser($this->userId, 20);

with:

report.getTransactions();

or:

report.getTransactions(20);

If the CamsModel class changed, then your current this or class in the question code above wouldn't need to be modified. Suppose the new version of CamsModel did not have a getIDUrl method. Only Report would be concerned with the details of CamsModel. Your this would depend only upon whatever methods Report provides. GeoIPUtils and CamsModel are now divorced from your current this class.

Replace 20, 100000 and any other literals in code with meaningful constants.

report.getTransactions(USERID_LIMIT);

Writing unit tests would be done first before crafting the object or method code, except for empty placeholder stub classes and methods. Get known input, desired output or object state for the non-existent class or method. Write tests which require that output or object state.

unit tests begin with comparison for equality. Simple String or other literals of the desired output or values are compared with your new object's envisioned getter methods, if( report.getChargebackTotal() != 959.30) failTest("Charge back total invalid");. Once unit test is written, then at this point begin writing the actual code to produce that output.

Will rewrite this code repeatedly, adding new objects and methods, painfully deleting code just typed, but writing the unit tests first is well worth the effort. Add unit tests for unexpected input, boundary testing, very small, large, negative values. Use regular expressions to whitelist and reject all input before validating by blacklisting. Protect against buffer overflow, exceeding array boundaries, numeric and String types. Modify code to validate all input, all return values. Fail immediately whenever out of range or unexpected.

\$\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.