Is it poor technique to have functions within a class depend on one another in a "cascade" type fashion?

Question

I am fairly new to PHP and OOP. Having said that, I wanted to know how independent functions should be within a class. I know that each function should be responsible for doing just one thing. However, as you can see in my class below, I occasionally find myself creating functions that build upon one another in a "cascade" type fashion.

Is this good practice? Is there a different way I should be structuring this class/these types of functions? I want to make sure I am developing good habits.

Thanks in advance for your help.

class PubMedQuery {

private $query;
private $searchParameters;
private $searchURL;
private $fetchParameters;
private $fetchURL;
private $searchResults;
private $fetchResults;
private $matches;
private $matchRegex;
private $emailAddresses;

public function __construct($query) {
    $this->query = $query;
}

public function setSearchParameters() {
    $this->searchParameters = array(
        'db'         => 'pubmed',
        'term'       => $this->query,
        'retmode'    => 'xml',
        'retstart'   => '0',
        'retmax'     => '1000',
        'usehistory' => 'y'
    );
}

public function getSearchParameters() {
    return $this->searchParameters; 
} 

public function setFetchParameters() {
    $this->fetchParameters = array(
        'db'        => 'pubmed',
        'retmax'    => '1000',
        'query_key' => (string) $this->searchResults->QueryKey,
        'WebEnv'    => (string) $this->searchResults->WebEnv
    );
}

public function getFetchParameters() {
    return $this->fetchParameters; 
} 

public function setSearchURL() {
    $this->baseSearchURL = 'http://eutils.ncbi.nlm.nih.gov/entrez/eutils /esearch.fcgi?';
    $this->searchURL = $this->baseSearchURL . http_build_query($this->getSearchParameters());
}

public function getSearchURL() {
    return $this->searchURL; 
}

public function setFetchURL() {
    $this->baseFetchURL = 'http://eutils.ncbi.nlm.nih.gov/entrez/eutils/efetch.fcgi?';
    $this->fetchURL = $this->baseFetchURL . http_build_query($this->getFetchParameters());
}

public function getFetchURL() {
    return $this->fetchURL; 
}

public function setSearchResults() {
    $this->setSearchParameters();  
    $this->setSearchURL();
    $this->searchResults = simplexml_load_file($this->getSearchURL());
}

public function getSearchResults() {
    $this->setFetchParameters();
    $this->setFetchURL();
    return file_get_contents($this->getFetchURL()); 
}

public function setEmailAddresses() {
    $this->matches = array();
    $this->matchRegex = '/[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,4}/'; 
    preg_match_all($this->matchRegex, $this->getSearchResults(), $this->matches);
    $this->emailAddresses = array_unique(array_values($this->matches[0]));
}

public function getEmailAddresses() {
    $this->setSearchResults();
    $this->getSearchResults();
    $this->setEmailAddresses();
    return $this->emailAddresses;
}
}

//Example using search term "psoriasis"
$query  = new PubMedQuery('psoriasis');
echo implode('<br />', $query->getEmailAddresses());

Answer 1

What you have looks fine.

You have several actions setEmailAddresses, getEmailAddresses each of which comprise of a series of steps. Some of these steps are common across multiple actions so it makes perfect sense to factor them out into their own methods.

The remaining part of the action could be coded in the top level method, but having it in it's own "sub method" is not a problem - in fact it could be said it's better practice as you are keeping each method responsible for just one thing.

Answer 2

To answer your question, no, it's not at all bad practice to divide the internals of a class across multiple methods as you've done here. As the other answers and comments have said, it's actually good practice.

You're on the right track, but I did see a couple of things that I think need attention. From the example you give outside the class definition, it appears that the primary use of objects derived from this class is (roughly) to extract email addresses from a PubMed query. You've divided the general tasks needed to do this throughout the class appropriately; however, many of these tasks merely set some immutable information. Do these sorts of methods, e.g. setSearchParameters(), really need to be accessible outside the class? I would argue that no, they don't- because most of these methods do nothing beyond getting or setting private member variables.

I bring up the whole encapsulation issue because one way you could improve your code would be to drop some of the private member variables unless you absolutely need them. I don't know the requirements of your project, but it appears to me that each PubMed query being made is represented by its own object. If so, is it necessary to make, for instance, the $searchParameters variable a class member? Why not simply do something like:

public function setSearchParameters() {
    return array(
        'db'         => 'pubmed',
        'term'       => $this->query,
        'retmode'    => 'xml',
        'retstart'   => '0',
        'retmax'     => '1000',
        'usehistory' => 'y'
    );
}

And then, for the next method in the sequence:

public function setSearchURL() {
    $baseSearchURL = 'http://eutils.ncbi.nlm.nih.gov/entrez/eutils /esearch.fcgi?';
    return $baseSearchURL . http_build_query($this->setSearchParameters());

}

Why do this? Try to think about what a member variable really is- it's a representation of the state of the object in question. The $query member makes sense; it represents the basic state of the PubMedQuery object. Working backwards from the top-level method, getEmailAddresses(), everything in the object essentially depends on the $query member. At least in the code you've provided, the other methods simply operate on this member variable in succession. (I'd probably set the $baseSearchURL variable as a constant instead of a variable, but that's beyond the scope of this answer).

Removing the $searchParameters member variable and structuring your code more like what is shown above also gets rid of the need for the getter method, which reduces coupling. If for some reason another object needs the non-member $searchParameters variable produced by the setSearchParameters method, you can keep that method public. If you're sure no other objects will need it, make it private or protected. Again, I don't know your requirements, but I suspect nothing beyond your top-level method really needs to be public. To further reduce coupling, if you do make only your top-level method public, you can define it as an interface method. This way, objects that depend on your PubMedQuery object can depend on the interface instead.

I generally tend to try and rethink the design of a class if I wind up with more than a few member variables. If it turns out I really need an object to keep track of that many states, I start thinking that maybe I might want to refactor some of the code of that object into other objects. An object should only have one responsibility. I'm sure cases exist, but I personally have never had to make an object that needed to keep track of more than four or five member variables at most in order to fulfill that responsibility.

Overall, you're definitely on the right track. Subdividing the overall responsibility of an object into multiple methods that depend upon one another within a class is definitely good practice. What I've discussed here is simply taking things to the next level.