1
\$\begingroup\$

In a controller (PHP, CodeIgniter) I'm adding scripts & css to variables to be used in the view output. My initial coding looked like this:

private function _add_script($path, $where = self::ADD_LAST) {
    $html = "<script src='$path'></script>\n";
    switch($where) {
        case self::ADD_LAST:
            array_push($this->_data['script'], $html);
            break;
        case self::ADD_FIRST:
            array_unshift($this->_data['script'], $html);
            break;
    }
}

private function _add_css($path, self::ADD_LAST) {
    $html = "    <link rel='stylesheet' href='$path'>\n";
    switch($where) {
        case self::ADD_LAST:
            array_push($this->_data['css'], $html);
            break;
        case self::ADD_FIRST:
            array_unshift($this->_data['css'], $html);
            break;
    }
}

Well, I noticed that these two are basically just repeating themselves, so I immediately rewrote them into this format:

private function _add_script($path, $where = self::ADD_LAST) {
    $html = "<script src='$path'></script>\n";
    $this->_add_external('script', $html, $where);
}


private function _add_css($path, $where = self::ADD_LAST) {
    $html = "<link rel='stylesheet' href='$path'>\n";
    $this->_add_external('css', $html, $where);
}



private function _add_external($type, $html, $where) {
    switch($where) {
        case self::ADD_LAST:
            array_push($this->_data[$type], $html);
            break;
        case self::ADD_FIRST:
            array_unshift($this->_data[$type], $html);
            break;
    }
}

Does the second version seem like the way to go? Or perhaps a 3rd alternative. The requirements call for being able to add elements to the list either before or after all other elements already in the list.

\$\endgroup\$
1
  • \$\begingroup\$ Your rewrite would be good if you were adding many different tags. I.E. more than just the two (script and link) \$\endgroup\$ Apr 27 '11 at 0:39
4
\$\begingroup\$

I think what you did was a good thing. I also think you could combine the first 2 functions as well.

Something like this:

private function _add_item($path, $type, $where = self::ADD_LAST) {
    $script = "<script src='$path'></script>\n";
    $css = "<link rel='stylesheet' href='$path'>\n";

    if ($type == 'script') {
        $html = $script; 
    }
    elseif ($type == 'css') {
        $html = $css;
    }

    $this->_add_external($type, $html, $where);
}

Or this:

private function _add_item($path, $type, $where = self::ADD_LAST) {
    $script = "<script src='$path'></script>\n";
    $css = "<link rel='stylesheet' href='$path'>\n";

    switch($type) {
        case 'script':
            $html = $script; 
            break;
        case 'type':
            $html = $css;
            break;
    }

    $this->_add_external($type, $html, $where);
}

Or this (My favorite):

private function _add_item($path, $type, $where = self::ADD_LAST) {
    $array['script'] = "<script src='$path'></script>\n";
    $array['css'] = "<link rel='stylesheet' href='$path'>\n";

    $html = $array[$type]; 

    $this->_add_external($type, $html, $where);
}

EDIT:

And I think this would work too, an all in one solution:

private function _add_item_external($path, $type, $where = self::ADD_LAST) {
    $array['script'] = "<script src='$path'></script>\n";
    $array['css'] = "<link rel='stylesheet' href='$path'>\n";

    switch($where) {
        case self::ADD_LAST:
            array_push($this->_data[$type], $array[$type]);
            break;
        case self::ADD_FIRST:
            array_unshift($this->_data[$type], $array[$type]);
            break;
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ I don't think you can go "too far", can you? So long as what you do saves space and time, I say it is a good thing. It may make your code look sloppy, but just have two versions: one to look at and one for actually running? \$\endgroup\$ Apr 27 '11 at 3:24
  • \$\begingroup\$ @Mr_CryptoPrime That is just begging for insidious bugs to sneak through the discrepancies between the two versions of the code. \$\endgroup\$
    – Null Set
    Apr 27 '11 at 19:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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