Array_Swap function in PHP

Question

In PHP/Laravel I have the following function. Can you please review my approach?

private function array_swap_assoc($key1, $key2, $array)
    {
        $newArray = array();
        foreach ($array as $key => $value) {
            if ($key == $key1) {
                $newArray[$key2] = $array[$key2];
            } elseif ($key == $key2) {
                $newArray[$key1] = $array[$key1];
            } else {
                $newArray[$key] = $value;
            }
        }
        return $newArray;
    }

Answer 1

This will swap the values but is not very robust. If for instance one of the two keys is not defined this will generate notices and might cause sneaky errors further down in the code.

private function array_swap_assoc($key1, $key2, $array)

I don't see why that would be in a class. This is not realated to a specific group of activities. A simple function would do very well, and possibly be better (as in, respect the Single Responsibility Principle), as the class that contains it almost certainly is not a class containing exclusively array functions.

$newArray = array();

Creating a new array is a valid approach, but a different approach could be to pass the array as a reference to swap the values in the original array, unless you specifically need not to modify the original array. This will most of the time not do any noticeable difference, but it is more memory efficient:

private function array_swap_assoc($key1, $key2, &$array) {}

Notice the & in front of the variable. The $array local to this method will change the original one.

foreach ($array as $key => $value) {

It is a bit heavy handed to recreate the whole array. We only want to swap two values. Since we passed it by reference, we could just swap those two values without needing to reassign each and every key.

if ($key == $key1) {
    $newArray[$key2] = $array[$key2];
} elseif ($key == $key2) {
    $newArray[$key1] = $array[$key1];
} else {
    $newArray[$key] = $value;
}

The problem here is that there is no validation that both of the keys exist. We should validate that they do, and throw an exception if they don't.

if (array_key_exists($key1, $array) && array_key_exists($key2, $array) {}

Notice that I use array_key_exists and not isset, because the values could be null (which is a totally valid value) and null values will return false with isset.

So taking into account adding validation and using a passed by reference array, you could make that function more robust like this:

function array_swap_assoc($key1, $key2, &$array)
{
    if (array_key_exists($key1, $array) && array_key_exists($key2, $array)) {
        $value1 = $array[$key1];
        $value2 = $array[$key2];
        $array[$key1] = $value2;
        $array[$key2] = $value1;
    } else {
        throw new InvalidKeys;
    }
}

class InvalidKeys extends \Exception {}

---

array_swap_assoc('first', 'second', $array);
// no need to use the void return, the original array is modified

Answer 2

Your method custom sorts the array while maintaining all associative relationships.

To demonstrate with some data:

$array=['a'=>1,'b'=>2,'c'=>3,'d'=>4,'e'=>5];
var_export(array_swap_assoc('b','d',$array));

Output:

array (
  'a' => 1,
  'd' => 4,
  'c' => 3,
  'b' => 2,
  'e' => 5,
)

---

Félix Gagnon-Grenier's method does not provide the same effect and the function would be better named swap_values_by_key(). Notice the difference in output, only the values are re-ordered while the keys stay the same. This yields a set of dissociated elements.

array (
  'a' => 1,
  'b' => 4,
  'c' => 3,
  'd' => 2,
  'e' => 5,
)

If this is the result that you desire, (skipping the check for key existence) it can be achieved with a single temporary variable.

function simple_value_swap($key1,$key2,$array){
    $tmp=$array[$key1];
    $array[$key1]=$array[$key2];
    $array[$key2]=$tmp;
    return $array;
}

---

---

All that said, Félix's recommendation about checking for keys is advisable to avoid receiving a Notice about trying to access an element by a non-existent key.

I am certainly not against modifying variables by reference, but some developers discourage it.

If you are handling small arrays, then your posted method that iterates the entire array will not cause any noticeable impact on performance.

If you are handling a much larger array, and cannot sort the data more eloquently during an earlier process in your code, then you may or may not receive improved performance (I didn't do any benchmarking) with this:

function swap_assoc_offsets($key1,$key2,$array){
    $keys=array_keys($array);
    if(($offset1=array_search($key1,$keys))!==false && ($offset2=array_search($key2,$keys))!==false){
        $tmp=$keys[$offset1];
        $keys[$offset1]=$keys[$offset2];
        $keys[$offset2]=$tmp;
        return array_merge(array_flip($keys),$array);
    }
}

...p.s. Just for the sake of trying, here is a less eloquent method: (Demo)

function repack_slices($key1,$key2,$array){
    $keys=array_keys($array);
    if(($offset1=array_search($key1,$keys))!==false && ($offset2=array_search($key2,$keys))!==false){
        $max=max($offset1,$offset2);
        $min=min($offset1,$offset2);
        $result=array_slice($array,0,$min,true);  // front elements (possibly empty)
        $result+=array_slice($array,$max,1,true);  // the max/later element
        $result+=array_slice($array,$min+1,$max-1-$min,true);  // middle elements (possibly empty)
        $result+=array_slice($array,$min,1,true);  // the min/earlier element
        $result+=array_slice($array,++$max,sizeof($array)-$max,true);  // back elements (possibly empty)
        return $result;
    }
}