2
\$\begingroup\$

The following is part of a personal budgeting program I'm writing.

This code pulls line item information from multiple tables and writes it into an array and then displays the information by transid => family => category => lineItems. Everything works, and I get the results I want out of it. I want to know if there is a more efficient way to accomplish this task.

Since this is a personal program, I'm only asking so that I can improve my coding abilities.

<?php

include ('../cfg/connect.php');

$s = " : ";
$br = "<br>";
$ul = "<ul>";
$li = "<li>";
$_ul = "</ul>";
$_li = "</li>";

$data = [];
$itemCount = 0;
$arrayItemCount = 0;


$categoryQry = "SELECT a.itemQty, b.transDate, b.transID, b.amount, a.itemPrice, a.itemCategory, c.catFamily, a.itemName, a.itemSource FROM budget.lineItems AS a JOIN budget.quickEntry AS b ON a.transID = b.transID JOIN budget.categories AS c ON a.itemCategory = c.catName WHERE b.processed = 'y' ORDER BY c.catFamily, c.catName, b.transDate";
$categories = $conn->prepare ($categoryQry);
$categories->execute ();
$categories->store_result ();
$categories->bind_result ($itemQty, $transDate, $transID, $totalPrice, $itemPrice, $category, $family, $itemName, $source);

while ($categories->fetch ()) {

    if (!isset($data[$transID]['amount'])) {
        $data[$transID]['amount'] = 0;
    }
    if (!isset($data[$transID]['line'])) {
        $data[$transID]['line'] = '';
    }
    if (!isset($data[$transID]['line'][$family]['amount'])) {
        $data[$transID]['line'][$family]['amount'] = 0;
    }
    if (!isset($data[$transID]['line'][$family]['line'])) {
        $data[$transID]['line'][$family]['line'] = '';
    }
    if (!isset($data[$transID]['line'][$family]['line'][$category]['amount'])) {
        $data[$transID]['line'][$family]['line'][$category]['amount'] = 0;
    }
    if (!isset($data[$transID]['line'][$family]['line'][$category]['line'])) {
        $data[$transID]['line'][$family]['line'][$category]['line'] = '';
    }

    $itemCount++;

    $qtyPrice = $itemPrice * $itemQty;
    $data[$transID]['amount'] += $qtyPrice;
    $data[$transID]['transDate'] = $transDate;
    $data[$transID]['source'] = $source;
    $data[$transID]['line'][$family]['amount'] += $qtyPrice;
    $data[$transID]['line'][$family]['line'][$category]['amount'] += $qtyPrice;
    $data[$transID]['line'][$family]['line'][$category]['line'][$itemName] = ['itemQty' => $itemQty, 'itemPrice' => $itemPrice];
}


foreach ($data as $transID => $transValue) {
    echo $transID .$s.$transValue['transDate'].$s.$transValue['source'].$s.$transValue['amount']. $ul;
    foreach ($transValue['line'] as $category => $categoryValue) {
        echo $li . $category .$s.$categoryValue['amount']. $ul;
        foreach ($categoryValue['line'] as $line => $lineValue) {
            echo $li . $line .$s.$lineValue['amount']. $ul;
            foreach ($lineValue['line'] as $item => $details) {
                echo $li . $item .$s . $details['itemQty'] . $s . $details['itemPrice'] . $_li;
            }
            echo $_ul . $_li;
        }
        echo $_ul . $_li;
    }
    echo $_ul . $br;
}
\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

My thoughts in walking through your code are shown below:

include ('../cfg/connect.php');

This appears to be hard dependency. You should use require such that this script fails out of you can't load the dependency. Also, your style of adding space between function/method name and opening parenthesis would be considered non-standard by most PHP developers. Typical usage has no space. This makes reader less likely to confuse this parenthesis with a logical operation.


$s = " : ";
$br = "<br>";
$ul = "<ul>";
$li = "<li>";
$_ul = "</ul>";
$_li = "</li>";

Consider using constants for these, as they don't change during script execution. You might also find this helps readability of your code when you begin to concatenate with these.


$categoryQry = "SELECT a.itemQty, b.transDate, b.transID, b.amount, a.itemPrice, a.itemCategory, c.catFamily, a.itemName, a.itemSource FROM budget.lineItems AS a JOIN budget.quickEntry AS b ON a.transID = b.transID JOIN budget.categories AS c ON a.itemCategory = c.catName WHERE b.processed = 'y' ORDER BY c.catFamily, c.catName, b.transDate";

This is a horrendously long line of code. You should strive to keep character count on a line under 80. Break this query up across lines, making it much easier to read.

Example:

$categoryQry = <<<EOT
SELECT
    a.itemQty,
    b.transDate,
    b.transID,
    b.amount,
    a.itemPrice,
    a.itemCategory,
    c.catFamily,
    a.itemName,
    a.itemSource
FROM budget.lineItems AS a
JOIN budget.quickEntry AS b
    ON a.transID = b.transID
JOIN budget.categories AS c
    ON a.itemCategory = c.catName
WHERE b.processed = 'y'
ORDER BY c.catFamily, c.catName, b.transDate
EOT;

$categories = $conn->prepare ($categoryQry);
$categories->execute ();
$categories->store_result ();
$categories->bind_result ($itemQty, $transDate, $transID, $totalPrice, $itemPrice, $category, $family, $itemName, $source);

This all assumes happy-path execution. What happens if prepare(), or execute() fail? Last line of code is too long. Break it up

Example:

$categories->bind_result(
    $itemQty, $transDate, $transID, $totalPrice, $itemPrice,
    $category, $family, $itemName, $source
);

if (!isset($data[$transID]['amount'])) {
    $data[$transID]['amount'] = 0;
}
if (!isset($data[$transID]['line'])) {
    $data[$transID]['line'] = '';
}
if (!isset($data[$transID]['line'][$family]['amount'])) {
    $data[$transID]['line'][$family]['amount'] = 0;
}
if (!isset($data[$transID]['line'][$family]['line'])) {
    $data[$transID]['line'][$family]['line'] = '';
}
if (!isset($data[$transID]['line'][$family]['line'][$category]['amount']))
{
    $data[$transID]['line'][$family]['line'][$category]['amount'] = 0;
}
if (!isset($data[$transID]['line'][$family]['line'][$category]['line']))
{
    $data[$transID]['line'][$family]['line'][$category]['line'] = '';
}

$itemCount++;

$qtyPrice = $itemPrice * $itemQty;
$data[$transID]['amount'] += $qtyPrice;
$data[$transID]['transDate'] = $transDate;
$data[$transID]['source'] = $source;
$data[$transID]['line'][$family]['amount'] += $qtyPrice;
$data[$transID]['line'][$family]['line'][$category]['amount'] += $qtyPrice;
$data[$transID]['line'][$family]['line'][$category]['line'][$itemName]
= ['itemQty' => $itemQty, 'itemPrice' => $itemPrice];

I have concern over the overall data structure you are building. First, I think there are more levels of nesting than is necessary. Second, the repeated use of line as key at different levels of depth seems odd and potentially confusing.

I would encourage you to really rethink your data structure with an object-oriented approach. Think about what are the "real world" properties that should exist for each item in the hierarchy and how you represent the relationship between items.

You might end up with a structure like that shown below (shown using JSON-like notation to indicate object structures - or alternately associative arrays in PHP). This has 4 levels of nesting instead of 8 as your current model does. Interestingly enough, you will also note that your logic to output this data structure has 4 levels. That is because four levels of nesting is what the "real-world" relations between these objects in your hierarchy really dictate.

{
    'transactionid1': {
        'amount': *,
        'date': *,
        'source': *,
        'categories': {
            'categorynameA': {
                'amount': *,
                'lines': {
                    'linenameA': {
                        'amount': *,
                        'items': {
                            'itemnameA': {
                                'qty': *,
                                'price': *
                            },
                            'itemnameB': // more items
                        },
                    'linenameB': // other lines
                },
            'categorynameB': // other categories
        },
    'transactoinid2': // other transactions
}

This isn't exactly optimized for your display loops (which do not require unique lookup capabilities by transaction id, category, etc. as are required to build this structure from the result set), but I don't think that it makes sense to reconfigure this structure for this simple use case.

There is additionally some confusion on naming of family, categories, lines between the DB and what is being output to user. For example it seems family in DB maps to categories for output which seem strange. You might consider aliasing your fields in your query to be able to use common terminology throughout both the section of code where you build the data structure and where you output it.


In your output, consider keeping spaces around all . used for concatenation. This helps make code easy to read and allows for good visual line breaks in editors if a line does happen to go too long while editor is using "word wrapping".

Also, I think using objects (as mentioned in data structure comment above) instead of associative arrays helps in readability. This along with suggestion to use constants might yield code that looks like the following:

// assume you have constants S, BR, UL, LI, _UL, _LI
foreach ($data as $txn_id => $txn) {
    echo $txn_id . S . $txn->date . S . $txn->source . S . $txn->amount . UL . PHP_EOL;
    foreach ($txn->categories as $cat_name => $cat) {
        echo LI . $cat_name . S .$cat->amount . UL . PHP_EOL;
        foreach ($cat->lines as $line_name => $line) {
            echo LI . $line_name . S . $line->amount . UL . PHP_EOL;
            foreach ($line->items as $item_name => $item) {
                echo LI . $item_name . S . $item->qty . S . $item->price . _LI . PHP_EOL;
            }
            echo _UL . _LI . PHP_EOL;
        }
        echo _UL . _LI . PHP_EOL;
    }
    echo _UL . BR . PHP_EOL;
}

To me, this is much easier to read and a pretty good approach short of going to more of a template approach.

\$\endgroup\$
1
  • \$\begingroup\$ Awesome answer. Exactly the kind of comments I'm looking for. As far is the categories mapping to family. I realized that yesterday and made the adjustment. Good call. And I'm slowly moving to a more object approach. I use arrays because that's what I know best, thank you for you examples. Will be very useful in future projects \$\endgroup\$ Commented Oct 12, 2016 at 3:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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