0
\$\begingroup\$

Following code has some issues:

$args = func_get_args();
$paths = array();
foreach ($args as $arg) {
    $paths = array_merge($paths, (array)$arg);
}

$paths = array_map(create_function('$p', 'return trim($p, DIRECTORY_SEPARATOR);'), $paths);
$paths = array_filter($paths);
return DIRECTORY_SEPARATOR . join(DIRECTORY_SEPARATOR, $paths);
  • Unnecessary array_merge usage
  • Using depecated create_function function

Would using modern PHP 5.6+ features (anonymous functions etc.. ) help to improve the code?

\$\endgroup\$
  • 1
    \$\begingroup\$ You seem to be asking very specific questions about how you wish for the code to be rewritten for you. Please be aware reviewers may choose to disregard your questions and address any aspect of the code. \$\endgroup\$ – 200_success Jul 13 '16 at 6:57
1
\$\begingroup\$

Seems like the problem has been over thought. This would be how I would write it.

<?php

//$args = func_get_args();
$args = array('foo/', '/bar', 'fiz');

$paths = [];

foreach ($args as $arg) {
    $paths[] = trim($arg, DIRECTORY_SEPARATOR);
}

$paths = array_filter($paths);

echo DIRECTORY_SEPARATOR . join(DIRECTORY_SEPARATOR, $paths);

I don't see any need to leverage any exclusive PHP 5.6+ features to solve this.

\$\endgroup\$
1
\$\begingroup\$

If you want to use an anonymous function with array_map() your code could be written like this:

$args=array_map(function($v){return trim($v,DIRECTORY_SEPARATOR);},$args); // traverse array, remove leading&trailing slashes
$args=array_filter($args,'strlen'); // traverse array, remove elements with no string length
return DIRECTORY_SEPARATOR.implode(DIRECTORY_SEPARATOR,$args);  // prepend output string with slash, then join elements with slash

A drawback with this approach is that each line of code is traversing the array, which makes it less efficient. I suppose the benefits are fewer lines of code and that no new array variable is declared -- the original is simply overwritten.


Compare with this foreach loop version of the same method:

$path='';
foreach($args as $v){
    if(strlen($v=trim($v,DIRECTORY_SEPARATOR))){  // trim and check for positive length
        $path.=DIRECTORY_SEPARATOR.$v;            // prepend slash and concatenate
    }
}
if(!$path){$path=DIRECTORY_SEPARATOR;}            // provide default value when empty
return $path;

This approach traverses the array just once and builds the desired slash-delimited string (setting a default value if no elements qualify). I would argue that this is the more sensible method because it requires less function calls and iterations. Here is a demo.


An additional consideration:
You will notice that I used strlen() with array_filter() in my first method. This ensures that "zero-ish" values are not omitted from the array. If not instructed otherwise, array_filter() will remove all null, empty, false-y, and zero-ish values from an array. Encountering a 0 in your path elements is a low probability edge case, but this little gotcha can catch developers out if they are not aware of array_filter's default behavior.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.