4
\$\begingroup\$

I'm visualizing my stored data in an histogram similar to this one:

enter image description here

However, I'm sure that my code smells a lot! So I'd like to have your opinion on how to make it better.

$listings = array(
              array('Price' => 10),
              array('Price' => 10),
              array('Price' => 11)
            ); // so on and so forth, this data is pulled from the database. 

// flattening the array so I remove the "Price" key
// the array takes the form of array(10,10,11,...)
foreach($listings as $listing) {
    $flattenedListings[] = $listing['Price'];
}

$widths = range(0, 100, 10); // creates array of the form: array(0, 10, 20, 30, 40, ...)
$bins = array();
$isLast = count($widths);
foreach($widths as $key => $value) {
    if($key < $isLast - 1) {
        $bins[] = array('min' => $value, 'max' => $widths[$key+1]);
    }
}

// creates array of the form:
// $bins = array(
//      array('min'=>0, 'max' => 10),
//      array('min'=>10,'max' => 20),
//      array('min'=>30, 'max'=>40)
//     );

$histogram = array();
        foreach($bins as $bin) {
            $histogram[$bin['min']."-".$bin['max']] = array_filter($flattenedListings, function($element) use ($bin) {
                if( ($element > $bin['min']) && ($element <= $bin['max']) ) {
                    return true;
                } 
                return false;
            });
        }

// Last one is a bit complicated, but basically what it does is that it creates an array of ranges as keys, so it generates this:

// array('0-10' => array(1, 2, 3, 4, 5, 6), 
//         '10-20' => array(11, 19, 12),
//         '20-30' => array(),
//   );
// Or in other words: foreach range in the histogram, php creates an array containing      values within the allowed limits.

    foreach($histogram as $key => $val) {
        $flotHistogram[$key] = (is_array($val)) ? ( (count($val)) ? count($val) : 0 ) : 0; 
    }

// And finally it just counts them, and returns a new array. 

As you can see in my code, I'm making an excessive use of foreach loops to basically do a "simple task."

Basically what I have is a list of items along with their prices, and I want to count how many items belong to each range. My ranges are 10 numbers apart: (0 - 10, 10 - 20, 20 - 30, etc...)

Ideally the output should tell me how many items there are in a given category, like 3 items in the range of $10 - $20, 2 items in the range of $20 - $30 and 1 item in the range of $30 - $40, etc... In the following format:

array('0-10' => 0, '10-20' => 3, '20-30' => 2, '30-40' => 1);

Given the following input:

array(
    array('Price' => 11),
    array('Price' => 12),
    array('Price' => 20),
    array('Price' => 25),
    array('Price' => 20.1),
    array('Price' => 33.5)
); 

Additional info:

I'm pulling my prices from a MySQL database using PDO option PDO::FETCH_ASSOC. How could the flattening of the array be skipped (instead of fetching an array of arrays, just fetch the numbers in a 1 dimensional array)?

\$\endgroup\$
6
\$\begingroup\$

Your code isn't exactly terrible, but this is code-review, and good code review has to be harsh, so please keep in mind that I mean this in the nicest of ways.
That said, there are some issues I'd like to address:

Redundancies - DNRY:
That old dead horse: Do Not Repeat Yourself. You have a lot of redundant code. Of course, you can get into code-golfing, and try to write everything as a one-liner. That will result in unmaintainable code, and that's not what we want, but in your case, there is room for some minor improvements that won't harm readability one bit, quite the contrary.

You say you're getting the data (prices, I take it) from a PDO resultset, why, then, don't you fill the $flattenedListings array like so:

$flattenedListings = array();
while($row = $pdoResOrStmt->fetch(PDO::FETCH_ASSOC))
{
    $flattenedListings[] = $row['Price'];
}

That's going to reduce the resources your script requires a tad, and fetches the data while at the same time constructing the flattened array.
Next, the $isLast = count($widths); variable can be ditched all together, either by a simple foreach, very close to to the loop you currently have:

$widths = range(0, 100, 10);
$bins = array();
foreach($widths as $key => $val)
{
    if (!isset($widths[$key +1])) break;//break on the last key
    $bins[] = array(
        'min' => $val,
        'max' => $widths[$key + 1]
    );
}

But, and this might be personal, I dislike this if() break; thing in a loop. Personally, I'd probably write something like:

for($i=0, $j=count($widths) -1; $i<$j;++$i)
{//count -1 = last key, but $i <$j => next to last key
    $bins[] = array(
        'min' => $widths[$i],
        'max' => $widths[$i+1],//can get last key
    );
}

Next, the $histogram. You're using a closure function + array_filter to group your values according to the price range. In itself, it's quite easy to read, and not that inefficient. But I find that closures in PHP (which are objects, instances of Closure) are 9/10 overkill. They're also not a "natural" feature of the language. PHP isn't a functional language, like Lisp, Scheme or JavaScript are. I've compared them to drag queens: A man can wear a dress, stockings, a wig and put on make-up as much as he wants, it still doesn't change the fact that he's a man.
Often, what you end up with is a grotesque, bemusing performance that leaves some feeling ill at ease.

There is a far easier way of determining the range for each price:

$histogram = array();
foreach($flattenedListings as $price)
{//assuming nothing costs 0
    $key = floor(($price -1)/10);
    $range = $bins[$key];
    $key = $range['min'] .'-'. $range['max'];
    if (!isset($histogram[$key])) $histogram[$key] = array();
    $histogram[$key][] = $price;
}

If some price can be zero, then just replace:

$key = floor(($price -1)/10);

with

$key = $price ? floor(($price -1)/10) : 0;

And you're good to go.

Simplify:
Now we've got that sorted out, it's time to simplify things a bit. As you can see by the way I've filled $histogram, I don't actually need the min and max values from the $bins array, so instead of filling that array with array('min'=> $x, 'max'=> $x+1), I could easily have written:

$widths = range(0, 100, 10);
$bins = array();
foreach($widths as $key => $val)
{
    if (!isset($widths[$key + 1])) break;
    $bins[] = $val.'-'. ($widths[$key + 1]);
}

This also makes filling the $flotHistogram array a lot easier:

$flotHistogram = array_fill_keys($bins, 0);//set all keys to 0

Then, when filling the $histogram:

$histogram = array();
foreach($flattenedListings as $price)
{
    $key = $bins[floor(($price -1)/10)];
    if (!isset($histogram[$key])) $histogram[$key] = array();
    $flotHistogram[$key]++;//add 1 count
    $histogram[$key][] = $price;
}

This leaves us with the following code:

//get prices
$flattenedListings = array();
while($row = $pdoResOrStmt->fetch(PDO::FETCH_ASSOC))
{
    $flattenedListings[] = $row['Price'];
}
//construct range-keys array
$widths = range(0, 100, 10);
$bins = array();
foreach($widths as $key => $val)
{
    if (!isset($widths[$key + 1])) break;
    $bins[] = $val.'-'. ($widths[$key + 1]);
}
//construct flotHistogram count array
$flotHistogram = array_fill_keys($bins, 0);
//construct array of prices for each key
$histogram = array();
foreach($flattenedListings as $price)
{
    $key = $bins[floor(($price -1)/10)];
    if (!isset($histogram[$key])) $histogram[$key] = array();
    $flotHistogram[$key]++;
    $histogram[$key][] = $price;
}

A bridge too far(?)
I may be taking things a bit too far, but that $flattenedListings array seems a bit redundant, now. However I don't know what else you need the db results for, but all in all, you may want to consider doing something like

$widths = range(0, 100, 10);
$bins = array();
foreach($widths as $key => $val)
{
    if (!isset($widths[$key + 1])) break;
    $bins[] = $val.'-'. ($widths[$key + 1]);
}

$flotHistogram = array_fill_keys($bins, 0);

$histogram = array();
//query data:
while($row = $pdoVar->fetch(PDO::FETCH_ASSOC))
{
    $key = $bins[floor(($row['Price'] -1)/10)];
    if (!isset($histogram[$key])) $histogram[$key] = array();
    $flotHistogram[$key]++;
    $histogram[$key][] = $row['Price'];
}

Which reduces your code even more. Of course, if you find this too hard to read (imagine, a couple of months from now, will you still be able to make sense of this code), then don't use it, add more comments and don't feel shy to be a bit more verbose or repetitive.

| improve this answer | |
\$\endgroup\$
  • 2
    \$\begingroup\$ This is excellent! By no mean I would take this the wrong way, and the refactored code 100% understandable. Thanks a lot! \$\endgroup\$ – ILikeTacos Nov 28 '13 at 17:12

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.