I understand that local variables are limited to the scope they were declared in and instance variables exist as long as the object exists. But suppose I have two classes:
PersonalInformation.php
<?php
class PersonalInformation {
private $name;
private $surname;
private $gender;
private $birthday;
function getName(){...}
function setName(){...}
//Other getter and setter
}
and PersonalInformationController.php which takes the input from a form, istantiate a PersonalInformation object and set its attributes:
class PersonalInformationController {
private $personalInformation;
function __construct() {
$this->personalInformation = new PersonalInformation();
}
function doPost() {
$this->setPersonalDetails();
$this->setResidence();
$this->setContact();
}
private function setPersonalDetails() {
$name = filter_input(INPUT_POST, "name");
$surname = filter_input(INPUT_POST, "surname");
$gender = filter_input(INPUT_POST, "gender");
$birthday = filter_input(INPUT_POST, "birthday");
$nationality = filter_input(INPUT_POST, "nationality");
if (empty($name) || empty($surname)) {
throw new RequiredFieldException("Name and surname can't be empty");
} else if (!is_string($name) || !is_string($surname)) {
throw new InvalidFieldException('Input must be a string!');
} else {
$this->personalInformation->setName($name);
$this->personalInformation->setSurname($surname);
}
if (!empty($gender) && is_string($gender)) {
$this->personalInformation->setGender($gender);
} else {
throw new InvalidFieldException('Input must be a string!');
}
if (!empty($birthday) && is_string($birthday)) {
$this->personalInformation->setBirthday($birthday);
}
if (!empty($nationality) && is_string($nationality)) {
$this->personalInformation->setNationality($nationality);
}
}
private function setResidence() {
$address = filter_input(INPUT_POST, "address");
$zipCode = filter_input(INPUT_POST, "zipCode");
$city = filter_input(INPUT_POST, "city");
$nation = filter_input(INPUT_POST, "nation");
if (!empty($address) && is_string($address)) {
$this->personalInformation->setAddress($address);
}
//...//
}
private function setContact() { ... }
}
This class have three main methods (setPersonalDetails() - setResidence() - setContact()) which take the input from a form, in the html page, put them in local variables (that is, $name, $surname etc..) and check on type in order to set them in the PersonalInformation object.
Here my question: "From point of view of code design(especially extensibility and readability), there exists some differences between the use of these local variables or declare them as instance variables in order to leave the three methods only for checking the type of these variables (and not take the input) ?" . And so make something like that:
class PersonalInformationController {
private $personalInformation;
private $name;
private $surname;
private $gender;
private $birthday;
private $nationality;
private $cellphone;
//Other instance variables
function __construct() {
$this->personalInformation = new PersonalInformation();
}
function doPost() {
$name = filter_input(INPUT_POST, "name");
$surname = filter_input(INPUT_POST, "surname");
$gender = filter_input(INPUT_POST, "gender");
$birthday = filter_input(INPUT_POST, "birthday");
$nationality = filter_input(INPUT_POST, "nationality");
//...
$address = filter_input(INPUT_POST, "address");
$zipCode = filter_input(INPUT_POST, "zipCode");
$city = filter_input(INPUT_POST, "city");
$nation = filter_input(INPUT_POST, "nation");
}
private function setPersonalDetails() {
// NOW THIS METHOD ONLY CHECKS THE TYPE OF THE INPUT
if (empty($this->name) || empty($this->surname)) {
throw new RequiredFieldException("Name and surname can't be empty");
} else if (!is_string($this->name) || !is_string($this->surname)) {
throw new InvalidFieldException('Input must be a string!');
} else {
$this->personalInformation->setName($this->name);
$this->personalInformation->setSurname($this->surname);
}
if (!empty($this->gender) && is_string($this->gender)) {
$this->personalInformation->setGender($this->gender);
} else {
throw new InvalidFieldException('Input must be a string!');
}
if (!empty($this->birthday) && is_string($this->birthday)) {
$this->personalInformation->setBirthday($this->birthday);
}
if (!empty($this->nationality) && is_string($this->nationality)) {
$this->personalInformation->setNationality($this->nationality);
}
}
//setResidence() and setContact() are like the previous method.
}
This question seems to be more related to application design than performance.
Instead of contemplating the performance of local vs instance variables, consider the purpose of each variable, and ask yourself Does it belong here?. Classes can become quite large. In the end, you only want to store as much information as is necessary.
Instance variables are properties of an object (name, birthday, width, height, etc). They either describe an object or contain information that the object must know about. Variables that do not meet one of those requirements should not be an instance variable. Instead, you can pass them into your methods as parameters.
In object-oriented applications, local variables are generally limited to the scope of the function (aka method) that they reside in. These variables are usually destroyed or are no longer used after the function finishes executing.
Your application contains a domain object called PersonalInformation. A domain object has a single purpose: to store information about a thing or abstract thing (such as personal information). Therefore, it should only contain properties, such as name, surname, birthday, ..., and method that directly manipulate those properties, such as getters and setters.
The domain object is also where you should place your validation logic. If you use the PersonalInformation object in multiple controllers, you will find yourself rewriting the same input validation rules across many files. This can be avoided by setting values to the properties of the object, and then validating them internally through a validate() method.
By doing this, your validation logic can be removed from your controller.
Controllers are meant to accept requests, tell other parts of the application to process POST/DELETE/PUT data, and then either render a view (such as a HTML template or JSON string) or redirect to another part of the application.
In most cases, your controllers should not handle validation logic for your domain objects.
I've refactored your code to meet these principles, and added comments to explain the purpose for the changes as I made them.
<?php
class PersonalInformationController
{
/**
* Though this controller interacts with one or more PersonalInformation objects, those objects
* do not describe this controller. Nor do they provide any additional information that the
* controller MUST know about.
*
* This variable should be local, not a class property.
*/
//private $personalInformation;
/**
* Because $personalInformation is no longer a class property,
* it does not need to be set in the constructor.
*/
function __construct() {}
function doPost()
{
/**
* The try/catch block will catch any validation exceptions thrown by the validate()
* method withing the $personalInformation object.
*/
try {
/**
* This instance of the PersonalInformation object is only being used within the scope of
* this function, so it should be a local variable.
*
* @var PersonalInformation personalInformation
*/
$personalInformation = new PersonalInformation();
$personalInformation->setName(filter_input(INPUT_POST, "name"));
$personalInformation->setSurname(filter_input(INPUT_POST, "surname"));
$personalInformation->setGender(filter_input(INPUT_POST, "gender"));
$personalInformation->setBirthday(filter_input(INPUT_POST, "birthday"));
$personalInformation->setNationality(filter_input(INPUT_POST, "nationality"));
/** Set other properties of personalInformation ... */
/**
* This validate() method will check the integrity of the data you passed into the setter
* methods above. If any of the domain object's properties are invalid, an exception will be thrown.
*/
$personalInformation->validate();
/** save the object / show view / other controller logic */
} catch(RequiredFieldException $e) {
/** A field was empty. Error handling logic here. */
} catch(InvalidFieldException $e) {
/** A field value was incorrect. Error handling logic here. */
}
}
}
/**
* Class PersonalInformation
*
* This class is a domain object. Notice that it only contains properties that describe PersonalInformation.
* All of the logic contained in this class manipulates the properties, and is "unaware" of any outside entities.
*/
class PersonalInformation {
private $name;
private $surname;
private $gender;
private $birthday;
private $nationality;
/**
* Try to avoid duplicate code. If all the validation for your domain object is the same,
* use a method, such as this, to store all of the validation logic.
*
* @throws \InvalidFieldException
* @throws \RequiredFieldException
*/
public function validate() {
if(empty($this->name) || empty($this->surname)) {
throw new RequiredFieldException("Name and surname can't be empty");
} else if(!is_string($this->name) || !is_string($this->surname)) {
throw new InvalidFieldException('Name and surname must be a strings!');
}
if(empty($this->gender) || !is_string($this->gender)) {
throw new InvalidFieldException('Gender must be a string!');
}
if(empty($this->birthday) || !is_string($this->birthday)) {
throw new InvalidFieldException('Birthday must be a string!');
}
if(empty($this->nationality) || !is_string($this->nationality)) {
throw new InvalidFieldException('Nationality must be a string!');
}
/** other validation rules **/
}
/**
* @return mixed
*/
public function getName()
{
return $this->name;
}
/**
* @param mixed $name
*/
public function setName($name)
{
$this->name = $name;
}
/**
* @return mixed
*/
public function getSurname()
{
return $this->surname;
}
/**
* @param mixed $surname
*/
public function setSurname($surname)
{
$this->surname = $surname;
}
/**
* @return mixed
*/
public function getGender()
{
return $this->gender;
}
/**
* @param mixed $gender
*/
public function setGender($gender)
{
$this->gender = $gender;
}
/**
* @return mixed
*/
public function getBirthday()
{
return $this->birthday;
}
/**
* @param mixed $birthday
*/
public function setBirthday($birthday)
{
$this->birthday = $birthday;
}
/**
* @return mixed
*/
public function getNationality()
{
return $this->nationality;
}
/**
* @param mixed $nationality
*/
public function setNationality($nationality)
{
$this->nationality = $nationality;
}
}
Notice that the application is now using less variables and functions. It only stores as much information as is necessary in each class. Therefore, your application will experience better performance and use less memory. The code is clean, simple, and easy to maintain.
Hope this helps!