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();
}
}