6
\$\begingroup\$

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:

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 .= '<option value="'.$value.'">'.$value.'</option>';
        }

        return $this->option;
    }


    public function BuildSelect() {
        $bs  = '<select name="select_brwoser">';
        $bs .= $this->option;
        $bs .= '</select>';

        return $bs;
    }
}

The output:

require_once("class.php");

$n = new completeSelect(array("Mozilla","Opera","Internet Explorer"));
echo $n->buildSelect();
\$\endgroup\$
5
  • \$\begingroup\$ I found one that said to build a select input that is generated and populated with options by the object. I'm wondering if it's your code ? \$\endgroup\$
    – Marc-Andre
    Commented Mar 4, 2014 at 18:11
  • \$\begingroup\$ i just wrote it :P \$\endgroup\$
    – PhpJunior
    Commented Mar 4, 2014 at 18:13
  • \$\begingroup\$ Superfluous whitespace between the last two functions in the class is slightly annoying (no biggie), I don't do PHP so can't quite comment about the rest - looks pretty neat to my non-PHP eyes. \$\endgroup\$ Commented Mar 4, 2014 at 18:14
  • \$\begingroup\$ @PhpJunior Good, was just making sure! +1 the context is clear and well explained! \$\endgroup\$
    – Marc-Andre
    Commented Mar 4, 2014 at 18:16
  • \$\begingroup\$ @Mat's Mug This went better than I tought. \$\endgroup\$
    – PhpJunior
    Commented Mar 4, 2014 at 18:24

3 Answers 3

4
\$\begingroup\$

I made a few changes - namely normalizing the styling (PascalCase classname, lower_case properties, camelCase methods) and spacing. I then made the buildOptions() function shorter and renamed it to generateOption() as that is more adept to what it does. I also added an InvalidArgumentException in the construct that is tripped whenever you pass in an empty array.

class CompleteSelect{
    private $options_set;
    private $select;

    public function __construct($option_values = array()){
        if(count($option_values) == 0){
            throw new InvalidArgumentException('Options set is empty in CompleteSelect::__construct()');
        }

        $this->options_set = $option_values;
    }

    public function buildSelect($name = 'select_browser'){
        $this->select = '<select name="'.$name.'">';
        foreach($this->options_set as $value){
            $this->select .= $this->generateOption($value);
        }

        return ($this->select . '</select>');
    }

    private function generateOption($value){
        return '<option value="'.$value.'">'.$value.'</option>';
    }
}

try{
    $n = new CompleteSelect();
    echo $n->buildSelect();
} catch(InvalidArgumentException $e){
    echo $e->getMessage();
}

try{
    $n = new CompleteSelect(array("Mozilla","Opera","Internet Explorer"));
    echo $n->buildSelect();
} catch(InvalidArgumentException $e){
    echo $e->getMessage();
}
\$\endgroup\$
1
  • \$\begingroup\$ You're very welcome! \$\endgroup\$
    – jsanc623
    Commented Mar 5, 2014 at 16:14
6
\$\begingroup\$

A few minors:

  1. Your spacing is a bit inconsistent (e.g. missing spaces here completeSelect{ and here $this->setIn=$val;)

  2. 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).

  3. You have a typo here select_brwoser - should probably be select_browser

  4. 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.

\$\endgroup\$
0
2
\$\begingroup\$

IMHO, it's pretty good. I would check this out:

  • The class name should be: *C*ompleteSelect

  • All the methods should be the first letter in lower case. (*b*uildSelect)

  • I think the return in the buildOptions function could be avoid.

  • What if I have more than one select in the page? They all have the same name name="select_brwoser"

\$\endgroup\$
1
  • \$\begingroup\$ well , this is an exercise so the last point does not really matter at the moment , i guess every select with it's own object giving it it's name :-? , anyway i've noted your remarks , thanks for your feedback \$\endgroup\$
    – PhpJunior
    Commented Mar 5, 2014 at 16:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.