HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMajor

Calculate a median

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mediancalculatestackoverflow

Problem

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.

= 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;
    }
}

Solution

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)

Code Snippets

/**
 * 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;
}
/**
 * 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 
    )
  );
}

Context

StackExchange Code Review Q#220, answer score: 23

Revisions (0)

No revisions yet.