Fix lexicographic multi-key sorting (#356)

GameEngine/Multisort.php

@@ -30,57 +30,67 @@ class multiSort
 	{
 		$args = func_get_args();
 		$array = $args[0];
+		$criteria = array();
 
-		// iterate key/order/type triplets
+		// Collect key/order/type triplets in their declared priority order.
 		for ($i = 1; $i < count($args); $i += 3)
 		{
 			$key   = isset($args[$i]) ? $args[$i] : null;
 			$order = isset($args[$i + 1]) ? $args[$i + 1] : true; // true = ASC
 			$type  = isset($args[$i + 2]) ? $args[$i + 2] : 0;
 
-			if ($key === null) {
+			if ($key !== null) {
-				continue;
+				$criteria[] = array($key, $order, $type);
 			}
-
-			// comparator
-			$cmp = function ($a, $b) use ($key, $type, $order)
-			{
-				$va = isset($a[$key]) ? $a[$key] : null;
-				$vb = isset($b[$key]) ? $b[$key] : null;
-
-				switch ($type)
-				{
-					case 1: // Case insensitive natural
-						$result = strnatcasecmp($va, $vb);
-						break;
-
-					case 2: // Numeric
-						$result = ($va == $vb) ? 0 : (($va < $vb) ? -1 : 1);
-						break;
-
-					case 3: // Case sensitive string
-						$result = strcmp((string)$va, (string)$vb);
-						break;
-
-					case 4: // Case insensitive string
-						$result = strcasecmp((string)$va, (string)$vb);
-						break;
-
-					default: // Case sensitive natural
-						$result = strnatcmp((string)$va, (string)$vb);
-						break;
-				}
-
-				return $order ? $result : -$result;
-			};
-
-			usort($array, $cmp);
 		}
 
+		if (!count($criteria)) {
+			return $array;
+		}
+
+		usort($array, function ($a, $b) use ($criteria) {
+			foreach ($criteria as $criterion) {
+				list($key, $order, $type) = $criterion;
+				$va = isset($a[$key]) ? $a[$key] : null;
+				$vb = isset($b[$key]) ? $b[$key] : null;
+				$result = $this->compareValues($va, $vb, $type);
+
+				if ($result !== 0) {
+					return $order ? $result : -$result;
+				}
+			}
+
+			return 0;
+		});
+
 		return $array;
 	}
+
+	/**
+	 * Compare two values using the sort type expected by sorte().
+	 */
+	private function compareValues($a, $b, $type)
+	{
+		switch ($type)
+		{
+			case 1: // Case insensitive natural
+				return strnatcasecmp((string)$a, (string)$b);
+
+			case 2: // Numeric
+				return ($a == $b) ? 0 : (($a < $b) ? -1 : 1);
+
+			case 3: // Case sensitive string
+				return strcmp((string)$a, (string)$b);
+
+			case 4: // Case insensitive string
+				return strcasecmp((string)$a, (string)$b);
+
+			default: // Case sensitive natural
+				return strnatcmp((string)$a, (string)$b);
+		}
+	}
 }
 
 $multisort = new multiSort();
 
-?>
+?>

tests/MultisortTest.php

@@ -0,0 +1,84 @@
+<?php
+
+require_once dirname(__DIR__) . '/GameEngine/Multisort.php';
+
+function assertSameMultisort($expected, $actual, $message)
+{
+	if ($expected !== $actual) {
+		fwrite(
+			STDERR,
+			$message . PHP_EOL .
+			'Expected: ' . var_export($expected, true) . PHP_EOL .
+			'Actual:   ' . var_export($actual, true) . PHP_EOL
+		);
+		exit(1);
+	}
+}
+
+function sortedIds($rows)
+{
+	return array_column($rows, 'id');
+}
+
+$sorter = new multiSort();
+
+$rankings = array(
+	array('id' => 'a', 'x' => 0, 'y' => 0, 'pop' => 1),
+	array('id' => 'b', 'x' => 1, 'y' => 0, 'pop' => 9),
+	array('id' => 'c', 'x' => 0, 'y' => 1, 'pop' => 5),
+);
+
+assertSameMultisort(
+	array('a', 'c', 'b'),
+	sortedIds($sorter->sorte(
+		$rankings,
+		'x', true, 2,
+		'y', true, 2,
+		'pop', false, 2
+	)),
+	'Sort keys must be applied lexicographically in their declared order.'
+);
+
+$offers = array(
+	array('id' => 'slow', 'duration' => 30),
+	array('id' => 'fast', 'duration' => 5),
+	array('id' => 'medium', 'duration' => 15),
+);
+
+assertSameMultisort(
+	array('fast', 'medium', 'slow'),
+	sortedIds($sorter->sorte($offers, 'duration', true, 2)),
+	'Single-key numeric sorting must retain its existing behavior.'
+);
+
+$naturalNames = array(
+	array('id' => 'ten', 'name' => 'Item10'),
+	array('id' => 'two', 'name' => 'item2'),
+	array('id' => 'one', 'name' => 'ITEM1'),
+);
+
+assertSameMultisort(
+	array('one', 'two', 'ten'),
+	sortedIds($sorter->sorte($naturalNames, 'name', true, 1)),
+	'Case-insensitive natural sorting must retain its existing behavior.'
+);
+
+$ties = array(
+	array('id' => 'first', 'score' => 10),
+	array('id' => 'second', 'score' => 10),
+	array('id' => 'third', 'score' => 10),
+);
+
+assertSameMultisort(
+	array('first', 'second', 'third'),
+	sortedIds($sorter->sorte($ties, 'score', false, 2)),
+	'Rows equal across all criteria must retain their input order.'
+);
+
+assertSameMultisort(
+	$rankings,
+	$sorter->sorte($rankings),
+	'Calling sorte() without criteria must leave the input unchanged.'
+);
+
+echo "Multisort tests passed." . PHP_EOL;