Populating an HTML combo box by looped concatenation

This code takes the results of a database query and uses them to populate the options in a combobox on a form.

Would it be better to start with an empty htmlCode and then concatenate all the ` code in the loop before finally using echo outside the loop?

Also, do I need to declare $boxOption, $paramKey and $paramValue` as class variables or are they OK as is because they're only used in this method?

public function populateComboBox ($boxToPopulate)
    {
        $this->boxOptions = $this->getQueryResults($this->boxParams[$boxToPopulate]['query']);

        foreach ($this->boxOptions as $boxOption) {

            foreach ($this->htmlParams as $paramKey => $paramValue) {

                if (!empty($this->boxParams[$boxToPopulate][$paramKey])) {

                    $this->htmlParams[$paramKey] = $boxOption[$this->boxParams[$boxToPopulate][$paramKey]];
                }
            }

            $this->htmlCode = "htmlParams['value']}' "
                              . "class='{$this->htmlParams['class']}'>"
                              . "{$this->htmlParams['text']}\n";

            echo htmlCode}
_CODE;
        }

    }

I use an array for each combobox on the form:

$this->boxParams['issue'] = array (
        'query' => 'SELECT interface_issue, current_issue '
                 . 'FROM interface_issue '
                 . 'WHERE current_issue = -1 '
                 . 'ORDER BY interface_issue;',
        'value' => 'interface_issue',
        'class' => '',
        'text' => 'interface_issue'
    );

You give us a very short piece of code, and two simple questions. I will not address the other issues of your code (or maybe I will), but concentrate on the two questions.

1. "Would it be better to start with an empty htmlCode and then concatenate all the code in the loop before finally using echo outside the loop?"

I guess the other alternative is echo'ing the html inside the loop? This is probably a question about performance? The best way to figure this out, is to test both cases. If the question is about coding I would not echo inside this method at all, but just return a string with the HTML. This allow you to decide later, in another part of the code, what to do with the HTML: Store it? Put it in an email? Or echo it?

1. "do I need to declare $boxOption, $paramKey and $paramValue as class variables or are they OK as is because they're only used in this method?".

Well, if they're only used in this method, then they should be local variables, NOT the class variables which you made them into now. So replace $this-> by $.

Just one remark about your method: A method should have one purpose, one thing to do. Yours does several: 1. retrieve data from the database using a query that's stored in a strange way. 2. Build HTML, but not even a whole ``, only the options. 3. Output the HTML. This is not very flexible. Your data always has to come from a predetermined place, and your output will always be echoed. Any variation on this method will require a completely new method, probably doing almost the same thing.

You also seem to have a class that does many things. The 'single responsibility principle' doesn't seem to have been applied. These principles are here for a very good reason. See:

http://en.wikipedia.org/wiki/SOLID_%28object-oriented_design%29

A programmer can ignore these principles for a long time, and programs will work, but when they grow, and expand, you will find that sticking to these principles can be very useful. Luckily the 'single responsibility principle' is one of the easiest to understand. A better explanation can be found here:

http://www.codemag.com/article/1001061

StackExchange Code Review Q#84077, answer score: 3