PHP Table generation

Question

I have been using the following function (in the kohana html class in a table) to generate tables.

function table($columns,$reverse=false,$fill=false,array $attr=null){
    $c = '';
    //ugly hack, looking for fix
    $max=sizeof(max($columns));
    for($i=0;$i<sizeof($columns);$i++){
            $column = $columns[$i];

        if($fill==true){
            $l = ($max-sizeof($column));
            for($ii=0;$ii<$l;$ii++){
                array_push($column,'');
            }
        }
        if($reverse==true){
            $columns[$i] = array_reverse($column);
        }else{
            $columns[$i] = $column;
        }
    }
    for($i=0,$l=sizeof($columns);$i<$l;$i++){
        $column = $columns[$i];
        $c.="<tr>";
        for($ii=0,$ll=sizeof($column);$ii<$ll;$ii++){
            $c.="<td>".$column[$ii]."</td>";
        }
        $c.="</tr>";
    }
    return "<table".html::attributes($attr).">".$c."</table>";
}

And then I have been calling it like so:

echo html::table(array(
    array('colunm 1 row 1','colunm 2 row 1'),
));

And a reversed table that auto fills the columns.

echo html::table(array(
    array('colunm 1 row 1','colunm 2 row 1'),
    array('column 1 row 2','column 2 row 2')
),true,true);

Answer 1

Didn't test, but I would write it something like this:

function table($rows, $reverse = false, $fill = false, array $attr = null) {
    $c = '';
    $max_cols = sizeof(max($columns));

    foreach ($rows as $columns) {
        if ($fill && sizeof($columns) < $max_cols) {
            $columns = array_merge($columns, array_fill(0, $max_cols - sizeof($columns), ''));
        }
        if ($reverse) {
            $columns = array_reverse($columns);
        }
        $c .= '<tr><td>' . implode('</td><td>', $columns) . '</td></tr>';
    }

    return '<table>' . $c . '</table>';
}

Answer 2

• Some of the variable names could benefit from being slightly less terse.
• Aren't you passing in an array of rows? Each element in the array is then an array of columns.
• What is max? Is it trying to find the row with the most columns? Either rename to something more self-documenting or add a comment.
• array_map may be worth a look and could feasibly allow for reverse to be replaced with a callback function.

• Consider changing the last lines of the function to:

  return "<table".html::attributes($attr).">".$c."</table>";
  

Answer 3

replace for loops with foreach