Originally Posted by
beetle
What? Why does this raise a flag for you? One of the basic powers of delegating processes to functions (et methods) is the ability to receive a return value. Be it a success state, value transformation, or something else altogether like a Factory.
Take the City/Citizen example I gave above. Say the reason I need to get all private citizens named John is because marketing has told me that citizens like this are the most likely to buy my product and I should send them a email. So I write this code.
PHP Code:
class City {
private $citizens;
private $altitude;
private $name;
public function getPrivateCitizensNamedJohn() {
$result = array();
foreach($this->citizens as $citizen) {
if($citizen->isPrivateCitizenNamedJohn()) {
$result[] = $citizen;
}
}
return $result;
}
}
class Citizen {
private $name;
private $email;
private $isPrivate;
public function isPrivateCitizenNamedJohn($citizen) {
return ($this->isPrivate && $this->name == 'John');
}
public function getEmail() {
return $this->email;
}
}
$city = $datasource->getCityNamed('London');
$privateJohns = $city->getPrivateCitizensNamedJohn();
$productEmail = new ProductEmail();
foreach($privateJohns as $privateJohn) {
$emailAddress = $privateJohn->getEmail();
$productEmail->sendTo($emailAddress);
}
Code seems pretty good right? I think it could be better by removing as many functions that return anything and make them do something. Maybe something like this:
PHP Code:
class City {
private $citizens;
private $altitude;
private $name;
public function sendEmailToCitzensMatching($specification) {
foreach($this->citizens as $citizen) {
$specification->sendEmailTo($citzen);
}
}
}
class CitizensNameJohnEmailSpec {
public function sendEmailTo($citizen) {
if($citizen->isPrivateCitizenNamedJohn()) {
$citizen->sendEmail($this->productEmail);
}
}
}
class Citizen {
private $name;
private $email;
private $isPrivate;
public function isPrivateCitizenNamedJohn($citizen) {
return ($this->isPrivate && $this->name == 'John');
}
public function sendEmail($email) {
$email->sendTo($this->email);
}
}
$city = $datasource->getCityNamed('London');
$spec = new CitizensNameJohnEmailSpec(new ProductEmail());
$city->sendEmailToCitizensMatching($spec);
This code does the same thing except we have alot more flexibility. This flexibility is gained by telling the method to do something instead of asking it for something. Pass in any spec to the sendEmailToCitizensMatching and it will send an email to those people matching that spec. You no longer need the getEmail method in Citizen you just pass the email you want to send. etc.
Not all methods that return something are bad but like above they are something that should be looked at.
Where on earth did you get that? I'm trying to learn something here. Being pedantic gets us nowhere.
Bookmarks