Refactoring loops

Recently I was writing some code which manipulated a string based on the contents of an array. When I first wrote the code there were four methods which each iterated over a passed array. I saw this and thought it was a clear opportunity to refactor my code by moving the iteration into its own method. However as I was just about to do so I realized that the road was blocked; each method performed a slightly different operation inside the loop, so while it was clear that I had some redundancy in my system it was not clear how I could factor it out and preserve the unique contents inside the loops. Additionally I was uncertain whether it was worth the pursuing the common code which consisted of a single line. That was the end of the story, and the code never did get refactored. That said I was struck by a solution which I believe elegantly solves the problem outlined above and is well worth your time, and here it is: For the sake of example let's imagine the following two methods:

public function appendValues(array $fields) {
	foreach ($fields as $row) {
		$string .= $row;
	}
	return $string;
}
public function prependValues(array $fields) {
	foreach ($fields as $row) {
		$string = $row . $string;
	}
	return $string;
}

The first improvement that can be made is a trivial one, but it's worth noting it anyway. As you may have guessed I'm working in PHP 5 and taking advantage of type hinting. In this case I'm requiring that $fields be an array, but I'm not taking advantage of any of its strengths -- for instance random access. In light of this why not require a more general type which still satisfies my requirement that it be iterable? Fortunately, such an object already exists in the Standard PHP Library. It is the poorly named Traversable interface. Let's revisit the code with this change in place.

public function appendValues(Traversable $fields) {
	foreach ($fields as $row) {
		$string .= $row;
	}
	return $string;
}
public function prependValues(Traversable $fields) {
	foreach ($fields as $row) {
		$string = $row . $string;
	}
	return $string;
}

Now my code is not limited to working only with arrays. Any object which can be iterated over can be passed as a parameter. This trivial change makes it easier to switch what types I use in this context. For instance, I may use these methods in conjunction with a database abstraction object. Today the DAO may return an array, but tomorrow it may return a record set object. I am prepared to accommodate this change without needing to modify my code. With the first change out of the way it is time to attack the problem at its core. Remember the goal is to create a single method which encapsulates the loop shared between the original two. However the logic inside the loop varies from case to case. This sounds familiar, so let's try to encapsulate the logic which varies...

public function manipulateValues(Traversable $fields) {
	$string = "";
	foreach ($fields as $row) {
		$this->execute($row, $string);
	}
	return $string;
}
abstract public function execute($row, & $string);

The above has isolated the changing logic inside the execute method. However by moving the code into a virtual method a hierarchy has been established. Differing implementations will have to reside in children of the original class. However this behavior is not the same as what was started with. Now several of the children must be maintained outside of this class. While it is possible to build a proxy class which houses the children with their distinct implementations of execute, or use this class as a proxy for its own children, I believe that would be a step in the wrong direction. The children exist solely to implement a single method, and the method contains an algorithm for dealing with strings so why not employ the strategy pattern?

public function manipulateValues(Traversable $fields, Strategy $business) {
	$string = "";
	foreach ($fields as $row) {
		$business->execute($row, $string);
	}
	return $string;
}

The final implementation shown above collapses the class hierarchy established by the previous refactoring and replaces it with a single level of classes defined by an interface. Recall that a strategy should have no state itself (or constant state). This seems right given the class is intended to do. The solution has successfully reduced the pair of methods to a single one, and now the loop can be re-used. Let's look at what the new implementation might look like:

public function appendValues(Traversable $fields) {
	return $this->manipulateValues($fields, new AppendFields());
}
public function prependValues(Traversable $fields) {
	return $this->manipulateValues($fields, new PrependFields());
}
private function manipulateValues(Traversable $fields, Strategy $business) {
	$string = "";
	foreach ($fields as $row) {
		$business->execute($row, $string);
	}
	return $string;
}

These steps have brought the application greater flexibility by employing indirection, but was the trade worth it? I believe it was if only because it has reduced the number of loops I need to write. There are more important benefits though. For one the code is now much more readable. Reading and understanding an algorithm is rarely as easy as reading a clear method name, and even in the case of concatenation I believe the method name speaks louder. In addition, it is now trivial to add new string manipulation algorithms. All that is necessary is to create a new strategy housing the algorithm and add a wrapper method to the class. One final payoff is the ability to swap strategies dynamically. It may be possible to forgo the wrapper methods altogether and implement a clever solution which loads the appropriate wrapper based on some context. So next time you find yourself copying and pasting a loop stop and consider whether it wouldn't just be easier to take a minute and repeat the steps I outlined above.