26
\$\begingroup\$

I got this as an implementation of "get me the median of those values". But it sort of doesn't feel right (too long, too many branch points) so I thought I'll post it here to see what you think.

<?php
private function calculateMedian($aValues) {
    $aToCareAbout = array();
    foreach ($aValues as $mValue) {
        if ($mValue >= 0) {
            $aToCareAbout[] = $mValue;
        }
    }
    $iCount = count($aToCareAbout);
    sort($aToCareAbout, SORT_NUMERIC);
    if ($iCount > 2) {
        if ($iCount % 2 == 0) {
            return ($aToCareAbout[floor($iCount / 2) - 1] + $aToCareAbout[floor($iCount / 2)]) / 2;
        } else {
            return $aToCareAbout[$iCount / 2];
        }
    } elseif (isset($aToCareAbout[0])) {
        return $aToCareAbout[0];
    } else {
        return 0;
    }
}
\$\endgroup\$
  • \$\begingroup\$ $iCount in 1st iteration should be $count. \$\endgroup\$ – Raveren Nov 21 '12 at 18:26
20
\$\begingroup\$

The first part of your function filter out negative values. This has nothing to do with calculating the median itself, so should be moved away from this function.

Way I would do it.

Create array_median() function in a global scope (or a static method) like this:

/**
 * Adapted from Victor T.'s answer
 */
function array_median($array) {
  // perhaps all non numeric values should filtered out of $array here?
  $iCount = count($array);
  if ($iCount == 0) {
    throw new DomainException('Median of an empty array is undefined');
  }
  // if we're down here it must mean $array
  // has at least 1 item in the array.
  $middle_index = floor($iCount / 2);
  sort($array, SORT_NUMERIC);
  $median = $array[$middle_index]; // assume an odd # of items
  // Handle the even case by averaging the middle 2 items
  if ($iCount % 2 == 0) {
    $median = ($median + $array[$middle_index - 1]) / 2;
  }
  return $median;
}

This way we have generally available all purpose function, with naming consistent with core php functions.

And your method would look like

/**
 * The name should probably be changed, to reflect more your business intent.
 */
private function calculateMedian($aValues) {
  return array_median(
    array_filter(
      $aValues, 
      function($v) {return (is_numeric($v) && $v >= 0);}
      // You can skip is_numeric() check here, if you know all values in $aValues are actually numeric 
    )
  );
}

Either within calculateMedian() or in the code that calls it, you should take care of catching the DomainException that can be thrown if the array is empty)

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

I'm wondering if you can just compact the above to this:

private function calculateMedian($aValues) {
    $aToCareAbout = array();
    foreach ($aValues as $mValue) {
        if ($mValue >= 0) {
            $aToCareAbout[] = $mValue;
        }
    }
    $iCount = count($aToCareAbout);
    if ($iCount == 0) return 0;

    // if we're down here it must mean $aToCareAbout
    // has at least 1 item in the array.
    $middle_index = floor($iCount / 2);
    sort($aToCareAbout, SORT_NUMERIC);
    $median = $aToCareAbout[$middle_index]; // assume an odd # of items

    // Handle the even case by averaging the middle 2 items
    if ($iCount % 2 == 0)
        $median = ($median + $aToCareAbout[$middle_index - 1]) / 2;

    return $median;
}

I don't write PHP but from looking at the online manual for count:

count() may return 0 for a variable that isn't set, but it may also return 0 for a variable that has been initialized with an empty array. Use isset() to test if a variable is set.

But in your case, the function doesn't seem to care whether the array is empty or the variable isn't set -- 0 is returned in both cases. By checking what count returns we could eliminate some of the if branches.

\$\endgroup\$
  • \$\begingroup\$ I don't like it returning 0 for empty $aToCareAbout. I would prefer null to be returned in such case. (Or perhaps an exception to be thrown?) \$\endgroup\$ – Mchl Jan 26 '11 at 8:21
  • \$\begingroup\$ @Mchl yeah that was one of the other points bothering me. The original code is using 0 to indicate some kind of problem. But a zero-filled array can have a valid median as well. \$\endgroup\$ – greatwolf Jan 26 '11 at 8:26
  • 2
    \$\begingroup\$ It should be +1 for the average shouldn't it ? (Or for 2 element it will try to access [-1] ? Will provide a first iteration with all the feedback in it shortly. \$\endgroup\$ – edorian Jan 26 '11 at 8:43
  • 2
    \$\begingroup\$ @edorian From looking at your original code, I assumed php arrays are 0-based, like C. So in the case of an array with 2 elements iCount is 2, middle_index would be 1, middle_index - 1 would be 0. Elements from these 2 indices are then averaged. \$\endgroup\$ – greatwolf Jan 26 '11 at 9:00
  • 2
    \$\begingroup\$ @edorian: No, this is the correct approach. Two quick examples: Odd number: (3 elements, indices 0 to 2, expected = 1) Median is element floor(3/2) == 1. Pass. Even number: (4 elements, indices 0 to 3, expected = avg of 1 and 2) For 4 elements, it will use floor(4/2) == 2. Even amount of elements, so it uses element 2 - 1 = 1 as well. Median is average of elements 1 and 2. Pass. \$\endgroup\$ – doppelgreener Jan 26 '11 at 9:03
2
\$\begingroup\$

Personally this is the way that I would build the function:

function calculateMedian($Values)
{
    //Remove array items less than 1
    $Values = array_filter($Values,array($this,"callback"));

    //Sort the array into descending order 1 - ?
    sort($Values, SORT_NUMERIC);

    //Find out the total amount of elements in the array
    $Count = count($Values);

    //Check the amount of remainders to calculate odd/even
    if($Count % 2 == 0)
    {
        return $Values[$Count / 2];
    }

    return (($Values[($Count / 2)] + $Values[($Count / 2) - 1]) / 2);
}

What changes have I done?

  • I have used less variables, overwriting the $Values where needed
  • Reduced the conditional statements to 1* from 2
  • Made the code look more readable and understandable.
  • I have however added a callback, which in turn removes the foreach and if statements but a logical check would have to be used in the callback. the callback would simple be a method in your class like so:
    • public function callback($value) { return $value > 0; }
  • Unfortunately as the native function empty is actually a language construct its not a valid callback, you can however use return !empty($value); within your callback method to also remove other entities such as NULL,FALSE etc
  • This can be removed as stated, and placed outside the function.

*Notes: I would advise you to have some kind of linear array check to make sure the arrays are based on an integer index, as our code assumes they are, a linear chack can be done like so:

if(array_keys($Values) !== range(0,($Count-1)))
{
    return null;
}

this would be added after the $Count value has come into play.

Example test that I had used to test it was:

$values = array(
    0,4,7,5,6,9,5,3,2,7,5,6,4,3,7
);
echo calculateMedian($values);

which resulted in the correct answer of 5

\$\endgroup\$
  • 3
    \$\begingroup\$ I don't mean to nitpick, but is there a specific reason you capitalized (some of) the variable names? I think it's generally a good idea for answers to use the same variable naming conventions as the question (unless the question's naming convention contradicts the language's conventions). \$\endgroup\$ – sepp2k Jan 26 '11 at 19:42
  • \$\begingroup\$ No reason, its just the way that I tend to write code, Im displaying the benefits of my logic, the OP would be able to change them accordingly as the capitalising is is just a naming convention I like to use. \$\endgroup\$ – RobertPitt Jan 27 '11 at 0:53
  • \$\begingroup\$ You have silently changed the meaning by converting >= 0 to > 0. \$\endgroup\$ – Fred Nurk Jan 27 '11 at 9:13
  • \$\begingroup\$ Fred, I believe your on about the callback, are you saying that zero values should be within the calculation ? \$\endgroup\$ – RobertPitt Jan 27 '11 at 9:17

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.