Heavily optimize SnakList::moveSnaksToBottom (#762)

Author: thiemowmde

src/Snak/SnakList.php

@@ -158,53 +158,33 @@ public function getHash() {
 	}
 
 	/**
-	 * Orders the snaks in the list grouping them by property.
+	 * Groups snaks by property, and optionally orders them.
 	 *
-	 * @param string[] $order List of serliazed property ids to order by.
+	 * @param string[] $order List of property ID strings to order by. Snaks with other properties
+	 *  will also be grouped, but put at the end, in the order each property appeared first in the
+	 *  original list.
 	 *
 	 * @since 0.5
 	 */
 	public function orderByProperty( array $order = [] ) {
-		$snaksByProperty = $this->getSnaksByProperty();
-		$orderedProperties = array_unique( array_merge( $order, array_keys( $snaksByProperty ) ) );
-
-		foreach ( $orderedProperties as $property ) {
-			if ( array_key_exists( $property, $snaksByProperty ) ) {
-				$snaks = $snaksByProperty[$property];
-				$this->moveSnaksToBottom( $snaks );
-			}
+		$byProperty = array_fill_keys( $order, [] );
+
+		/** @var Snak $snak */
+		foreach ( $this as $snak ) {
+			$byProperty[$snak->getPropertyId()->getSerialization()][] = $snak;
 		}
-	}
 
-	/**
-	 * @param Snak[] $snaks to remove and re add
-	 */
-	private function moveSnaksToBottom( array $snaks ) {
-		foreach ( $snaks as $snak ) {
-			$this->removeSnak( $snak );
-			$this->addSnak( $snak );
+		$ordered = [];
+		foreach ( $byProperty as $snaks ) {
+			$ordered = array_merge( $ordered, $snaks );
 		}
-	}
 
-	/**
-	 * Gets the snaks in the current object in an array
-	 * grouped by property id
-	 *
-	 * @return array[]
-	 */
-	private function getSnaksByProperty() {
-		$snaksByProperty = [];
+		$this->exchangeArray( $ordered );
 
-		foreach ( $this as $snak ) {
-			/** @var Snak $snak */
-			$propertyId = $snak->getPropertyId()->getSerialization();
-			if ( !isset( $snaksByProperty[$propertyId] ) ) {
-				$snaksByProperty[$propertyId] = [];
-			}
-			$snaksByProperty[$propertyId][] = $snak;
+		$index = 0;
+		foreach ( $ordered as $snak ) {
+			$this->offsetHashes[$snak->getHash()] = $index++;
 		}
-
-		return $snaksByProperty;
 	}
 
 	/**

tests/unit/Snak/SnakListTest.php

@@ -165,6 +165,7 @@ public function orderByPropertyProvider() {
 		$id1 = new PropertyId( 'P1' );
 		$id2 = new PropertyId( 'P2' );
 		$id3 = new PropertyId( 'P3' );
+		$id4 = new PropertyId( 'P4' );
 
 		/**
 		 * List of test data containing snaks to initialize SnakList objects. The first list of
@@ -238,6 +239,27 @@ public function orderByPropertyProvider() {
 				],
 				[ 'P1' ]
 			],
+			'Multiple IDs in order' => [
+				[
+					new PropertyValueSnak( $id1, new StringValue( 'a' ) ),
+					new PropertyValueSnak( $id2, new StringValue( 'b' ) ),
+					new PropertyValueSnak( $id1, new StringValue( 'c' ) ),
+					new PropertyValueSnak( $id3, new StringValue( 'd' ) ),
+					new PropertyValueSnak( $id4, new StringValue( 'e' ) ),
+					new PropertyValueSnak( $id2, new StringValue( 'f' ) ),
+					new PropertyValueSnak( $id4, new StringValue( 'g' ) ),
+				],
+				[
+					new PropertyValueSnak( $id2, new StringValue( 'b' ) ),
+					new PropertyValueSnak( $id2, new StringValue( 'f' ) ),
+					new PropertyValueSnak( $id3, new StringValue( 'd' ) ),
+					new PropertyValueSnak( $id1, new StringValue( 'a' ) ),
+					new PropertyValueSnak( $id1, new StringValue( 'c' ) ),
+					new PropertyValueSnak( $id4, new StringValue( 'e' ) ),
+					new PropertyValueSnak( $id4, new StringValue( 'g' ) ),
+				],
+				[ 'P2', 'P3', 'P1' ]
+			],
 		];
 
 		$arguments = [];
@@ -272,6 +294,16 @@ public function testOrderByProperty( SnakList $snakList, SnakList $expected, arr
 		} else {
 			$this->assertNotSame( $initialSnakList->getHash(), $snakList->getHash() );
 		}
+
+		/** @var Snak $snak */
+		foreach ( $snakList as $snak ) {
+			$hash = $snak->getHash();
+			$this->assertSame(
+				$hash,
+				$snakList->getSnak( $hash )->getHash(),
+				'Reordering must not mess up the lists internal state'
+			);
+		}
 	}
 
 	public function testComparableInterface() {