28
\$\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\$
1
  • \$\begingroup\$ $iCount in 1st iteration should be $count. \$\endgroup\$
    – Raveren
    Nov 21, 2012 at 18:26

4 Answers 4

23
\$\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\$
7
  • \$\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, 2011 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, 2011 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, 2011 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, 2011 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\$ Jan 26, 2011 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 made?

  • 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.Tthe callback would simply 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 it's 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 check 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\$
4
  • 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, 2011 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, 2011 at 0:53
  • \$\begingroup\$ You have silently changed the meaning by converting >= 0 to > 0. \$\endgroup\$
    – Fred Nurk
    Jan 27, 2011 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, 2011 at 9:17
1
\$\begingroup\$
  1. While there is nothing wrong with defining your class method as private, I'm going to demonstrate with public for simplicity.
  2. I prefer to write @throws in my docblock so that my IDE can remind me if I am not properly catching what is thrown.
  3. I recommend type declarations whenever reasonable. The sole incoming argument must be an array and the return value will be numeric. From PHP8, union types are available — in this method, the return may be an integer or a float.
  4. Checking for an empty array is as simple as writing a "falsey" check.
  5. Throw a LengthException when the array is empty.
  6. Sorting the array is an essential part of this process, but even if the array values are strings, sort() will still treat the values numerically. In other words, it is not necessary to explicitly flag the sort with SORT_NUMERIC.
  7. After calculating the middle index of the array, if the value is a float, truncate the float to become an integer and early-return the corresponding array value.
  8. Otherwise, the middle index is an integer, then the array has an even length and the middle two values must be used in the calculation.
  9. In earlier versions of PHP, it used to be acceptable to access array values via a float value as the key. Now Deprecated: Implicit conversion from float [floatValue] to int loses precision is presented if the key is not converted to an integer.
  10. The snippet below avoids single-use variable declarations and complies with PSR-12 coding guidelines.

Code: (Demo)

class MyMath
{
    /**
     * @throws Exception
     */
    public function calculateMedian(array $array): int|float
    {
        if (!$array) {
            throw new LengthException('Cannot calculate median because Argument #1 ($array) is empty');
        }
        sort($array);
        $middleIndex = count($array) / 2;
        if (is_float($middleIndex)) {
            return $array[(int) $middleIndex];
        }
        return ($array[$middleIndex] + $array[$middleIndex - 1]) / 2;
    }
}


$math = new MyMath;
var_export($math->calculateMedian([1, 2, 3, 4]));
\$\endgroup\$

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.