Recursive but Not Recursive - Form Optiongroup/Option Population

Question

I consider myself new to PHP. I can make it do whatever I want it to do, but that doesn't make it "right" or the "best way to do it". From a performance standpoint, there always seems to be a "better" way to do it based on code complexity and execution time.

This may be one of those questions that get closed because it's "opinion based" or whatever, but I'm not looking for an opinion. I'm looking for a "leaner and meaner" way of doing this, so that if nothing else I can learn how I can improve.

The following code works. It does exactly what I want it to do. But it seems very... cumbersome. So the question is: what's a cleaner, leaner and meaner way to accomplish this?

Model

public function bdgt_cat_select($id = NULL) {
    if ($id == NULL) {
        $query = $this -> db -> query("SELECT * FROM bdgt_cat WHERE parent_id = 0");
        if ($query -> num_rows() > 0) {
            foreach ($query->result() AS $row) {
                $array[] = get_object_vars($row);
            }
            return $array;
        }
    } else {
        $sql = "SELECT * FROM bdgt_cat WHERE parent_id = ?";
        $query = $this -> db -> query($sql, $id);
        if ($query -> num_rows() > 0) {
            foreach ($query->result() AS $row) {
                $array[$row->parent_id] = get_object_vars($row);
            }
            return $array;
        }
    }
}

Controller

public function index() {
    $data['cat_sel'] = $this->read->bdgt_cat_select();
    foreach($data['cat_sel'] AS $subcat) {
        $data['subcat_sel'] = $this->read->bdgt_cat_select($subcat['id']);
    }
    $this->stencil->paint('finance/add_transaction', $data);
}

View

<select class="form-control" name="id" id="id">
<option value="">-- SELECT --</option>
<?php
    foreach($cat_sel AS $row) {
        (int) $rid = $row['id'];
            if ((int) $row['parent_id'] === 0) {
                echo '<optgroup label="' . $row['cat_label'] . '">';
                    foreach($subcat_sel AS $sub) {
                        if ((int) $sub['parent_id'] == $rid) {
                            echo '<option value="' . $sub['id'] . '">' . $sub['cat_label'] . '</option>';
                        }
                    }
                echo '</optgroup>';
            }
        }
    ?>
</select>

Obviously this is a budget part of a larger project I'm working on. Personal thing. Whatever.

Output

<select class="form-control" name="id" id="id">
    <option value="">-- SELECT --</option>
    <optgroup label="Manual Adjustment"></optgroup>
    <optgroup label="Income">
        <option value="3">Placeholder</option>
    </optgroup>
</select>

Final Notes

This project is being built in CodeIgniter (this is by no means a "do I need to change frameworks" kind of question). It is purely a personal project. It will probably never show up on the internet anywhere. I can't figure out Git anyway. I may at some point convert it to a Phonegap thing or just run it off some kind of server that runs on my phone. This is why I'm really questioning performance and maintainability.

Answer 1

This question has way too many questions in it. On the other hand it provides a lot of code that needs to be reverse engineered. Anyway.

Code readability:

At some point in my career, I found out that when the database becomes with more than 20 tables, the team, and also me, needs to remember each table, each column. Why so? Because everything relies on some magic strings that are column names and table names.

When you give meaning to these entities, you do require your team (or, from now on, I will say "yourself", because you said it's not a professional project), or yourself, to remember them all in order to render table rows, manipulate data, etc.

On the first place, your controller is highly aware of what the model returns. You do use associative arrays and specify the column names right there as strings. Today you are aware, but after one year? What would you do? var_dump($data) to see all these magic keys?

The second place is the view. To render any option or group you are required to remember the column name. All of them. Which is controllable column for logical checks, which is data column for rendering.

Once I hit that problem, I found that my brain is full of tables and columns. When projects are getting more and more and some tables were having the same names, I started to have problems remembering which columns belong to project X and which to Y. Do users in X have expires_on column or it was in Y.

I highly recommend you to stop relying in magic strings and testing when your brain gonna explode. Make your life easy. Centralize the database operations and make an abstraction over them. Object Relational Mappings are a good start, but your can do it the carpenter way too. Extract the things once in your models and put them in objects. All the operations or column names will be now methods and properties which the IDE will autocomplete for you. You will be no more required to remember the API of your domain models. $bdgedCat-> and you recieve a full list of your available operations :-)

Code duplication:

Your model has a simple if/else, which duplicates a code there. The only difference is parent_id is 0 when the argument is NULL. Why not extracting only this in an if/else and the other part out of the construction. Now when something that is applied in both scenarios changes, you need to change it in if and else

public function bdgt_cat_select($id = NULL) {
     $parent_id = $id === NULL ? 0 : $id;
     $query = $this -> db -> query("SELECT * FROM bdgt_cat WHERE parent_id = ?", $parent_id);
        if ($query -> num_rows() > 0) {
            foreach ($query->result() AS $row) {
                $array[] = get_object_vars($row);
            }
            return $array;
        }     
}

PERFORMANCE:

If you look closely you will see the potential very big PERFORMANCE problem. You do iterate in your resultset to return an array in your model. Then you traverse the newly created array from your model to create the view data. Then in the view you are traversing the view data in order to render the HTML. You are doing THREE iterations over ONE resultset. PHP as of version 5.3 like many other languages support Generators. A way you can expose an iterator over data and not the data itself already loaded in memory. It can be done via yield keyword.

In many scenarios you may need to change the data model in the controller, and that's why some ORM's support functional approach, so you can provide functions how the data should be modeled, but you still work with the generator, so dataset is still not evaluated. I will skip this in my example below, for the simplicity sake.

Your layers could be simplified to this:

Model:

public function bdgt_cat_select($id = NULL) {
     $parent_id = $id === NULL ? 0 : $id;
     $query = $this -> db -> query("SELECT * FROM bdgt_cat WHERE parent_id = ?", $parent_id);
        if ($query -> num_rows() > 0) {
            foreach ($query->result() AS $row) {
                yield new Category($row['id'], $row['any_other_column']...);
            }
            return $array;
        }     
}

Controller:

public function index() {
    $data = $this->read->bdgt_cat_select();
    // the potential foreach here to model the data is skipped
    // but you can provide a function in your Category model
    // which will be evaluated on using the object
    // via lambda
    $this->stencil->paint('finance/add_transaction', $data);
}

View:

<?php /** @var Category $data */ ;?>
<select class="form-control" name="id" id="id">
<option value="">-- SELECT --</option>
<?php foreach($data AS $row): ?>
    <?php if ($row->getParentId() === 0): ?>
         <optgroup label="<?= $row->getCatLabel();?>">
               <?php foreach($row->getSubcatSel() AS $sub): ?>
                   <?php if ($sub->getParentId() == $row->getId()): ?>
                        <option value="<?= $sub->getId();?>"> <?= $sub->getCatLabel(); ?></option>
       <?php endif;?>
                <?php endforeach; ?>
                </optgroup>
            <?php endif; ?>
        <?php endforeach; ?>
</select>

But you really need to take a look at some kind of ORM, you do not need to develop it yourself. There are already some smarter teams that abstracted it before us. Maybe look at Propel ORM.