Parsing credit info

Question

The main class (SMSHelper) is responsible of query a REST web service which in turn returns an XML string. This helper is going to return a new SMSCreditInfo instance (merely a data container), with the help of a parser (SMSCreditParser):

class SMSHelper
{
    protected static $base = 'https://gateway.skebby.it/api/send/smseasy/advanced/rest.php';

    /**
     * @return SMSCreditInfo
     */
    public function getCreditInfo()
    {
        // Setting POST data
        $data = array();
        $data['method']   = 'get_credit';
        $data['username'] = $this->username;
        $data['password'] = $this->password;

        // Inizialize SMSCreditInfo container, assuming a failure
        $creditInfo = new SMSCreditInfo();
        $creditInfo->setIsSuccess(false)->setCreditLeft(null);

        // Actually send the request
        $response = $this->browser->post(self::$base, array(), http_build_query($data));

       // If it's a failure return SMSCreditInfo
       if(200 != $reponse->getStatusCode()) return $creditInfo();

       // Ask the parser to parse the XML response
       return SMSCreditParser::parse($reponse->getContent());
    }

}

Class SMSCreditParser is actually responsible of parsing the XML string:

class SkebbyCreditParser
{

    /**
     * @return SMSCreditInfo
     */
    public static function parse($xmlString)
    {
        $info = new SMSCreditInfo();

        // Failure in parsing XML string
        if(false === ($xml = @simplexml_load_string($xmlString)))
            return $info->setIsSuccess(false)->setCreditLeft(null);

        // Get credit node value
        ...
    }

}

Does this code follow loose coupling pattern or should be refactored/re-designed?

I don't like how these classes are organized and I think they're not following the loose coupling pattern at all. The main reason is because both getCreditInfo() and parse() return the container object, and are not DRY due to the way the container is initialized in case of failure.

Answer 1

I hope you're still watching this. Answers here at codereview tend to take a while. I've been on vacation, else I would have answered sooner. Hope this answer satisfies :)

"i think it's not following the loose coupling pattern at all." - You'd be correct.

"not DRY too" - Also correct.

It would help if you were to use a design pattern that used loose coupling. What you currently have does not appear to use a pattern at all. The best one for this, I think, is the MVC pattern and is what I'll use in my answer. I'd also like to point out that you are not breaking up your methods as much as you probably should. Methods should do one thing and do it well. I went ahead and broke them up in my answer.

SMSHelper

First, I broke up your methods into smaller, more focused, pieces. Methods and functions help clean up your code and are not just for reusability. Breaking up your methods in this manner will make debugging, reading, and future expansion much easier.

Second, I removed any instance of any other class as this violates loose coupling principle. You'll notice I left a note in the code about how $this->browser possibly violates this principle as well. To correct this, I would change the new _getResponse() method to public and call it in the new SMSController class (below) and have it return an array to pass to the $browser->post() method which I would also instantiate in the SMSController class. However I left it how it is because you asked specifically about the other two classes and didn't give me enough to be sure how to implement it in the controller.

Third, I made the getCreditInfo() method return FALSE on "failure" so it can be handled in the controller. This helps you to comply with the DRY principle as before you were not reusing the code.

class SMSHelper {
    private function _getData() {
        return array(
            'method' => 'get_credit',
            'username' => $this->username,
            'password' => $this->password,
        );
    }

    private function _getResponse() {
        return $this->browser->post(//not sure what browser is, but it probably violates loose coupling as well
            self::$base,
            array(),
            http_build_query($this->_getData())
        );
    }

    public function getCreditInfo() {
        $response = $this->_getResponse();

        if($response != 200) { return FALSE; }
        return $response->getContent();
    }
}

SMSCreditParser

Again, I removed any instance of other classes, for the same reasons.

Also, I changed the way you parsed the XML. First off, it was very cluttered. You shouldn't define variables in statements. There are some exceptions that are accepted, but this isn't one. Second, the statement you had was unnecessary. Load the string, if it has any errors it will return FALSE, so you can just use the XML variable as your statement.

I also returned FALSE here on "failure" so it can be handled in the controller.

class SMSCreditParser {
    public static function parse($xmlString) {
        $xml = simplexml_load_string($xmlString);

        if($xml) {
            //etc...
            return $creditNodeValue;
        }
        return FALSE;
    }
}

Controller

Here's the controller class that brings them all together. You start off by calling the getCreditNode() in your constructor before loading the view. This method loads the SMSHelper::getCreditInfo() method. I don't do anything if it returns FALSE because it is unnecessary. Trying to load FALSE into simplexml should just return FALSE. Something tells me it might through an error, so I say should. Sense there is no reason to check its value I immediately pass it to the SMSCreditParser::parse() method. If parse() returns FALSE, then the new _failure() method will be called to set the SMSCreditInfo class and return it.

class SMSController {
    public function __construct() {
        //all your includes for each class, or your autoloader
        $creditNode = $this->getCreditNode();
        $this->render($view);
    }

    private function _failure() {
        $SMSCreditInfo = new SMSCreditInfo();
        $SMSCreditInfo->setIsSuccess(false)->setCreditLeft(null);

        return $SMSCreditInfo();
    }

    public function getCreditNode() {
        $SMSHelper = new SMSHelper();

        $creditInfo = $SMSHelper->getCreditInfo();
        $parsed = SMSCreditParser::parse($creditInfo);

        if($parsed) { return $parsed; }
        return $this->_failure();
    }
}