I'm busy with refactoring of one class and now have doubt how to refactor 2 methods. Here they are:
public function transform($transformXml, $importXml, $xsdScheme = '')
{
...
if (!empty($xsdScheme)) {
$this->_validateXml($exportDoc, $xsdScheme);
}
...
}
protected function _validateXml(DOMDocument $xml, $xsdScheme)
{
...
if (!file_exists($xsdScheme)) {
throw new Exception('XSD file was not found in ' . $xsdScheme);
}
...
}
Parameter $xsdScheme
at method transform
is optional, if it's empty than we won't apply xsd validation. After it we call method _validateXml
, where we are checking if file_exists
. This validation is split on 2 parts and I don't like it, I prefer it at one place. So, I would write something like this:
public function transform($transformXml, $importXml, $xsdScheme = '')
{
...
if (!empty($xsdScheme)) {
if (!file_exists($xsdScheme)) {
throw new Exception('XSD file was not found in ' . $xsdScheme);
}
$this->_validateXml($exportDoc, $xsdScheme);
}
...
}
protected function _validateXml(DOMDocument $xml, $xsdScheme)
{
...
...
}
Is it a good approach? If no, why?
I'm thinking you may want to keep your current structure. Methods should really be designed to do one thing. Your _validateXml method checks to see if the scheme exists, which sounds like something a XML validation method should do. Checking for the scheme is not necessarily something the transformation method needs to do as scheme is optional.
If you think about the responsibilities of each method it should be easier to understand this.
The transform function does transformation, and delegates the validation to the other method. Transform only knows that it needs to pass the name of the file to the validator function - it doesn't really care what the validator function does with it.
Thus, in my opinion, it would make sense to keep the file related checks etc. in the validation function. Or, if you want to take the refactoring futher, maybe split the file loading and file checks into another method, or create an entirely separate class for performing validation.