1
\$\begingroup\$

In CodeIgniter, I have created function (helper) to populate, store and retrieve checkbox group. I just wonder if I have written code is in proper way and optimized or needs some more finishing?

Function

if ( ! function_exists('checkbox_group') )
{
    function checkbox_group($checkboxes = array(), $name)
    {

    // setting hidden field for null, if no checkbox selected
    echo form_hidden($name.'[]', 'null');

    // start checkbox loop
    foreach($checkboxes as $check => $label_text):

        $checked = FALSE;                   
        $selects = get_config_row($name);
        if(in_array($check, $selects))
        $checked = TRUE;

        echo '<div class="checkbox">';
        echo form_label(form_checkbox($name.'[]', $check, $checked, 'id="'.$check.'" class="checkbox"') . $label_text, $check);
        echo '</div>';
    endforeach;
    }
}

Usage

// checkbox items
$checkboxes = array(
    'noindex'=>'No Index',
    'nofollow'=>'No Follow',
    'noarchive'=>'No Archive',
    'nosnippet'=>'No Snippet'
);

// render checkboxes useing function
checkbox_group($checkboxes, 'indexes');
\$\endgroup\$
  • 1
    \$\begingroup\$ Is this code is really optimized so there is no suggestion or feedback yet? :S \$\endgroup\$ – pixelngrain Feb 5 '14 at 10:42
  • \$\begingroup\$ It's probably because few people here have used Codeigniter. I did but it was a long time ago. PHP-wise I think your code is fine. I don't know why you use a config file to store information about potential checkbox items, but you seem good enough that you probably know what you're doing. Maybe someone better than me will come along with some patience. \$\endgroup\$ – user36070 Feb 6 '14 at 22:36
  • \$\begingroup\$ @Pickett thanks for your feedback. I am storing config values in database and have separate admin section to manage those config settings. These checkboxes are for one of those settings. For instance these checkboxes allows or disallow robots to index, follow, archive, snippet etc.. will stored into db than the config.php will use that value \$\endgroup\$ – pixelngrain Feb 7 '14 at 4:49
  • 1
    \$\begingroup\$ Perhaps that works for you, but in general that is not recommended I think. A helper is supposed to be modular. You're supposed to be able to use it in any CI project. Now it's only valid for this particular project because it relies on how the database is constructed and another file, config.php. To keep it modular and MVC you should let it accept that data as a parameter instead, acquire the information through a model and then pass that information through the controller to the view and then as a parameter to your helper. \$\endgroup\$ – user36070 Feb 7 '14 at 6:54
2
\$\begingroup\$

I haven't worked with CodeIgniter before, so if I say something that argues their standards, please excuse me.

It's such a basic piece of code that there isn't too much to be said. One things I notice right off the bat, your checkbox_group() function has a first parameter with a default value. I'm surprised you haven't done anything about this because it should be throwing you a warning.

Note that when using default arguments, any defaults should be on the right side of any non-default arguments; otherwise, things will not work as expected.

As per the PHP Functions page

To better separate the function's business and the view, consider creating an output string and returning it to be echoed. You'd then have:

$output = '';
// setting hidden field for null, if no checkbox selected
$output .= form_hidden($name . '[]', 'null');

// start checkbox loop
foreach ($checkboxes as $check => $label_text) :

    $checked = FALSE;
    $selects = get_config_row($name);
    if (in_array($check, $selects))
        $checked = TRUE;

    $output .= '<div class="checkbox">';
    $output .= form_label(form_checkbox($name . '[]', $check, $checked, 'id="' . $check . '" class="checkbox"') . $label_text, $check);
    $output .= '</div>';
endforeach;
return $output;

Now you call:

// render checkboxes useing function
echo checkbox_group($checkboxes, 'indexes');
\$\endgroup\$
1
\$\begingroup\$

Splitting the assignment like this is clumsy:

$checked = FALSE;
…
if (in_array($check, $selects))
    $checked = TRUE;

in_array() returns a bool. Why not just say:

$checked = in_array($check, get_config_row($name));
\$\endgroup\$
1
\$\begingroup\$

The HTML output would look like this (pretty-printed):

<input type="hidden" name="indexes[]" value="null" />
<div class="checkbox">
  <label for="noindex">
    <input type="checkbox" name="indexes[]" value="noindex" id="noindex" class="checkbox" />No Index
  </label>
</div>
<div class="checkbox">
  <label for="nofollow">
    <input type="checkbox" name="indexes[]" value="nofollow" checked="checked" id="nofollow" class="checkbox" />No Follow
  </label>
</div>
…

I think that is problematic in several ways:

  • It will always submit a indexes[]=null value, even when some checkboxes are selected. What good does indexes[]=null do? Just omit that hidden field.

  • <div class="checkbox"> is an unnecessary level of nesting. Just <label class="checkbox"> would suffice. Then, in CSS, you can optionally style it with

    label.checkbox {
        display: block;
    }
    

    if you want it to be displayed as a block.

  • Putting class="checkbox" on an <input type="checkbox"> is redundant. You could just use input[type="checkbox"] as a CSS selector.

  • Every id attribute in an HTML document must be unique. (If any other element also has id="nofollow", then the document is invalid.) Therefore, you should make a greater effort to avoid conflicting element IDs. For example, id="indexes-nofollow" would be reasonable.

In addition, your code seems to be vulnerable to HTML injection. Since $label_text should be considered to contain arbitrary text, not just sane identifiers, it needs to be escaped.

Recommended rewrite:

echo form_label(
    form_checkbox($name.'[]', $check, $checked, "id=\"$name-$check\"") .
        htmlspecialchars($label_text),
    '',
    array('class' => 'checkbox')
);

… which would produce output (pretty-printed):

<label class="checkbox">
  <input type="checkbox" name="indexes[]" value="noindex" id="indexes-noindex" />No Index
</label>
<label class="checkbox">
  <input type="checkbox" name="indexes[]" value="nofollow" checked="checked" id="indexes-nofollow" />No Follow
</label>
…
\$\endgroup\$

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.