patternphpMinor
Select input that is generated and populated with options
Viewed 0 times
populatedgeneratedwithoptionsinputthatandselect
Problem
I started to learn PHP OOP recently and searched the web for some practical exercises and I found one that said to build a select input that is generated and populated with options by the object. I just want to ask for some opinions on code quality. Could it be done better? What is the biggest flaw?
The class:
The output:
The class:
class completeSelect{
private $setIn;
private $option;
public function __construct($val = array()) {
$this->setIn=$val;
$this->buildOptions();
}
public function buildOptions()
{
$this->option = "";
foreach($this->setIn as $value)
{
$this->option .= ''.$value.'';
}
return $this->option;
}
public function BuildSelect() {
$bs = '';
$bs .= $this->option;
$bs .= '';
return $bs;
}
}The output:
require_once("class.php");
$n = new completeSelect(array("Mozilla","Opera","Internet Explorer"));
echo $n->buildSelect();Solution
A few minors:
-
Your spacing is a bit inconsistent (e.g. missing spaces here
-
Your naming convention is a bit inconsistent as well. I don't do much PHP but class names are usually
-
You have a typo here
-
Your interface into the class is not quite clear. You build your options on construction yet the method to do so is public. Am I supposed to call it again - if so under which circumstances? If the class is immutable (e.g. you can't add more values for the options) then why not simply build the entire select in the first place and cache it?
A possible change would be to make the
-
Your spacing is a bit inconsistent (e.g. missing spaces here
completeSelect{ and here $this->setIn=$val;)-
Your naming convention is a bit inconsistent as well. I don't do much PHP but class names are usually
PascalCase. You should decide whether your methods should be camelCase or PascalCase (you have buildOptions vs BuildSelect).-
You have a typo here
select_brwoser - should probably be select_browser-
Your interface into the class is not quite clear. You build your options on construction yet the method to do so is public. Am I supposed to call it again - if so under which circumstances? If the class is immutable (e.g. you can't add more values for the options) then why not simply build the entire select in the first place and cache it?
A possible change would be to make the
buildXYZ methods private and build on construction (assuming the class is immutable) and have public getXYZ methods instead which return the cached value for the options or the select respectively.Context
StackExchange Code Review Q#43427, answer score: 6
Revisions (0)
No revisions yet.