5
\$\begingroup\$

I am building a config class for PHP which initially loads all given config files. To add files, users should be able to provide either a single file or a directory where files can be JSON or PHP.

Below is what I have so far, but it seems sluggish and inefficient to me. Is there a more elegant way to give users a wide choice of input? Should I split up the individual input cases into several private (or public?) methods? Are there any problems with this code apart from performance?

/**
* add function.
* merges the config data with another array
* 
* @access public
* @param string|array $input  the data to add
* @return void
*/

public function add($input)
{
    // if we got a string, it can be either JSON, a filename or a folder path.
    if (is_string($input)) {

      // if we got a directory, add each file separately again (except . and ..).
      if (is_dir($input)) {
        if (! is_readable($input)) throw new \RuntimeException('Directory "' . $input . '" is not readable.');

        // add each file in a directory to the config
        foreach (array_diff(scandir($input), array('..', '.')) as $file) {
          $this->add(rtrim($input, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR . $file);
        }
        return;
      }

      // if we got a filename, decide how to handle it based on the extension, then add its parsed content again
      if (is_file($input)) {
        if (! is_readable($input)) throw new \RuntimeException('File "' . $input . '" is not readable.');

        switch(pathinfo($input, PATHINFO_EXTENSION)) {
            case 'php':
              $content = require($input);
            break;

            case 'json':
              $content = $this->parseJSON(file_get_contents($input));
            break;
          }

        $this->add($content);
        return;
      }

      // string input is generally treated as JSON
      $this->add($this->parseJSON($input));
      return;
    }

    // if the input is an array, it must be plain config data, ready to merge in.
    if (is_array($input)) {
      $this->data = array_replace_recursive($this->data, $input);
      return;
    }

    // if we have no match, throw an exception (this happens if neither a string nor an array was given).
    throw new \RuntimeException('Provided data could not be parsed ("' . substr($input, 0, 20) . '...").');
    return;
}

To give a little context to the file extension switch: I used it to be able to implement more file extensions later on. I also thought about creating a parser interface for each type to be more expandable, that would prohibit further use of a single class file tho.

PHP config files consist of the following:

<?php return array(
  'foo' => 'bar'
);
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

I don't see glaring performance issues that should make this "sluggish".

But you could improve performance by extracting specialized logic to specialized functions:

  • add_file
  • add_dir
  • add_array
  • add_json

Because, in the current implementation, for example, when you scan a directory and call $this->add for each file, the recursive call will do unnecessary is_string, is_dir, is_file checks on it. Those checks don't need to be in the specialized add_file.

The same goes for the other input types, when you separate to multiple functions, there should be fewer unnecessary conditions to go through.

Users of the class, when they know that the input is JSON data, it will make sense that they should call add_json directly. In the end, I don't know what legitimate cases will be left for calling the generic add function. At that point, you can delete the function.


The return statement is pointless here:

throw new \RuntimeException('Provided data could not be parsed ("' . substr($input, 0, 20) . '...").');
return;

Instead of this:

    case 'php':
      $content = require($input);
    break;

The common practice is to align the break statement with the content of the case, like this:

    case 'php':
      $content = require($input);
      break;

It's recommended to use braces { ... } with single-statement blocks too, even they aren't strictly necessary.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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